[v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

[PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Yang Yingliang 3 years, 4 months ago

The 'parent' returned by fwnode_graph_get_port_parent()
with refcount incremented when 'prev' is not NULL, it
needs be put when finish using it.

Because the parent is const, introduce a new variable to
store the returned fwnode, then put it before returning
from fwnode_graph_get_next_endpoint().

Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
v2 -> v3:
  Add a out label.

v1 -> v2:
  Introduce a new variable to store the returned fwnode.
---
 drivers/base/property.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 2a5a37fcd998..7f338cb4fb7b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -989,26 +989,32 @@ struct fwnode_handle *
 fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
 			       struct fwnode_handle *prev)
 {
+	struct fwnode_handle *ep, *port_parent = NULL;
 	const struct fwnode_handle *parent;
-	struct fwnode_handle *ep;
 
 	/*
 	 * If this function is in a loop and the previous iteration returned
 	 * an endpoint from fwnode->secondary, then we need to use the secondary
 	 * as parent rather than @fwnode.
 	 */
-	if (prev)
-		parent = fwnode_graph_get_port_parent(prev);
-	else
+	if (prev) {
+		port_parent = fwnode_graph_get_port_parent(prev);
+		parent = port_parent;
+	} else {
 		parent = fwnode;
+	}
 	if (IS_ERR_OR_NULL(parent))
 		return NULL;
 
 	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
 	if (ep)
-		return ep;
+		goto out_put_port_parent;
+
+	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
 
-	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
+out_put_port_parent:
+	fwnode_handle_put(port_parent);
+	return ep;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
 
-- 
2.25.1

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Andy Shevchenko 3 years, 3 months ago

On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
> The 'parent' returned by fwnode_graph_get_port_parent()
> with refcount incremented when 'prev' is not NULL, it
> needs be put when finish using it.
> 
> Because the parent is const, introduce a new variable to
> store the returned fwnode, then put it before returning
> from fwnode_graph_get_next_endpoint().

Rafael, Greg, is this went through the cracks?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Greg KH 3 years, 3 months ago

On Wed, Dec 28, 2022 at 11:37:15AM +0200, Andy Shevchenko wrote:
> On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
> > The 'parent' returned by fwnode_graph_get_port_parent()
> > with refcount incremented when 'prev' is not NULL, it
> > needs be put when finish using it.
> > 
> > Because the parent is const, introduce a new variable to
> > store the returned fwnode, then put it before returning
> > from fwnode_graph_get_next_endpoint().
> 
> Rafael, Greg, is this went through the cracks?

Yes, but still in my queue.  I'll look at it when I get back from break.

thanks,

greg k-h

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Andy Shevchenko 3 years, 3 months ago

On Wed, Dec 28, 2022 at 10:46:12AM +0100, Greg KH wrote:
> On Wed, Dec 28, 2022 at 11:37:15AM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
> > > The 'parent' returned by fwnode_graph_get_port_parent()
> > > with refcount incremented when 'prev' is not NULL, it
> > > needs be put when finish using it.
> > > 
> > > Because the parent is const, introduce a new variable to
> > > store the returned fwnode, then put it before returning
> > > from fwnode_graph_get_next_endpoint().
> > 
> > Rafael, Greg, is this went through the cracks?
> 
> Yes, but still in my queue.  I'll look at it when I get back from break.

Have a nice one!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Daniel Scally 3 years, 4 months ago

Hi all - sorry that took so long

On 23/11/2022 02:25, Yang Yingliang wrote:
> The 'parent' returned by fwnode_graph_get_port_parent()
> with refcount incremented when 'prev' is not NULL, it
> needs be put when finish using it.
>
> Because the parent is const, introduce a new variable to
> store the returned fwnode, then put it before returning
> from fwnode_graph_get_next_endpoint().
>
> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---


This looks fine to me (thanks for fixing it), and it works fine on my
Surface:


Reviewed-and-tested-by: Daniel Scally <djrscally@gmail.com>

> v2 -> v3:
>   Add a out label.
>
> v1 -> v2:
>   Introduce a new variable to store the returned fwnode.
> ---
>  drivers/base/property.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..7f338cb4fb7b 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -989,26 +989,32 @@ struct fwnode_handle *
>  fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>  			       struct fwnode_handle *prev)
>  {
> +	struct fwnode_handle *ep, *port_parent = NULL;
>  	const struct fwnode_handle *parent;
> -	struct fwnode_handle *ep;
>  
>  	/*
>  	 * If this function is in a loop and the previous iteration returned
>  	 * an endpoint from fwnode->secondary, then we need to use the secondary
>  	 * as parent rather than @fwnode.
>  	 */
> -	if (prev)
> -		parent = fwnode_graph_get_port_parent(prev);
> -	else
> +	if (prev) {
> +		port_parent = fwnode_graph_get_port_parent(prev);
> +		parent = port_parent;
> +	} else {
>  		parent = fwnode;
> +	}
>  	if (IS_ERR_OR_NULL(parent))
>  		return NULL;
>  
>  	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
>  	if (ep)
> -		return ep;
> +		goto out_put_port_parent;
> +
> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>  
> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> +out_put_port_parent:
> +	fwnode_handle_put(port_parent);
> +	return ep;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Sakari Ailus 3 years, 4 months ago

Hi Yang,

Thank you for the patch.

On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
> The 'parent' returned by fwnode_graph_get_port_parent()
> with refcount incremented when 'prev' is not NULL, it
> needs be put when finish using it.
> 
> Because the parent is const, introduce a new variable to
> store the returned fwnode, then put it before returning
> from fwnode_graph_get_next_endpoint().
> 
> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Andy Shevchenko 3 years, 4 months ago

On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
> The 'parent' returned by fwnode_graph_get_port_parent()
> with refcount incremented when 'prev' is not NULL, it
> needs be put when finish using it.
> 
> Because the parent is const, introduce a new variable to
> store the returned fwnode, then put it before returning
> from fwnode_graph_get_next_endpoint().

To me this looks good enough. Not sure if Dan has a chance (time) to look at
this, though. And maybe even test...

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
> v2 -> v3:
>   Add a out label.
> 
> v1 -> v2:
>   Introduce a new variable to store the returned fwnode.
> ---
>  drivers/base/property.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 2a5a37fcd998..7f338cb4fb7b 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -989,26 +989,32 @@ struct fwnode_handle *
>  fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>  			       struct fwnode_handle *prev)
>  {
> +	struct fwnode_handle *ep, *port_parent = NULL;
>  	const struct fwnode_handle *parent;
> -	struct fwnode_handle *ep;
>  
>  	/*
>  	 * If this function is in a loop and the previous iteration returned
>  	 * an endpoint from fwnode->secondary, then we need to use the secondary
>  	 * as parent rather than @fwnode.
>  	 */
> -	if (prev)
> -		parent = fwnode_graph_get_port_parent(prev);
> -	else
> +	if (prev) {
> +		port_parent = fwnode_graph_get_port_parent(prev);
> +		parent = port_parent;
> +	} else {
>  		parent = fwnode;
> +	}
>  	if (IS_ERR_OR_NULL(parent))
>  		return NULL;
>  
>  	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
>  	if (ep)
> -		return ep;
> +		goto out_put_port_parent;
> +
> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>  
> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> +out_put_port_parent:
> +	fwnode_handle_put(port_parent);
> +	return ep;
>  }
>  EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>  
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Daniel Scally 3 years, 4 months ago

Hello

On 23/11/2022 13:31, Andy Shevchenko wrote:
> On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
>> The 'parent' returned by fwnode_graph_get_port_parent()
>> with refcount incremented when 'prev' is not NULL, it
>> needs be put when finish using it.
>>
>> Because the parent is const, introduce a new variable to
>> store the returned fwnode, then put it before returning
>> from fwnode_graph_get_next_endpoint().
> To me this looks good enough. Not sure if Dan has a chance (time) to look at
> this, though. And maybe even test...
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


Apologies; didn't notice this earlier. I will look at and test this today

>
>> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>> ---
>> v2 -> v3:
>>   Add a out label.
>>
>> v1 -> v2:
>>   Introduce a new variable to store the returned fwnode.
>> ---
>>  drivers/base/property.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index 2a5a37fcd998..7f338cb4fb7b 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -989,26 +989,32 @@ struct fwnode_handle *
>>  fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>>  			       struct fwnode_handle *prev)
>>  {
>> +	struct fwnode_handle *ep, *port_parent = NULL;
>>  	const struct fwnode_handle *parent;
>> -	struct fwnode_handle *ep;
>>  
>>  	/*
>>  	 * If this function is in a loop and the previous iteration returned
>>  	 * an endpoint from fwnode->secondary, then we need to use the secondary
>>  	 * as parent rather than @fwnode.
>>  	 */
>> -	if (prev)
>> -		parent = fwnode_graph_get_port_parent(prev);
>> -	else
>> +	if (prev) {
>> +		port_parent = fwnode_graph_get_port_parent(prev);
>> +		parent = port_parent;
>> +	} else {
>>  		parent = fwnode;
>> +	}
>>  	if (IS_ERR_OR_NULL(parent))
>>  		return NULL;
>>  
>>  	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
>>  	if (ep)
>> -		return ep;
>> +		goto out_put_port_parent;
>> +
>> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>>  
>> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>> +out_put_port_parent:
>> +	fwnode_handle_put(port_parent);
>> +	return ep;
>>  }
>>  EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>>  
>> -- 
>> 2.25.1
>>

