Re: [PATCH] IB/hfi1: fix a memleak in init_credit_return

[Dennis Dalessandro]

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@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxxxxxxx>


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