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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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