Received-SPF: pass (google.com: domain of linux-kernel+bounces-30626-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <28aeb877-c0b4-4236-87d5-0bbaeb185656@cornelisnetworks.com>
Date: Thu, 18 Jan 2024 18:14:11 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] IB/hfi1: fix a memleak in init_credit_return
Content-Language: en-US
To: Leon Romanovsky <leon@kernel.org>, Zhipeng Lu <alexious@zju.edu.cn>
Cc: Jason Gunthorpe <jgg@ziepe.ca>,
 Ravi Krishnaswamy <ravi.krishnaswamy@intel.com>,
 Harish Chegondi <harish.chegondi@intel.com>,
 Brendan Cunningham <brendan.cunningham@intel.com>,
 linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240112085523.3731720-1-alexious@zju.edu.cn>
 <20240114090434.GD6404@unreal>
From: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
In-Reply-To: <20240114090434.GD6404@unreal>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aDhLaEVMNHBCaFhMMUtWMWVSTllicHhJZ0UxbWtpbTg0U0VWSVVVWDZPYndr?=
 =?utf-8?B?MFgrWGozaS9UbGdVc2JpVmVNUDMwa1o5M1JlL0RZT3A0dHZyaEZFY1ZvZGMx?=
 =?utf-8?B?WlR3RGVGT1hNNVJXT1UrNVl3djdnVWVzdDVOam5QNld0Q2wwczRHNjRvc09D?=
 =?utf-8?B?WGNpTnpvNUhPNlAyQmdSVXMrNHF3aEFhaU55bHYveHVZWmlwZUdSZDRaV3Za?=
 =?utf-8?B?QjljQU9UK2NoU0RQSGJzME5iSWkydVdsYWx1Tmo0dWg0eStyNTRmWjhRZWlQ?=
 =?utf-8?B?V3FsOGdINzdUa00zOVNYNlBwcEdnTG8rcTRFZCtLa3hWYjgwbll5VXE4dzZ2?=
 =?utf-8?B?Z2E2cUoybkRPby9KNjFaQ00wM09YUHA1ZWdBWUIvVmliaStrdHJxYW9qNFhI?=
 =?utf-8?B?S0w4WjNpNEJ4S0V3aFJqWHRjaW5HN3pZTUlMcWlTZUN6REVqM0cxWldXY0VO?=
 =?utf-8?B?Wi9OUHE5NGVBSXV4UnZ6aHJKNStLeUtESGJINmpWRWl0Tm9FeG4xbVhJSnhy?=
 =?utf-8?B?SjduT0V0MUJIZ09uWXdUc2k1bHZMRURVOUQ3OHlwUlNqZFpweGJGSXllYjlr?=
 =?utf-8?B?WGwvdWViYjNwMFRNenJWWHl1RC9HVlJzR1k0TXFINkhWNkZUQlo2OTdBdWVF?=
 =?utf-8?B?VlBTcFdaVTBkeHNTNWdZTnZTM0tjem1XcmZHNHJ4RXdKMm1uR3FWQlBiM3Nn?=
 =?utf-8?B?aWFkdmZjcVBVQ2RVbVJUL2VMKzQ4Z2dYSUJxNFJTazdVa0o1TDVHdVgzSS9C?=
 =?utf-8?B?SVRLR080c3JFMWl1SmJzWTg5WmpoR3B5N3YvaFI3ZnhuSC80UXJaSkVzalVN?=
 =?utf-8?B?WXhJS3pMOHpVMGJsYVZXQitpbXBxMC9KeU1FUERXNjRMWFVnL3RKN0tqMWFB?=
 =?utf-8?B?M29JRzIxUVR4NjRzUHhKd0dEUE9CbVEwQ3l0UFNkdXd3d3JQamN5cStmd204?=
 =?utf-8?B?L2lnemZWYTB6ZTN1T3lWYUJNMWFrNlQyWEFqc2dYQjRXRmlYNTBIS0tnbjdm?=
 =?utf-8?B?TUNTVkJGcGNxODJMWGh5akZFcTRpS3YybEI2Zys1aVZGMjB5V29OaDcwbmNy?=
 =?utf-8?B?NEZkNlllYTJ4SGdpWlZuUWI2SlBRWmwrTWhTWTJYZGJ1UnRneEJQWGNFMlVr?=
 =?utf-8?B?bVl6VjRqdmgvWnBQRTRTZ3czT3BESmhvL2VxV0ZKK1prY0t6RVNWZWl5MytG?=
 =?utf-8?B?b1hwdzhvQzdKOHpVQkdwNVRtbk5Rayt4eHV6ZmNmRUUzZFFKMUFHd09TV3pm?=
 =?utf-8?B?a0MwYUt3dTE1WXpkOXJXMDljY2k4UzYvV0ZmQ0prZWVqM1RtUXR5WSszSEoy?=
 =?utf-8?B?Y290aVNOQk4wQU1PdzNpaDFpTkJNU2ZJNW5heU1kelp1WUMzOGxwS09kK1pa?=
 =?utf-8?B?bGdxTkFRcnNRa3JYbm9HZ0ZURnRmQjh0dEphdXRBeW5vUk52NVE0aXN2TG5o?=
 =?utf-8?B?QjZMdnMrdzVaNi9NcDZ4QW1kR0pCRUdta2hONVk2Tis0UzlGMVpEbGhSYnUv?=
 =?utf-8?B?YnJHRlRmeUtOeFN3bEc4NGJyRkM1TlRxQW1BeDAwNEd1VXp6bXZxT2NpSTc1?=
 =?utf-8?B?UWxIM3ArN1NWaklXS3pRZ1doRXgySXBwWEZoTnBTQ1RFSXdxQnFqK3gwcUti?=
 =?utf-8?B?ckJpQmJpVlQ1ajU4dzZtRnM2QWVneGdnL2FLREs1UzE0aTBsWlNqYll4L2pG?=
 =?utf-8?B?SlRnK1BacHNrckRoZ0hxb2lsbDZRaDBTay9WSXZ5aDBFODdRZjlhRTc3citE?=
 =?utf-8?B?bHJUR1JrNkVsUVgxN09HeGo4Q0s1QTdHemQrUHMvK3lva0dQTExRdnhsTGUr?=
 =?utf-8?B?YlZ2dGl2VXA5c3ZQMkFtSTJSYWhYcUdPcUtyL1NWNW9qUjlOdjJYVUlMeGwx?=
 =?utf-8?B?dy9RNXhydG4xMzhzM3RYY1NRcDdxWWtJSzFoYVNMeFMrblFKWk1TbndhWEhz?=
 =?utf-8?B?WVJrWEJHLy9PZVpuUEx0MU5ZR0RtOXQvZ0VNUUd4dDJ3S0ljcENyN3RkYTdP?=
 =?utf-8?B?eEo5Z2JaRzlVcnNZRXpuSXZ3VitkWEtCcU9mdjJ6MHNBR1lTb1gxdUFWYmRs?=
 =?utf-8?B?V3BjVk9XeWl6Mys5V29yQlhJQVNCUm5rL3diQVlZM3IwYVZoQUczWnVxdEt2?=
 =?utf-8?B?QUNQQklEVEczaTFCM3pVWi9TUHdneFd0eEZVZEVzWFFOc1I5T3NKR2hvMTEv?=
 =?utf-8?Q?03h4krMy3H1QV0UfP2mu0ME=3D?=
