| Message ID | 174114125011.1367354.2670882864492961789.stgit@dwillia2-xfh.jf.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | resource: Fix resource leak in get_free_mem_region() | expand |
On 05/03/2025 10:22, Dan Williams wrote: > 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). Make sense! > > 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> Thanks for your quickly fix! Tested-by: Li Zhijian <lizhijian@fujitsu.com> Comment inline below: > --- > > Andrew, here is a replacement patch for > resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It > addresses missing calls to remove_resource() in addition to > free_resource() in the error path. > > 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); IIUC 'this->parent' now points to base instead of the previous '&iomem_resource', which might change earlier logical assumptions elsewhere if other code tries to reference it Is it interned? Other looks good to me. Thanks Zhijian > + } 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 > * >
Zhijian Li (Fujitsu) wrote: > > > On 05/03/2025 10:22, Dan Williams wrote: > > 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). > > Make sense! > > > > > > 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> > > > Thanks for your quickly fix! > > Tested-by: Li Zhijian <lizhijian@fujitsu.com> > > Comment inline below: > > > > --- > > > > Andrew, here is a replacement patch for > > resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It > > addresses missing calls to remove_resource() in addition to > > free_resource() in the error path. > > > > 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); > > > IIUC 'this->parent' now points to base instead of the previous '&iomem_resource', > which might change earlier logical assumptions elsewhere if other code tries to > reference it > > Is it interned? Good question, it is intentional. I had the same concern that someone might have been dependent on it pointing to &iomem_resource even though @base is some other resource. However, if you take a look, all of the request_free_mem_region() callers set @base to &iomem_resource. Now, alloc_free_mem_region() *does* arrange for @base to be "cxlrd->res" (not &iomem_resource), but prior to this change no 'struct region_devres' was allocated for the alloc_free_mem_region() path. So there is no risk that something could have depended on "dr->parent == &iomem_resource" when @base is "cxlrd->res". > Other looks good to me. Thanks for the review, and the report!
On Tue, Mar 04, 2025 at 06:22:53PM -0800, Dan Williams wrote: > 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 The above lines are not important and may be removed from the commit message. > 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. ... > 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); > + } Why not positive conditional? if (this->res) { remove_resource(this->res); free_resource(this->res); } else { __release_region(this->parent, this->start, this->n); } > } ... > +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); sizeof(*dr) > + if (!dr) > + return ERR_PTR(-ENOMEM); > + } > + res = __get_free_mem_region(base, size, align, name, desc, flags); > + if (IS_ERR(res) || !dr) > + return res; But wouldn't be easier to utilise devm_add_action_or_reset()? > + 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; > +}
On Tue, 04 Mar 2025 18:22:53 -0800 Dan Williams <dan.j.williams@intel.com> wrote: > 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> > --- > > Andrew, here is a replacement patch for > resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It > addresses missing calls to remove_resource() in addition to > free_resource() in the error path. Fiddly code. So feel free to ignore comments below as even if these help, it's still code that requires more coffee than I have had today to follow easily. Jonathan > > +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; > + Avoid split of the constructor / destructor with a ternary though error check is worse... Hmm. struct region_devres *dr __free(kfree) = dev ? devres_alloc(devm_region_release, sizeof(*dr), GFP_KERNEL) : NULL; if (dev && !dr) return ERR_PTR(-ENOMEM); So on balance up to you, but at very least put that __free(kfree) direclty above the set. > + 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); > + Probably no blank line here would be nicer. > + if (IS_ERR(res) || !dr) This conflates error and don't care if error cases and briefly made me scratch my head. Maybe though it increase indent don't do early return on the !dr case. if (IS_ERR(res)) return res; if (dr) { dr->parent = base; ... } return res; > + 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 > * > >
On Fri, Mar 14, 2025 at 11:49:01AM +0000, Jonathan Cameron wrote: > On Tue, 04 Mar 2025 18:22:53 -0800 > Dan Williams <dan.j.williams@intel.com> wrote: ... > > + struct region_devres *dr __free(kfree) = NULL; > > + struct resource *res; > > + > Avoid split of the constructor / destructor with a ternary though > error check is worse... Hmm. > > struct region_devres *dr __free(kfree) = > dev ? devres_alloc(devm_region_release, sizeof(*dr), GFP_KERNEL) : NULL; > if (dev && !dr) > return ERR_PTR(-ENOMEM); > > So on balance up to you, I'm against ternary, it looks unreadable in this case. ... > but at very least put that __free(kfree) direclty > above the set. This I am neutral about. Any thing works for me. The rest you suggested is +1 from me. > > + 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); > > + > Probably no blank line here would be nicer. > > > + if (IS_ERR(res) || !dr) > > This conflates error and don't care if error cases and briefly > made me scratch my head. > > Maybe though it increase indent don't do early > return on the !dr case. > > if (IS_ERR(res)) > return res; > > if (dr) { > dr->parent = base; > > ... > > } > > return res; > > > + 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));
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 *
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> --- Andrew, here is a replacement patch for resource-fix-resource-leak-in-get_free_mem_region.patch in your tree. It addresses missing calls to remove_resource() in addition to free_resource() in the error path. kernel/resource.c | 105 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 60 insertions(+), 45 deletions(-)