Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto
@ 2019-07-23 10:35 Nishka Dasgupta
  2019-07-23 12:02 ` Robin Murphy
  0 siblings, 1 reply; 5+ messages in thread
From: Nishka Dasgupta @ 2019-07-23 10:35 UTC (permalink / raw)
  To: okaya, agross, vkoul, dan.j.williams, linux-arm-kernel,
	linux-arm-msm, dmaengine
  Cc: Nishka Dasgupta

Each iteration of for_each_available_child_of_node puts the previous
node, but in the case of a goto from the middle of the loop, there is
no put, thus causing a memory leak. Add an of_node_put before the
goto in 4 places.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
 drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
index 3022d66e7a33..209adc6ceabe 100644
--- a/drivers/dma/qcom/hidma_mgmt.c
+++ b/drivers/dma/qcom/hidma_mgmt.c
@@ -362,16 +362,22 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
 		struct platform_device *new_pdev;
 
 		ret = of_address_to_resource(child, 0, &res[0]);
-		if (!ret)
+		if (!ret) {
+			of_node_put(child);
 			goto out;
+		}
 
 		ret = of_address_to_resource(child, 1, &res[1]);
-		if (!ret)
+		if (!ret) {
+			of_node_put(child);
 			goto out;
+		}
 
 		ret = of_irq_to_resource(child, 0, &res[2]);
-		if (ret <= 0)
+		if (ret <= 0) {
+			of_node_put(child);
 			goto out;
+		}
 
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 		pdevinfo.fwnode = &child->fwnode;
@@ -386,6 +392,7 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
 		new_pdev = platform_device_register_full(&pdevinfo);
 		if (IS_ERR(new_pdev)) {
 			ret = PTR_ERR(new_pdev);
+			of_node_put(child);
 			goto out;
 		}
 		of_node_get(child);
-- 
2.19.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto
  2019-07-23 10:35 [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto Nishka Dasgupta
@ 2019-07-23 12:02 ` Robin Murphy
  2019-07-23 15:45   ` Sinan Kaya
  2019-07-24  8:05   ` Nishka Dasgupta
  0 siblings, 2 replies; 5+ messages in thread
From: Robin Murphy @ 2019-07-23 12:02 UTC (permalink / raw)
  To: Nishka Dasgupta, okaya, agross, vkoul, dan.j.williams,
	linux-arm-kernel, linux-arm-msm, dmaengine

On 23/07/2019 11:35, Nishka Dasgupta wrote:
> Each iteration of for_each_available_child_of_node puts the previous
> node, but in the case of a goto from the middle of the loop, there is
> no put, thus causing a memory leak. Add an of_node_put before the
> goto in 4 places.

Why not just add it once at the "out" label itself? (Consider the 
conditions for the loop terminating naturally)

And if you're cleaning up the refcounting here anyway then I'd also note 
that the reference held by the loop iterator makes the extra get/put 
inside that loop entirely redundant. It's always worth taking a look at 
the wider context rather than just blindly focusing on what a given 
script picks up - it's fairly rare that a piece of code has one obvious 
issue but is otherwise perfect.

Robin.

> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
>   drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/qcom/hidma_mgmt.c b/drivers/dma/qcom/hidma_mgmt.c
> index 3022d66e7a33..209adc6ceabe 100644
> --- a/drivers/dma/qcom/hidma_mgmt.c
> +++ b/drivers/dma/qcom/hidma_mgmt.c
> @@ -362,16 +362,22 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
>   		struct platform_device *new_pdev;
>   
>   		ret = of_address_to_resource(child, 0, &res[0]);
> -		if (!ret)
> +		if (!ret) {
> +			of_node_put(child);
>   			goto out;
> +		}
>   
>   		ret = of_address_to_resource(child, 1, &res[1]);
> -		if (!ret)
> +		if (!ret) {
> +			of_node_put(child);
>   			goto out;
> +		}
>   
>   		ret = of_irq_to_resource(child, 0, &res[2]);
> -		if (ret <= 0)
> +		if (ret <= 0) {
> +			of_node_put(child);
>   			goto out;
> +		}
>   
>   		memset(&pdevinfo, 0, sizeof(pdevinfo));
>   		pdevinfo.fwnode = &child->fwnode;
> @@ -386,6 +392,7 @@ static int __init hidma_mgmt_of_populate_channels(struct device_node *np)
>   		new_pdev = platform_device_register_full(&pdevinfo);
>   		if (IS_ERR(new_pdev)) {
>   			ret = PTR_ERR(new_pdev);
> +			of_node_put(child);
>   			goto out;
>   		}
>   		of_node_get(child);
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto
  2019-07-23 12:02 ` Robin Murphy
@ 2019-07-23 15:45   ` Sinan Kaya
  2019-07-24  8:05   ` Nishka Dasgupta
  1 sibling, 0 replies; 5+ messages in thread
From: Sinan Kaya @ 2019-07-23 15:45 UTC (permalink / raw)
  To: Robin Murphy, Nishka Dasgupta, agross, vkoul, dan.j.williams,
	linux-arm-kernel, linux-arm-msm, dmaengine

On 7/23/2019 8:02 AM, Robin Murphy wrote:
> Why not just add it once at the "out" label itself? (Consider the
> conditions for the loop terminating naturally)
> 

+1

>>
>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>> ---
>>   drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/hidma_mgmt.c
>> b/drivers/dma/qcom/hidma_mgmt.c
>> index 3022d66e7a33..209adc6ceabe 100644
>> --- a/drivers/dma/qcom/hidma_mgmt.c
>> +++ b/drivers/dma/qcom/hidma_mgmt.c
>> @@ -362,16 +362,22 @@ static int __init
>> hidma_mgmt_of_populate_channels(struct device_node *np)
>>           struct platform_device *new_pdev;
>>             ret = of_address_to_resource(child, 0, &res[0]);
>> -        if (!ret)
>> +        if (!ret) {
>> +            of_node_put(child);

The spacing on this also looks weird.

>>               goto out;
>> +        } 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto
  2019-07-23 12:02 ` Robin Murphy
  2019-07-23 15:45   ` Sinan Kaya
@ 2019-07-24  8:05   ` Nishka Dasgupta
  2019-07-24 10:10     ` Robin Murphy
  1 sibling, 1 reply; 5+ messages in thread
From: Nishka Dasgupta @ 2019-07-24  8:05 UTC (permalink / raw)
  To: Robin Murphy, okaya, agross, vkoul, dan.j.williams,
	linux-arm-kernel, linux-arm-msm, dmaengine

On 23/07/19 5:32 PM, Robin Murphy wrote:
> On 23/07/2019 11:35, Nishka Dasgupta wrote:
>> Each iteration of for_each_available_child_of_node puts the previous
>> node, but in the case of a goto from the middle of the loop, there is
>> no put, thus causing a memory leak. Add an of_node_put before the
>> goto in 4 places.
> 
> Why not just add it once at the "out" label itself? (Consider the 
> conditions for the loop terminating naturally)

If the loop terminates naturally then, as far as I understand, child 
will be put by the loop itself; then an extra of_node_put() under the 
out label would put the child node even though it has already been put. 
If I'm understanding this correctly (and I might not be) is it okay to 
decrement refcount more times that it is incremented?

> And if you're cleaning up the refcounting here anyway then I'd also note 
> that the reference held by the loop iterator makes the extra get/put 
> inside that loop entirely redundant. It's always worth taking a look at 
> the wider context rather than just blindly focusing on what a given 
> script picks up - it's fairly rare that a piece of code has one obvious 
> issue but is otherwise perfect.

Thank  you for pointing this out; I've added it in v2.

Thanking you,
Nishka
> Robin.
> 
>> Issue found with Coccinelle.
>>
>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>> ---
>>   drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/hidma_mgmt.c 
>> b/drivers/dma/qcom/hidma_mgmt.c
>> index 3022d66e7a33..209adc6ceabe 100644
>> --- a/drivers/dma/qcom/hidma_mgmt.c
>> +++ b/drivers/dma/qcom/hidma_mgmt.c
>> @@ -362,16 +362,22 @@ static int __init 
>> hidma_mgmt_of_populate_channels(struct device_node *np)
>>           struct platform_device *new_pdev;
>>           ret = of_address_to_resource(child, 0, &res[0]);
>> -        if (!ret)
>> +        if (!ret) {
>> +            of_node_put(child);
>>               goto out;
>> +        }
>>           ret = of_address_to_resource(child, 1, &res[1]);
>> -        if (!ret)
>> +        if (!ret) {
>> +            of_node_put(child);
>>               goto out;
>> +        }
>>           ret = of_irq_to_resource(child, 0, &res[2]);
>> -        if (ret <= 0)
>> +        if (ret <= 0) {
>> +            of_node_put(child);
>>               goto out;
>> +        }
>>           memset(&pdevinfo, 0, sizeof(pdevinfo));
>>           pdevinfo.fwnode = &child->fwnode;
>> @@ -386,6 +392,7 @@ static int __init 
>> hidma_mgmt_of_populate_channels(struct device_node *np)
>>           new_pdev = platform_device_register_full(&pdevinfo);
>>           if (IS_ERR(new_pdev)) {
>>               ret = PTR_ERR(new_pdev);
>> +            of_node_put(child);
>>               goto out;
>>           }
>>           of_node_get(child);
>>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto
  2019-07-24  8:05   ` Nishka Dasgupta