Re: [PATCH v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint()

Posted by Yang Yingliang 3 years, 4 months ago

On 2022/11/25 17:32, Daniel Scally wrote:
> Hello
>
> On 23/11/2022 13:31, Andy Shevchenko wrote:
>> On Wed, Nov 23, 2022 at 10:25:42AM +0800, Yang Yingliang wrote:
>>> The 'parent' returned by fwnode_graph_get_port_parent()
>>> with refcount incremented when 'prev' is not NULL, it
>>> needs be put when finish using it.
>>>
>>> Because the parent is const, introduce a new variable to
>>> store the returned fwnode, then put it before returning
>>> from fwnode_graph_get_next_endpoint().
>> To me this looks good enough. Not sure if Dan has a chance (time) to look at
>> this, though. And maybe even test...
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Apologies; didn't notice this earlier. I will look at and test this today

Thanks,

I tested it, without this patch, I got this message:
OF: ERROR: memory leak, expected refcount 1 instead of 4, 
of_node_get()/of_node_put() unbalanced - destroy cset entry: attach 
overlay node /i2c/pmic@34/tcpc/connector
after this patch,  the message is gone.
>
>>> Fixes: b5b41ab6b0c1 ("device property: Check fwnode->secondary in fwnode_graph_get_next_endpoint()")
>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
>>> ---
>>> v2 -> v3:
>>>    Add a out label.
>>>
>>> v1 -> v2:
>>>    Introduce a new variable to store the returned fwnode.
>>> ---
>>>   drivers/base/property.c | 18 ++++++++++++------
>>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>>> index 2a5a37fcd998..7f338cb4fb7b 100644
>>> --- a/drivers/base/property.c
>>> +++ b/drivers/base/property.c
>>> @@ -989,26 +989,32 @@ struct fwnode_handle *
>>>   fwnode_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
>>>   			       struct fwnode_handle *prev)
>>>   {
>>> +	struct fwnode_handle *ep, *port_parent = NULL;
>>>   	const struct fwnode_handle *parent;
>>> -	struct fwnode_handle *ep;
>>>   
>>>   	/*
>>>   	 * If this function is in a loop and the previous iteration returned
>>>   	 * an endpoint from fwnode->secondary, then we need to use the secondary
>>>   	 * as parent rather than @fwnode.
>>>   	 */
>>> -	if (prev)
>>> -		parent = fwnode_graph_get_port_parent(prev);
>>> -	else
>>> +	if (prev) {
>>> +		port_parent = fwnode_graph_get_port_parent(prev);
>>> +		parent = port_parent;
>>> +	} else {
>>>   		parent = fwnode;
>>> +	}
>>>   	if (IS_ERR_OR_NULL(parent))
>>>   		return NULL;
>>>   
>>>   	ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, prev);
>>>   	if (ep)
>>> -		return ep;
>>> +		goto out_put_port_parent;
>>> +
>>> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>>>   
>>> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>>> +out_put_port_parent:
>>> +	fwnode_handle_put(port_parent);
>>> +	return ep;
>>>   }
>>>   EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
>>>   
>>> -- 
>>> 2.25.1
>>>
> .