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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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