X-OriginatorOrg: cornelisnetworks.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0c9f4a11-d5e0-4ac5-1bf6-08dc187b2f64
X-MS-Exchange-CrossTenant-AuthSource: BL0PR01MB4099.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2024 23:14:14.1827
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4dbdb7da-74ee-4b45-8747-ef5ce5ebe68a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qR1YBePYYciK9md0bZO2iOD/S33SLRrEnd117ywx9ApSG7nomDI8kLs/iYfgZ+rDpo70mIe72SooszryAAgGsMzjKpH0CT3JBeiqma7RvdGnrV2KE9n/GgK44uFgPSjv
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR01MB7020

On 1/14/24 4:04 AM, Leon Romanovsky wrote:
> On Fri, Jan 12, 2024 at 04:55:23PM +0800, Zhipeng Lu wrote:
>> When dma_alloc_coherent fails to allocate dd->cr_base[i].va,
>> init_credit_return should deallocate dd->cr_base and
>> dd->cr_base[i] that allocated before. Or those resources
>> would be never freed and a memleak is triggered.
>>
>> Fixes: 7724105686e7 ("IB/hfi1: add driver files")
>> Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn>
>> ---
>>  drivers/infiniband/hw/hfi1/pio.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/infiniband/hw/hfi1/pio.c b/drivers/infiniband/hw/hfi1/pio.c
>> index 68c621ff59d0..5a91cbda4aee 100644
>> --- a/drivers/infiniband/hw/hfi1/pio.c
>> +++ b/drivers/infiniband/hw/hfi1/pio.c
>> @@ -2086,7 +2086,7 @@ int init_credit_return(struct hfi1_devdata *dd)
>>  				   "Unable to allocate credit return DMA range for NUMA %d\n",
>>  				   i);
>>  			ret = -ENOMEM;
>> -			goto done;
>> +			goto free_cr_base;
>>  		}
>>  	}
>>  	set_dev_node(&dd->pcidev->dev, dd->node);
>> @@ -2094,6 +2094,10 @@ int init_credit_return(struct hfi1_devdata *dd)
>>  	ret = 0;
>>  done:
>>  	return ret;
>> +
>> +free_cr_base:
>> +	free_credit_return(dd);
> 
> Dennis,
> 
> The idea of this patch is right, but it made me wonder, if
> free_credit_return() is correct.

Yes, I've double checked the call path and if init_credit_return() fails we do
not call the free_credit_return().

So this patch:

Acked-by: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>


> 
> init_credit_return() iterates with help of for_each_node_with_cpus():
> 
>   2062 int init_credit_return(struct hfi1_devdata *dd)
>   2063 {
> ...
>   2075         for_each_node_with_cpus(i) {
>   2076                 int bytes = TXE_NUM_CONTEXTS * sizeof(struct credit_return);
>   2077
> 
> But free_credit_return uses something else:
>   2099 void free_credit_return(struct hfi1_devdata *dd)
>   2100 {
> ...
>   2105         for (i = 0; i < node_affinity.num_possible_nodes; i++) {
>   2106                 if (dd->cr_base[i].va) {
> 
> Thanks
> 
>> +	goto done;
>>  }
>>  
>>  void free_credit_return(struct hfi1_devdata *dd)

I think we are OK because the allocation uses node_affinity.num_possible_nodes
and in free_credit_return() we walk that entire array and if something is
allocated we free it.

Now why do we use for_each_node_with_cpus() at all? I believe that is because it
produces a subset of what is represented by num_possible_nodes(), which is OK
and doesn't leak anything.

-Denny