Re: [PATCH] nfp: Fix memory leak in nfp_cpp_area_cache_add()

[Simon Horman <simon.horman@corigine.com>]

Hi Jianglei,

On Thu, Dec 09, 2021 at 02:15:11PM +0800, Jianglei Nie wrote:
> In line 800 (#1), nfp_cpp_area_alloc() allocates and initializes a
> CPP area structure. But in line 807 (#2), when the cache is allocated
> failed, this CPP area structure is not freed, which will result in
> memory leak.
> 
> We can fix it by freeing the CPP area when the cache is allocated
> failed (#2).
> 
> 792 int nfp_cpp_area_cache_add(struct nfp_cpp *cpp, size_t size)
> 793 {
> 794 	struct nfp_cpp_area_cache *cache;
> 795 	struct nfp_cpp_area *area;
> 
> 800	area = nfp_cpp_area_alloc(cpp, NFP_CPP_ID(7, NFP_CPP_ACTION_RW, 0),
> 801 				  0, size);
> 	// #1: allocates and initializes
> 
> 802 	if (!area)
> 803 		return -ENOMEM;
> 
> 805 	cache = kzalloc(sizeof(*cache), GFP_KERNEL);
> 806 	if (!cache)
> 807 		return -ENOMEM; // #2: missing free
> 
> 817	return 0;
> 818 }
> 
> Signed-off-by: Jianglei Nie <niejianglei2021@163.com>

Thanks for noticing this. I agree that this seems to be incorrect
and that your patch addresses the problem.

I do wonder if there is a value in adding:

Fixes: 4cb584e0ee7d ("nfp: add CPP access core")

Also, as I don't think this is hurting anything in practice, perhaps
this is for net-next (as oppoed to net), which is not specified
in the patch subject.

Regardless,

Acked-by: Simon Horman <simon.horman@corigine.com>

> ---
>  drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
> index d7ac0307797f..34c0d2ddf9ef 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c
> @@ -803,8 +803,10 @@ int nfp_cpp_area_cache_add(struct nfp_cpp *cpp, size_t size)
>  		return -ENOMEM;
>  
>  	cache = kzalloc(sizeof(*cache), GFP_KERNEL);
> -	if (!cache)
> +	if (!cache) {
> +		nfp_cpp_area_free(area);
>  		return -ENOMEM;
> +	}
>  
>  	cache->id = 0;
>  	cache->addr = 0;
> -- 
> 2.25.1
>