[PATCH] dmaengine: ti: k3-udma-private: Fix refcount leak bug in of_xudma_dev_get()

[PATCH] dmaengine: ti: k3-udma-private: Fix refcount leak bug in of_xudma_dev_get()

[Liang He]

We should call of_node_put() for the reference returned by
of_parse_phandle() in fail path or when it is not used anymore.
Here we only need to move the of_node_put() before the check.

Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine users")
Signed-off-by: Liang He <windhl@126.com>
---

I cannot find the 'k3-udma-private.c' comiple item in drivers/dma/ti/Makefile, 
I wonder if the author has forgotten the compile item and the
k3-udma-private.c has not been compiled before.

I have tried to add k3-udma-private.o in the Makefile, but there are lots of
compile errors.  
Please check it carefully and if possbile please teach me how to compile it, thanks.


 drivers/dma/ti/k3-udma-private.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
index d4f1e4e9603a..ec274ef7d5ea 100644
--- a/drivers/dma/ti/k3-udma-private.c
+++ b/drivers/dma/ti/k3-udma-private.c
@@ -31,14 +31,13 @@ struct udma_dev *of_xudma_dev_get(struct device_node *np, const char *property)
 	}
 
 	pdev = of_find_device_by_node(udma_node);
+	if (np != udma_node)
+		of_node_put(udma_node);
 	if (!pdev) {
 		pr_debug("UDMA device not found\n");
 		return ERR_PTR(-EPROBE_DEFER);
 	}
 
-	if (np != udma_node)
-		of_node_put(udma_node);
-
 	ud = platform_get_drvdata(pdev);
 	if (!ud) {
 		pr_debug("UDMA has not been probed\n");
-- 
2.25.1

Re: [PATCH] dmaengine: ti: k3-udma-private: Fix refcount leak bug in of_xudma_dev_get()

[Péter Ujfalusi]

Hi,

On 16/07/2022 11:46, Liang He wrote:
> We should call of_node_put() for the reference returned by
> of_parse_phandle() in fail path or when it is not used anymore.
> Here we only need to move the of_node_put() before the check.

Thank you for the analysis, yes, this is a bug.

If you add the missing blank line, you can attach my:
Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine users")
> Signed-off-by: Liang He <windhl@126.com>
> ---
> 
> I cannot find the 'k3-udma-private.c' comiple item in drivers/dma/ti/Makefile, 
> I wonder if the author has forgotten the compile item and the
> k3-udma-private.c has not been compiled before.
> 
> I have tried to add k3-udma-private.o in the Makefile, but there are lots of
> compile errors.  
> Please check it carefully and if possbile please teach me how to compile it, thanks.

the k3-udma-private.c is included in to k3-udma.c (see end of file).
When the UDMA stack was introduced we needed to have a glue layer to
service the networking users due to the lack of infrastructure via
DMAengine.
The glue layer needs to tap in to the DMAengine driver, but I did not
wanted to expose low level interfaces, structs, ops in order to be able
to get rid of the glue layer when we have all the needed features in
DMAengine.

The k3-udma-private.c is part of the k3-udma.c but kept as separate file
to make it explicit that it is suppose to go away and to not mix it with
the DMAengine driver.

>  drivers/dma/ti/k3-udma-private.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
> index d4f1e4e9603a..ec274ef7d5ea 100644
> --- a/drivers/dma/ti/k3-udma-private.c
> +++ b/drivers/dma/ti/k3-udma-private.c
> @@ -31,14 +31,13 @@ struct udma_dev *of_xudma_dev_get(struct device_node *np, const char *property)
>  	}
>  
>  	pdev = of_find_device_by_node(udma_node);
> +	if (np != udma_node)
> +		of_node_put(udma_node);

Can you add a blank line here for readability?

>  	if (!pdev) {
>  		pr_debug("UDMA device not found\n");
>  		return ERR_PTR(-EPROBE_DEFER);
>  	}
>  
> -	if (np != udma_node)
> -		of_node_put(udma_node);
> -
>  	ud = platform_get_drvdata(pdev);
>  	if (!ud) {
>  		pr_debug("UDMA has not been probed\n");

-- 
Péter

Re:Re: [PATCH] dmaengine: ti: k3-udma-private: Fix refcount leak bug in of_xudma_dev_get()

[Liang He]

At 2022-07-20 14:39:56, "Péter Ujfalusi" <peter.ujfalusi@gmail.com> wrote:
>Hi,
>
>On 16/07/2022 11:46, Liang He wrote:
>> We should call of_node_put() for the reference returned by
>> of_parse_phandle() in fail path or when it is not used anymore.
>> Here we only need to move the of_node_put() before the check.
>
>Thank you for the analysis, yes, this is a bug.
>
>If you add the missing blank line, you can attach my:
>Acked-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
>

Thanks very much for your effort to review my patch.

I will do that in new version soon.

>> Fixes: d70241913413 ("dmaengine: ti: k3-udma: Add glue layer for non DMAengine users")
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>> 
>> I cannot find the 'k3-udma-private.c' comiple item in drivers/dma/ti/Makefile, 
>> I wonder if the author has forgotten the compile item and the
>> k3-udma-private.c has not been compiled before.
>> 
>> I have tried to add k3-udma-private.o in the Makefile, but there are lots of
>> compile errors.  
>> Please check it carefully and if possbile please teach me how to compile it, thanks.
>
>the k3-udma-private.c is included in to k3-udma.c (see end of file).
>When the UDMA stack was introduced we needed to have a glue layer to
>service the networking users due to the lack of infrastructure via
>DMAengine.
>The glue layer needs to tap in to the DMAengine driver, but I did not
>wanted to expose low level interfaces, structs, ops in order to be able
>to get rid of the glue layer when we have all the needed features in
>DMAengine.
>
>The k3-udma-private.c is part of the k3-udma.c but kept as separate file
>to make it explicit that it is suppose to go away and to not mix it with
>the DMAengine driver.
>

Thanks for you explaination, this is a great lesson!

>>  drivers/dma/ti/k3-udma-private.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/dma/ti/k3-udma-private.c b/drivers/dma/ti/k3-udma-private.c
>> index d4f1e4e9603a..ec274ef7d5ea 100644
>> --- a/drivers/dma/ti/k3-udma-private.c
>> +++ b/drivers/dma/ti/k3-udma-private.c
>> @@ -31,14 +31,13 @@ struct udma_dev *of_xudma_dev_get(struct device_node *np, const char *property)
>>  	}
>>  
>>  	pdev = of_find_device_by_node(udma_node);
>> +	if (np != udma_node)
>> +		of_node_put(udma_node);
>
>Can you add a blank line here for readability?
>

Thanks, I will do it.

>>  	if (!pdev) {
>>  		pr_debug("UDMA device not found\n");
>>  		return ERR_PTR(-EPROBE_DEFER);
>>  	}
>>  
>> -	if (np != udma_node)
>> -		of_node_put(udma_node);
>> -
>>  	ud = platform_get_drvdata(pdev);
>>  	if (!ud) {
>>  		pr_debug("UDMA has not been probed\n");
>
>-- 
>Péter