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 <[email protected]>
---
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);
+ goto done;
}
void free_credit_return(struct hfi1_devdata *dd)
--
2.34.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
>
>
> >
> > 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,
--
Leon Romanovsky <[email protected]>