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@xxxxxxxxxxx>
> ---
>  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 --