| Message ID | 20250304043415.610286-1-lizhijian@fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | resource: Fix resource leak in get_free_mem_region() | expand |
On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote: > The leak is detected by the kernel memory leak detector (`kmemleak`) > following a `cxl create-region` failure: > > cxl_acpi ACPI0017:00: decoder0.0: created region2 > cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > > __kmalloc_cache_noprof+0x28c/0x350 > get_free_mem_region+0x45/0x380 > alloc_free_mem_region+0x1d/0x30 > size_store+0x180/0x290 [cxl_core] > kernfs_fop_write_iter+0x13f/0x1e0 > vfs_write+0x37c/0x540 > ksys_write+0x68/0xe0 > do_syscall_64+0x6e/0x190 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > kernel/resource.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 12004452d999..aa0b1da143eb 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base, > devres_free(dr); > } else if (dev) > devm_release_action(dev, remove_free_mem_region, res); > + else > + free_resource(res); It should use {} as per coding style: } else { free_resource(res); } (probably should add that to the previous branch too).
On 04/03/2025 17:22, Mika Westerberg wrote: > On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote: >> The leak is detected by the kernel memory leak detector (`kmemleak`) >> following a `cxl create-region` failure: >> >> cxl_acpi ACPI0017:00: decoder0.0: created region2 >> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] >> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) >> >> __kmalloc_cache_noprof+0x28c/0x350 >> get_free_mem_region+0x45/0x380 >> alloc_free_mem_region+0x1d/0x30 >> size_store+0x180/0x290 [cxl_core] >> kernfs_fop_write_iter+0x13f/0x1e0 >> vfs_write+0x37c/0x540 >> ksys_write+0x68/0xe0 >> do_syscall_64+0x6e/0x190 >> entry_SYSCALL_64_after_hwframe+0x76/0x7e >> >> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> >> --- >> kernel/resource.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/kernel/resource.c b/kernel/resource.c >> index 12004452d999..aa0b1da143eb 100644 >> --- a/kernel/resource.c >> +++ b/kernel/resource.c >> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base, >> devres_free(dr); >> } else if (dev) >> devm_release_action(dev, remove_free_mem_region, res); >> + else >> + free_resource(res); > > It should use {} as per coding style: The script/checkpatch.pl is happy with both of these 2 styles in practice. Thanks Zhijian > > } else { > free_resource(res); > } > > (probably should add that to the previous branch too).
On Tue, Mar 04, 2025 at 09:44:38AM +0000, Zhijian Li (Fujitsu) wrote: > > > On 04/03/2025 17:22, Mika Westerberg wrote: > > On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote: > >> The leak is detected by the kernel memory leak detector (`kmemleak`) > >> following a `cxl create-region` failure: > >> > >> cxl_acpi ACPI0017:00: decoder0.0: created region2 > >> cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] > >> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > >> > >> __kmalloc_cache_noprof+0x28c/0x350 > >> get_free_mem_region+0x45/0x380 > >> alloc_free_mem_region+0x1d/0x30 > >> size_store+0x180/0x290 [cxl_core] > >> kernfs_fop_write_iter+0x13f/0x1e0 > >> vfs_write+0x37c/0x540 > >> ksys_write+0x68/0xe0 > >> do_syscall_64+0x6e/0x190 > >> entry_SYSCALL_64_after_hwframe+0x76/0x7e > >> > >> Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") > >> Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > >> --- > >> kernel/resource.c | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/kernel/resource.c b/kernel/resource.c > >> index 12004452d999..aa0b1da143eb 100644 > >> --- a/kernel/resource.c > >> +++ b/kernel/resource.c > >> @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base, > >> devres_free(dr); > >> } else if (dev) > >> devm_release_action(dev, remove_free_mem_region, res); > >> + else > >> + free_resource(res); > > > > It should use {} as per coding style: > > > The script/checkpatch.pl is happy with both of these 2 styles in practice. It is but the coding style prefers then use {} around all branches, see: https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces } else if (dev) { devm_release_action(dev, remove_free_mem_region, res); } else { free_resource(res); }
On Tue, Mar 04, 2025 at 12:34:15PM +0800, Li Zhijian wrote: > The leak is detected by the kernel memory leak detector (`kmemleak`) > following a `cxl create-region` failure: > > cxl_acpi ACPI0017:00: decoder0.0: created region2 > cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > > __kmalloc_cache_noprof+0x28c/0x350 > get_free_mem_region+0x45/0x380 > alloc_free_mem_region+0x1d/0x30 > size_store+0x180/0x290 [cxl_core] The below lines have no importance to be present in the commit message. Please read Submitting Patches documentation that justifies my comment. > kernfs_fop_write_iter+0x13f/0x1e0 > vfs_write+0x37c/0x540 > ksys_write+0x68/0xe0 > do_syscall_64+0x6e/0x190 > entry_SYSCALL_64_after_hwframe+0x76/0x7e
Li Zhijian wrote: > The leak is detected by the kernel memory leak detector (`kmemleak`) > following a `cxl create-region` failure: > > cxl_acpi ACPI0017:00: decoder0.0: created region2 > cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] > kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) > > __kmalloc_cache_noprof+0x28c/0x350 > get_free_mem_region+0x45/0x380 > alloc_free_mem_region+0x1d/0x30 > size_store+0x180/0x290 [cxl_core] > kernfs_fop_write_iter+0x13f/0x1e0 > vfs_write+0x37c/0x540 > ksys_write+0x68/0xe0 > do_syscall_64+0x6e/0x190 > entry_SYSCALL_64_after_hwframe+0x76/0x7e > > Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") > Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> > --- > kernel/resource.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/resource.c b/kernel/resource.c > index 12004452d999..aa0b1da143eb 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base, > devres_free(dr); > } else if (dev) > devm_release_action(dev, remove_free_mem_region, res); > + else > + free_resource(res); This looks deceptively correct, but if the __insert_resource() call succeeded above then this needs to optionally be paired with remove_resource(). I think this function needs a rethink because mixing the devres, devm, and alloc_resource() failure cases makes mistakes like this hard to see. Here is a replacement proposal, only compile-tested: -- >8 -- From a01a28304e547da1f6287eecf3aeb0ebc6f48e2b Mon Sep 17 00:00:00 2001 From: Dan Williams <dan.j.williams@intel.com> Date: Tue, 4 Mar 2025 15:12:19 -0800 Subject: [PATCH] resource: Fix resource leak in get_free_mem_region() Li reports a kmemleak detection in get_free_mem_region() error unwind path: cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) __kmalloc_cache_noprof+0x28c/0x350 get_free_mem_region+0x45/0x380 alloc_free_mem_region+0x1d/0x30 size_store+0x180/0x290 [cxl_core] kernfs_fop_write_iter+0x13f/0x1e0 vfs_write+0x37c/0x540 ksys_write+0x68/0xe0 do_syscall_64+0x6e/0x190 entry_SYSCALL_64_after_hwframe+0x76/0x7e It turns out it not only leaks memory, also fails to unwind changes to the resource tree (@base, usually iomem_resource). Fix this by consolidating the devres and devm paths into just devres, and move those details to a wrapper function. So now __get_free_mem_region() only needs to worry about alloc_resource() unwinding, and the devres failure path is resolved before touching the resource tree. Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") Reported-by: Li Zhijian <lizhijian@fujitsu.com> Closes: http://lore.kernel.org/20250304043415.610286-1-lizhijian@fujitsu.com Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- kernel/resource.c | 105 ++++++++++++++++++++++++++-------------------- 1 file changed, 60 insertions(+), 45 deletions(-) diff --git a/kernel/resource.c b/kernel/resource.c index 12004452d999..80d10714cb38 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -172,6 +172,8 @@ static void free_resource(struct resource *res) kfree(res); } +DEFINE_FREE(free_resource, struct resource *, if (_T) free_resource(_T)) + static struct resource *alloc_resource(gfp_t flags) { return kzalloc(sizeof(struct resource), flags); @@ -1631,17 +1633,29 @@ void devm_release_resource(struct device *dev, struct resource *new) } EXPORT_SYMBOL(devm_release_resource); +/* + * The GFR_REQUEST_REGION case performs a request_region() to be paired + * with release_region(). The alloc_free_mem_region() path performs + * insert_resource() to be paired with {remove,free}_resource(). The + * @res member differentiates the 2 cases. + */ struct region_devres { struct resource *parent; resource_size_t start; resource_size_t n; + struct resource *res; }; static void devm_region_release(struct device *dev, void *res) { struct region_devres *this = res; - __release_region(this->parent, this->start, this->n); + if (!this->res) { + __release_region(this->parent, this->start, this->n); + } else { + remove_resource(this->res); + free_resource(this->res); + } } static int devm_region_match(struct device *dev, void *res, void *match_data) @@ -1908,43 +1922,19 @@ static resource_size_t gfr_next(resource_size_t addr, resource_size_t size, return addr + size; } -static void remove_free_mem_region(void *_res) -{ - struct resource *res = _res; - - if (res->parent) - remove_resource(res); - free_resource(res); -} - static struct resource * -get_free_mem_region(struct device *dev, struct resource *base, - resource_size_t size, const unsigned long align, - const char *name, const unsigned long desc, - const unsigned long flags) +__get_free_mem_region(struct resource *base, resource_size_t size, + const unsigned long align, const char *name, + const unsigned long desc, const unsigned long flags) { resource_size_t addr; - struct resource *res; - struct region_devres *dr = NULL; size = ALIGN(size, align); - res = alloc_resource(GFP_KERNEL); + struct resource *res __free(free_resource) = alloc_resource(GFP_KERNEL); if (!res) return ERR_PTR(-ENOMEM); - if (dev && (flags & GFR_REQUEST_REGION)) { - dr = devres_alloc(devm_region_release, - sizeof(struct region_devres), GFP_KERNEL); - if (!dr) { - free_resource(res); - return ERR_PTR(-ENOMEM); - } - } else if (dev) { - if (devm_add_action_or_reset(dev, remove_free_mem_region, res)) - return ERR_PTR(-ENOMEM); - } - write_lock(&resource_lock); for (addr = gfr_start(base, size, align, flags); gfr_continue(base, addr, align, flags); @@ -1958,17 +1948,9 @@ get_free_mem_region(struct device *dev, struct resource *base, size, name, 0)) break; - if (dev) { - dr->parent = &iomem_resource; - dr->start = addr; - dr->n = size; - devres_add(dev, dr); - } - res->desc = desc; write_unlock(&resource_lock); - /* * A driver is claiming this region so revoke any * mappings. @@ -1985,25 +1967,58 @@ get_free_mem_region(struct device *dev, struct resource *base, * Only succeed if the resource hosts an exclusive * range after the insert */ - if (__insert_resource(base, res) || res->child) + if (__insert_resource(base, res)) + break; + if (res->child) { + remove_resource(res); break; + } write_unlock(&resource_lock); } - return res; + return no_free_ptr(res); } write_unlock(&resource_lock); - if (flags & GFR_REQUEST_REGION) { - free_resource(res); - devres_free(dr); - } else if (dev) - devm_release_action(dev, remove_free_mem_region, res); - return ERR_PTR(-ERANGE); } +static struct resource * +get_free_mem_region(struct device *dev, struct resource *base, + resource_size_t size, const unsigned long align, + const char *name, const unsigned long desc, + const unsigned long flags) +{ + + struct region_devres *dr __free(kfree) = NULL; + struct resource *res; + + if (dev) { + dr = devres_alloc(devm_region_release, + sizeof(struct region_devres), GFP_KERNEL); + if (!dr) + return ERR_PTR(-ENOMEM); + } + + res = __get_free_mem_region(base, size, align, name, desc, flags); + + if (IS_ERR(res) || !dr) + return res; + + dr->parent = base; + dr->start = res->start; + dr->n = resource_size(res); + + /* See 'struct region_devres' definition for details */ + if ((flags & GFR_REQUEST_REGION) == 0) + dr->res = res; + + devres_add(dev, no_free_ptr(dr)); + + return res; +} + /** * devm_request_free_mem_region - find free region for device private memory *
diff --git a/kernel/resource.c b/kernel/resource.c index 12004452d999..aa0b1da143eb 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -2000,6 +2000,8 @@ get_free_mem_region(struct device *dev, struct resource *base, devres_free(dr); } else if (dev) devm_release_action(dev, remove_free_mem_region, res); + else + free_resource(res); return ERR_PTR(-ERANGE); }
The leak is detected by the kernel memory leak detector (`kmemleak`) following a `cxl create-region` failure: cxl_acpi ACPI0017:00: decoder0.0: created region2 cxl region2: HPA allocation error (-34) for size:0x0000000100000000 in CXL Window 0 [mem 0xa90000000-0x1a8fffffff flags 0x200] kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak) __kmalloc_cache_noprof+0x28c/0x350 get_free_mem_region+0x45/0x380 alloc_free_mem_region+0x1d/0x30 size_store+0x180/0x290 [cxl_core] kernfs_fop_write_iter+0x13f/0x1e0 vfs_write+0x37c/0x540 ksys_write+0x68/0xe0 do_syscall_64+0x6e/0x190 entry_SYSCALL_64_after_hwframe+0x76/0x7e Fixes: 14b80582c43e ("resource: Introduce alloc_free_mem_region()") Signed-off-by: Li Zhijian <lizhijian@fujitsu.com> --- kernel/resource.c | 2 ++ 1 file changed, 2 insertions(+)