Received-SPF: pass (google.com: domain of linux-kernel+bounces-32062-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 21 Jan 2024 15:09:40 +0200
From: Leon Romanovsky <leon@kernel.org>
To: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>
Cc: Zhipeng Lu <alexious@zju.edu.cn>, 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
Subject: Re: [PATCH] IB/hfi1: fix a memleak in init_credit_return
Message-ID: <20240121130940.GA7547@unreal>
References: <20240112085523.3731720-1-alexious@zju.edu.cn>
 <20240114090434.GD6404@unreal>
 <28aeb877-c0b4-4236-87d5-0bbaeb185656@cornelisnetworks.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <28aeb877-c0b4-4236-87d5-0bbaeb185656@cornelisnetworks.com>

On Thu, Jan 18, 2024 at 06:14:11PM -0500, Dennis Dalessandro wrote:
> 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.

You are right, let's wait till merge window ends and we will apply this patch to rdma-rc.

Thanks

> 
> -Denny
>