| Message ID | 20240112085523.3731720-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | IB/hfi1: fix a memleak in init_credit_return | expand |
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 >
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. -Denny
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 >
On Fri, 12 Jan 2024 16:55:23 +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. > > Applied, thanks! [1/1] IB/hfi1: fix a memleak in init_credit_return https://git.kernel.org/rdma/rdma/c/809aa64ebff51e Best regards,
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); + goto done; } void free_credit_return(struct hfi1_devdata *dd)
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(-)