1. Patchew
  2. linux
  3. [PATCH v2 0/7] dmaengine: idxd: fix memory leak in error handling path
  4. View patch

[PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs

Shuai Xue posted 7 patches 10 months, 1 week ago
There is a newer version of this series
[PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
Posted by Shuai Xue 10 months, 1 week ago 
Memory allocated for wqs is not freed if an error occurs during
idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
order of allocation before exiting the function in case of an error.

Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/dma/idxd/init.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index b946f78f85e1..b85736fd25bd 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 
 	idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
 	if (!idxd->wq_enable_map) {
-		kfree(idxd->wqs);
-		return -ENOMEM;
+		rc = -ENOMEM;
+		goto err_bitmap;
 	}
 
 	for (i = 0; i < idxd->max_wqs; i++) {
@@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
 		if (rc < 0) {
 			put_device(conf_dev);
+			kfree(wq);
 			goto err;
 		}
 
@@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
 		if (!wq->wqcfg) {
 			put_device(conf_dev);
+			kfree(wq);
 			rc = -ENOMEM;
 			goto err;
 		}
@@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 		if (idxd->hw.wq_cap.op_config) {
 			wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
 			if (!wq->opcap_bmap) {
+				kfree(wq->wqcfg);
 				put_device(conf_dev);
+				kfree(wq);
 				rc = -ENOMEM;
 				goto err;
 			}
@@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
 	return 0;
 
  err:
-	while (--i >= 0) {
+	while (i-- > 0) {
 		wq = idxd->wqs[i];
+		if (idxd->hw.wq_cap.op_config)
+			bitmap_free(wq->opcap_bmap);
+		kfree(wq->wqcfg);
 		conf_dev = wq_confdev(wq);
 		put_device(conf_dev);
+		kfree(wq);
+
 	}
+	bitmap_free(idxd->wq_enable_map);
+
+err_bitmap:
+	kfree(idxd->wqs);
+
 	return rc;
 }
 
-- 
2.39.3
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
Posted by Fenghua Yu 10 months ago 
Hi, Shuai,

On 2/14/25 21:44, Shuai Xue wrote:
> Memory allocated for wqs is not freed if an error occurs during
> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/dma/idxd/init.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index b946f78f85e1..b85736fd25bd 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   
>   	idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>   	if (!idxd->wq_enable_map) {
> -		kfree(idxd->wqs);
> -		return -ENOMEM;
> +		rc = -ENOMEM;
> +		goto err_bitmap;
>   	}
>   
>   	for (i = 0; i < idxd->max_wqs; i++) {
> @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
>   		if (rc < 0) {
>   			put_device(conf_dev);
> +			kfree(wq);
>   			goto err;
>   		}
>   
> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>   		if (!wq->wqcfg) {
>   			put_device(conf_dev);
> +			kfree(wq);
>   			rc = -ENOMEM;
>   			goto err;
>   		}
> @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   		if (idxd->hw.wq_cap.op_config) {
>   			wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
>   			if (!wq->opcap_bmap) {
> +				kfree(wq->wqcfg);
>   				put_device(conf_dev);
> +				kfree(wq);
>   				rc = -ENOMEM;
>   				goto err;
>   			}
> @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>   	return 0;
>   
>    err:
> -	while (--i >= 0) {
> +	while (i-- > 0) {

Why changed to "i-- > 0" here? Before coming to here, the mem areas 
allocated for wqs[i] are freed already and there is not need to free 
them again here, right? And if i>1, mem areas for wqs[0] won't be freed 
and will leak, right?

>   		wq = idxd->wqs[i];
> +		if (idxd->hw.wq_cap.op_config)
> +			bitmap_free(wq->opcap_bmap);
> +		kfree(wq->wqcfg);
>   		conf_dev = wq_confdev(wq);
>   		put_device(conf_dev);
> +		kfree(wq);
> +
>   	}
> +	bitmap_free(idxd->wq_enable_map);
> +
> +err_bitmap:
> +	kfree(idxd->wqs);
> +
>   	return rc;
>   }
>   

Thanks.


-Fenghua
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
Posted by Shuai Xue 10 months ago 

在 2025/2/19 00:32, Fenghua Yu 写道:
> Hi, Shuai,
> 
> On 2/14/25 21:44, Shuai Xue wrote:
>> Memory allocated for wqs is not freed if an error occurs during
>> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   drivers/dma/idxd/init.c | 20 +++++++++++++++++---
>>   1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index b946f78f85e1..b85736fd25bd 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>       idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>>       if (!idxd->wq_enable_map) {
>> -        kfree(idxd->wqs);
>> -        return -ENOMEM;
>> +        rc = -ENOMEM;
>> +        goto err_bitmap;
>>       }
>>       for (i = 0; i < idxd->max_wqs; i++) {
>> @@ -191,6 +191,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           rc = dev_set_name(conf_dev, "wq%d.%d", idxd->id, wq->id);
>>           if (rc < 0) {
>>               put_device(conf_dev);
>> +            kfree(wq);
>>               goto err;
>>           }
>> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>>           if (!wq->wqcfg) {
>>               put_device(conf_dev);
>> +            kfree(wq);
>>               rc = -ENOMEM;
>>               goto err;
>>           }
>> @@ -211,7 +213,9 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>           if (idxd->hw.wq_cap.op_config) {
>>               wq->opcap_bmap = bitmap_zalloc(IDXD_MAX_OPCAP_BITS, GFP_KERNEL);
>>               if (!wq->opcap_bmap) {
>> +                kfree(wq->wqcfg);
>>                   put_device(conf_dev);
>> +                kfree(wq);
>>                   rc = -ENOMEM;
>>                   goto err;
>>               }
>> @@ -225,11 +229,21 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>       return 0;
>>    err:
>> -    while (--i >= 0) {
>> +    while (i-- > 0) {
> 
> Why changed to "i-- > 0" here? Before coming to here, the mem areas allocated for wqs[i] are freed already and there is not need to free them again here, right? 

Yes.

> And if i>1, mem areas for wqs[0] won't be freed and will leak, right?

No, the two ways of writing are equivalent.

#include <stdio.h>

int main()
{
     int i = 1;
     while (i-- > 0)
         printf("freeing i %d\n", i);

     return 0;
}

// console output
// freeing i 0

I will drop this line to avoid confusion.

Thanks.
Shuai
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
Posted by Markus Elfring 10 months, 1 week ago 
…
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
…

Would other commits be more appropriate for the desired reference
(according to the affected function implementation)?

Examples:
2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap")
2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev")
2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime")


Regards,
Markus
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs
Posted by Shuai Xue 10 months ago 

在 2025/2/15 21:34, Markus Elfring 写道:
> …
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> …
> 
> Would other commits be more appropriate for the desired reference
> (according to the affected function implementation)?
> 
> Examples:
> 2022-09-29: de5819b994893197c71c86d21af10f85f50d6499 ("dmaengine: idxd: track enabled workqueues in bitmap")
> 2021-04-21: 700af3a0a26cbac87e4a0ae1dfa79597d0056d5f ("dmaengine: idxd: add 'struct idxd_dev' as wrapper for conf_dev")
> 2021-04-20: 7c5dd23e57c14cf7177b8a5e0fd08916e0c60005 ("dmaengine: idxd: fix wq conf_dev 'struct device' lifetime")
> 

Ok, I will add the fixes.

> 
> Regards,
> Markus

Thanks.
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()
Posted by Markus Elfring 10 months, 1 week ago 
> Memory allocated for wqs is not freed if an error occurs during
> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
> order of allocation before exiting the function in case of an error.
>
> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
…

Will a “stable tag” become relevant also for this patch series?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3


> +++ b/drivers/dma/idxd/init.c
> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
…
> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>  		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>  		if (!wq->wqcfg) {
>  			put_device(conf_dev);
> +			kfree(wq);
>  			rc = -ENOMEM;
>  			goto err;
>  		}
…

I got the impression that more common exception handling code could be moved
to additional jump targets at the end of such function implementations.
Will further adjustment opportunities be taken into account for
the affected resource management?

Regards,
Markus
Re: [PATCH v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()
Posted by Shuai Xue 10 months ago 

在 2025/2/15 19:00, Markus Elfring 写道:
>> Memory allocated for wqs is not freed if an error occurs during
>> idxd_setup_wqs(). To fix it, free the allocated memory in the reverse
>> order of allocation before exiting the function in case of an error.
>>
>> Fixes: a8563a33a5e2 ("dmanegine: idxd: reformat opcap output to match bitmap_parse() input")
> …
> 
> Will a “stable tag” become relevant also for this patch series?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
> 

I don't know if this is a real serious issue for stable kernel.
But I would like to add stable tag if the mantainer @Vinicius and @Dave ask.

> 
>> +++ b/drivers/dma/idxd/init.c
>> @@ -169,8 +169,8 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
> …
>> @@ -204,6 +205,7 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>   		wq->wqcfg = kzalloc_node(idxd->wqcfg_size, GFP_KERNEL, dev_to_node(dev));
>>   		if (!wq->wqcfg) {
>>   			put_device(conf_dev);
>> +			kfree(wq);
>>   			rc = -ENOMEM;
>>   			goto err;
>>   		}
> …
> 
> I got the impression that more common exception handling code could be moved
> to additional jump targets at the end of such function implementations.
> Will further adjustment opportunities be taken into account for
> the affected resource management?

Sure, I will move them to the jump targets.

> 
> Regards,
> Markus

Thanks for valuable comments.
Shuai
Re: [v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()
Posted by Markus Elfring 10 months ago 
>> Will a “stable tag” become relevant also for this patch series?
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
>
> I don't know if this is a real serious issue for stable kernel.
How many resource leaks would you like to avoid with your contributions?

Regards,
Markus
Re: [v2 1/7] dmaengine: idxd: fix memory leak in error handling path of idxd_setup_wqs()
Posted by Shuai Xue 10 months ago 

在 2025/2/16 17:34, Markus Elfring 写道:
>>> Will a “stable tag” become relevant also for this patch series?
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst?h=v6.14-rc2#n3
>>
>> I don't know if this is a real serious issue for stable kernel.
> How many resource leaks would you like to avoid with your contributions?

For this patch:

     - wqs = 8 * 8 bytes = 64 bytes
     - wq = 8 * 2400 bytes =  19200 bytes
     - opcap_bmap = 256 bit / 8 = 32 bytes

Around 18 KB.


> 
> Regards,
> Markus

Thanks.
Shuai

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.