Received-SPF: pass (google.com: domain of linux-kernel+bounces-25426-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 14 Jan 2024 11:04:34 +0200
From: Leon Romanovsky <leon@kernel.org>
To: Zhipeng Lu <alexious@zju.edu.cn>
Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Jason Gunthorpe <jgg@ziepe.ca>, Jubin John <jubin.john@intel.com>,
	Mitko Haralanov <mitko.haralanov@intel.com>,
	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
Subject: Re: [PATCH] IB/hfi1: fix a memleak in init_credit_return
Message-ID: <20240114090434.GD6404@unreal>
References: <20240112085523.3731720-1-alexious@zju.edu.cn>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240112085523.3731720-1-alexious@zju.edu.cn>

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
>