linux-kernel - Re: [PATCH] IB/hfi1: fix a memleak in init_credit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240114090434.GD6404@unreal>
Date: Sun, 14 Jan 2024 11:04:34 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Zhipeng Lu <alexious@....edu.cn>
Cc: Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
	Jason Gunthorpe <jgg@...pe.ca>, Jubin John <jubin.john@...el.com>,
	Mitko Haralanov <mitko.haralanov@...el.com>,
	Ravi Krishnaswamy <ravi.krishnaswamy@...el.com>,
	Harish Chegondi <harish.chegondi@...el.com>,
	Brendan Cunningham <brendan.cunningham@...el.com>,
	linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] IB/hfi1: fix a memleak in init_credit_return

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