Date:   Thu, 9 Dec 2021 09:23:47 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Jianglei Nie <niejianglei2021@163.com>
Cc:     kuba@kernel.org, davem@davemloft.net, libaokun1@huawei.com,
        oss-drivers@corigine.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] nfp: Fix memory leak in nfp_cpp_area_cache_add()
Message-ID: <20211209082347.GC30443@corigine.com>
References: <20211209061511.122535-1-niejianglei2021@163.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211209061511.122535-1-niejianglei2021@163.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?o9gYQEMFeiPNGN73EbqimFvhPmFa3fdFSBAlasFAcCOKIPglp6zNvV4j/b/5?=
 =?us-ascii?Q?cWw9gAkMeHfvvEKA1Y//36IAmiQnq8LjysSfk8ZfDgRz/HX5jXkaRfrXzlE7?=
 =?us-ascii?Q?Vr6Qeig8FCx5no4thCc6TlVIhGdiQkO6mEWuKgJxzw+2Ip4ncAPgMDIoyWt3?=
 =?us-ascii?Q?mDJl/RzrXPqB3H6LBYD2PlB0Ndo96v4KZj1Q5XSfST5a1QcHXt0ysoFBd+Pm?=
 =?us-ascii?Q?vebXOp4MiePVW8rNgKcMiLEAC0F1PPBalz6CUL5uEzr9u5NJgCbClPe5szg4?=
 =?us-ascii?Q?/MTfWUBRdxUYTpl/H2cnNNpV5ntvoPSK+mi0W46YiflIkyH05ytQa53dtY+U?=
 =?us-ascii?Q?QC8UCDuQnUU4kaYHz0tQ/2gB9zYUO88UjZq9U3vAnNFPDp4fL8dJd48izImO?=
 =?us-ascii?Q?bqHkL2azDG/WP7IuMrNR6HxIf90IBIdVkWCrSss1m225kgVSmmCbCZTnQKTW?=
 =?us-ascii?Q?3/YVU5thR4UQhK49TCAh6wLn/FP/J1XSEnWmp3tZWF89KzvfOZ2ArdT/qrS3?=
 =?us-ascii?Q?8A4OF1R4rseiOtvt+GCvX6IpEjiduOvD85Ip8+YezuEVHOQ1wwczzbGoy96G?=
 =?us-ascii?Q?lIq1mpJnI00/0gCQfwVyqa+gZlAmzQ8nzFF3Tnv7STWfxLkjn76hhSr0yfre?=
 =?us-ascii?Q?3EbbNer9q8yJKB8ZCpibp9HJuwy/uOE8psw/Y1t5fyJlo+b2sJnDVPtD368D?=
 =?us-ascii?Q?l3eR5uFqXsyyhc11xi2osXl7E7yx/kAMkadCyUErEaoHwzwVMgFGwzBJrVJ8?=
 =?us-ascii?Q?MvB6xLApVh6ff7W9KGf4QHDlAN8aWwq4lgGndBeGmRefHVg/MtnmMWrR9luh?=
 =?us-ascii?Q?LKDU/z3kAG2P4LidL5/BAIDrD1erao8gVXY6ZNGrBiib4m0j5YK1SyEXXCTn?=
 =?us-ascii?Q?brHmsFBAhU3cYQaE7HecwXE6NPyE2F6EF2x0Nwxgd82/1JnpYGISHTEV94L1?=
 =?us-ascii?Q?ynfMMljlFzXZQPGDtKVdPxC6SZ6CE08EE5NDswwHSHRNbBOFq7Fb+NUga1YL?=
 =?us-ascii?Q?Cz6UgC5nbX6lwotQeHdlwDQRCmmnSTn1VuS38Znz/M0+ZzAj5Za6AYz5kAXr?=
 =?us-ascii?Q?/dw1m8UsRnO8qH2cSzkyxYwEDtVeoxoDHhLdmbGLqI89v4sPHnvHsQ1rgt8f?=
 =?us-ascii?Q?IdR5DQBbGIOK85oSqUTn8H/qPdF7Pz6y82dXriq+RInsknqMocMogiR+ekt6?=
 =?us-ascii?Q?+UBhC4nYZqNfAwY6ds8ILvgpWIiGk6F2S7FRXO5FAZd6VHtOIHfbaIKJHrzh?=
 =?us-ascii?Q?sMS+hD7KJrl12FyTdNrBsSgISrlwNZXUCOLx5WMLiHWLDxZhdM18l0qEntSb?=
 =?us-ascii?Q?8VPrNhnaJjZei125/+Nh63gCzYBgW0CSQlYo5yOapfc5ILhVOqudB8qzcGV6?=
 =?us-ascii?Q?nkneCOdRAr3NFA27+LnLVGY4mpm6vQR9E+eCBW553CwKWP3TEwLzP20+semh?=
 =?us-ascii?Q?l1scbpntITt1AMONQgPfsqf3uThkgwXfhxqNxcWCwk43Z2J09qw7oc09e6HG?=
 =?us-ascii?Q?wbnNRNNZP4VLHg2lvH5dBjqtMGwKQQXYbxe2fjoGHlFZD9iO4+qRSBUoIRK+?=
 =?us-ascii?Q?A4bJq9aKWBWt7DYm5jjFShL3sLRtIzchLnMwXmHIbULzQ64AhsiJwLX907En?=
 =?us-ascii?Q?dOMxIcDPmfqSFl+HQSlmSnmcrowlC3x985CgZ/sgxeibghc2iDaJsj/8R4XU?=
 =?us-ascii?Q?ROnHFwJG7Lltw5qxcQO8ibbzd61oHIr43aaNDy/tI38TnLsj6Pnm14DQk0CN?=
 =?us-ascii?Q?BzoagrpFF/icOWEDDKU5AnZQVccdGXo=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1786d4b2-fcf3-4dde-bb21-08d9baed3ce2
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Dec 2021 08:23:54.2029
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: gq0ebpdxWzCmxbl1AvLaFU36EaXVpiqvDdftkaQ5pGEWWm/o6IMRMtjj7sDfF0riLbnkeVRZJl+o3s7lVi0MW+pBN2eCjQn72R6UmPCP9UU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR13MB5716
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>