[PATCH] resource: Fix resource leak in get_free_mem_region()

[PATCH] resource: Fix resource leak in get_free_mem_region()

[Li Zhijian]

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);
 
 	return ERR_PTR(-ERANGE);
 }
-- 
2.47.0

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Mika Westerberg]

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).

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Zhijian Li (Fujitsu)]

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).

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Mika Westerberg]

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);
	}

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Dan Williams]

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
  *
-- 
2.48.1

[PATCH] resource: Fix resource leak in get_free_mem_region()

[Dan Williams]

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(-)

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
  *

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Zhijian Li (Fujitsu)]

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
>    *
> 

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Dan Williams]

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!

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Andy Shevchenko]

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;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Jonathan Cameron]

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
>   *
> 
> 

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Andy Shevchenko]

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));

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Zhijian Li (Fujitsu)]

Hello,

Ping, I hit his leak again in v6.15+

I'm surprised this patch has not been  merged into upstream.

Thanks
Zhijian


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).
> 
> 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(-)
> 
> 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
>    *
> 

Re: [PATCH] resource: Fix resource leak in get_free_mem_region()

[Andrew Morton]

On Wed, 4 Jun 2025 06:06:48 +0000 "Zhijian Li (Fujitsu)" <lizhijian@fujitsu.com> wrote:

> Hello,
> 
> Ping, I hit his leak again in v6.15+
> 
> I'm surprised this patch has not been  merged into upstream.

I'm struggling because I'm not subscribed to linux-cxl (please cc
linux-kernel on patches) but it appears that Dan's alternative patch is
due for an update (thanks to comments from Andy and Jonathan) and that
appears to have not happened.

So Dan, can you please take a look, see if we can get this finished?