@ 2019-07-24 10:10     ` Robin Murphy
  0 siblings, 0 replies; 5+ messages in thread
From: Robin Murphy @ 2019-07-24 10:10 UTC (permalink / raw)
  To: Nishka Dasgupta, okaya, agross, vkoul, dan.j.williams,
	linux-arm-kernel, linux-arm-msm, dmaengine

On 24/07/2019 09:05, Nishka Dasgupta wrote:
> On 23/07/19 5:32 PM, Robin Murphy wrote:
>> On 23/07/2019 11:35, Nishka Dasgupta wrote:
>>> Each iteration of for_each_available_child_of_node puts the previous
>>> node, but in the case of a goto from the middle of the loop, there is
>>> no put, thus causing a memory leak. Add an of_node_put before the
>>> goto in 4 places.
>>
>> Why not just add it once at the "out" label itself? (Consider the 
>> conditions for the loop terminating naturally)
> 
> If the loop terminates naturally then, as far as I understand, child 
> will be put by the loop itself; then an extra of_node_put() under the 
> out label would put the child node even though it has already been put. 
> If I'm understanding this correctly (and I might not be) is it okay to 
> decrement refcount more times that it is incremented?

Ah, but is it really the same thing being put both times? The loop 
*iterator* will indeed drop its reference on the last valid child node, 
but what's the actual termination condition, and thus the state 
afterwards? ;)

Robin.

>> And if you're cleaning up the refcounting here anyway then I'd also 
>> note that the reference held by the loop iterator makes the extra 
>> get/put inside that loop entirely redundant. It's always worth taking 
>> a look at the wider context rather than just blindly focusing on what 
>> a given script picks up - it's fairly rare that a piece of code has 
>> one obvious issue but is otherwise perfect.
> 
> Thank  you for pointing this out; I've added it in v2.
> 
> Thanking you,
> Nishka
>> Robin.
>>
>>> Issue found with Coccinelle.
>>>
>>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
>>> ---
>>>   drivers/dma/qcom/hidma_mgmt.c | 13 ++++++++++---
>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma/qcom/hidma_mgmt.c 
>>> b/drivers/dma/qcom/hidma_mgmt.c
>>> index 3022d66e7a33..209adc6ceabe 100644
>>> --- a/drivers/dma/qcom/hidma_mgmt.c
>>> +++ b/drivers/dma/qcom/hidma_mgmt.c
>>> @@ -362,16 +362,22 @@ static int __init 
>>> hidma_mgmt_of_populate_channels(struct device_node *np)
>>>           struct platform_device *new_pdev;
>>>           ret = of_address_to_resource(child, 0, &res[0]);
>>> -        if (!ret)
>>> +        if (!ret) {
>>> +            of_node_put(child);
>>>               goto out;
>>> +        }
>>>           ret = of_address_to_resource(child, 1, &res[1]);
>>> -        if (!ret)
>>> +        if (!ret) {
>>> +            of_node_put(child);
>>>               goto out;
>>> +        }
>>>           ret = of_irq_to_resource(child, 0, &res[2]);
>>> -        if (ret <= 0)
>>> +        if (ret <= 0) {
>>> +            of_node_put(child);
>>>               goto out;
>>> +        }
>>>           memset(&pdevinfo, 0, sizeof(pdevinfo));
>>>           pdevinfo.fwnode = &child->fwnode;
>>> @@ -386,6 +392,7 @@ static int __init 
>>> hidma_mgmt_of_populate_channels(struct device_node *np)
>>>           new_pdev = platform_device_register_full(&pdevinfo);
>>>           if (IS_ERR(new_pdev)) {
>>>               ret = PTR_ERR(new_pdev);
>>> +            of_node_put(child);
>>>               goto out;
>>>           }
>>>           of_node_get(child);
>>>
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23 10:35 [PATCH] dma: qcom: hidma_mgmt: Add of_node_put() before goto Nishka Dasgupta
2019-07-23 12:02 ` Robin Murphy
2019-07-23 15:45   ` Sinan Kaya
2019-07-24  8:05   ` Nishka Dasgupta
2019-07-24 10:10     ` Robin Murphy

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox