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

[Leon Romanovsky <leon@kernel.org>]

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.

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