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

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

[Yang Yingliang]

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>
---
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..7a32582aaca8 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)
+	if (ep) {
+		fwnode_handle_put(port_parent);
 		return ep;
+	}
 
-	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
+	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
+	fwnode_handle_put(port_parent);
+	return ep;
 }
 EXPORT_SYMBOL_GPL(fwnode_graph_get_next_endpoint);
 
-- 
2.25.1

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:
> The 'parent' returned by fwnode_graph_get_port_parent()
> with refcount incremented when 'prev' is not null, it

NULL

> 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().

...

>  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)
> +	if (ep) {
> +		fwnode_handle_put(port_parent);
>  		return ep;
> +	}
>  
> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> +	fwnode_handle_put(port_parent);
> +	return ep;

It seems too complicated for the simple fix.

As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
branch. This will allow you to drop if (prev) at the end.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 02:54:36PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:

One more thing below.

...

> >  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)

> > +	if (ep) {
> > +		fwnode_handle_put(port_parent);
> >  		return ep;
> > +	}

	if (ep)
		goto out;

> > -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
> > +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);

out:

> > +	fwnode_handle_put(port_parent);
> > +	return ep;
> 
> It seems too complicated for the simple fix.
> 
> As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
> branch. This will allow you to drop if (prev) at the end.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 02:56:55PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 02:54:36PM +0200, Andy Shevchenko wrote:

...

> out:

Actually better name is

out_put_parent:

> > > +	fwnode_handle_put(port_parent);
> > > +	return ep;

-- 
With Best Regards,
Andy Shevchenko

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

[Yang Yingliang]

On 2022/11/22 20:58, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 02:56:55PM +0200, Andy Shevchenko wrote:
>> On Tue, Nov 22, 2022 at 02:54:36PM +0200, Andy Shevchenko wrote:
> ...
>
>> out:
> Actually better name is
>
> out_put_parent:
OK, change it in next version.

Thanks,
Yang
>>>> +	fwnode_handle_put(port_parent);
>>>> +	return ep;

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

[Yang Yingliang]

On 2022/11/22 20:54, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:
>> The 'parent' returned by fwnode_graph_get_port_parent()
>> with refcount incremented when 'prev' is not null, it
> NULL
>
>> 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().
> ...
>
>>   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)
>> +	if (ep) {
>> +		fwnode_handle_put(port_parent);
>>   		return ep;
>> +	}
>>   
>> -	return fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>> +	ep = fwnode_graph_get_next_endpoint(parent->secondary, NULL);
>> +	fwnode_handle_put(port_parent);
>> +	return ep;
> It seems too complicated for the simple fix.
>
> As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
> branch. This will allow you to drop if (prev) at the end.

fwnode is const, fwnode_handle_get doesn't accept this type.

>

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 09:12:41PM +0800, Yang Yingliang wrote:
> On 2022/11/22 20:54, Andy Shevchenko wrote:
> > On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:

...

> > It seems too complicated for the simple fix.
> > 
> > As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
> > branch. This will allow you to drop if (prev) at the end.
> 
> fwnode is const, fwnode_handle_get doesn't accept this type.

I'm talking about parent.

-- 
With Best Regards,
Andy Shevchenko

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

[Yang Yingliang]

On 2022/11/22 21:16, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 09:12:41PM +0800, Yang Yingliang wrote:
>> On 2022/11/22 20:54, Andy Shevchenko wrote:
>>> On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:
> ...
>
>>> It seems too complicated for the simple fix.
>>>
>>> As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
>>> branch. This will allow you to drop if (prev) at the end.
>> fwnode is const, fwnode_handle_get doesn't accept this type.
> I'm talking about parent.
You suggested this:

"Instead you might consider to replace

	parent = fwnode;

by

	parent = fwnode_handle_get(fwnode);"


It has compile warning:
drivers/base/property.c: In function ‘fwnode_graph_get_next_endpoint’:
drivers/base/property.c:1004:30: warning: passing argument 1 of ‘fwnode_handle_get’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
    parent = fwnode_handle_get(fwnode);
                               ^~~~~~
drivers/base/property.c:809:63: note: expected ‘struct fwnode_handle *’ but argument is of type ‘const struct fwnode_handle *’
  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)

~~~~~~~~~~~~~~~~~~~~~~^~~~~~

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 09:41:28PM +0800, Yang Yingliang wrote:
> On 2022/11/22 21:16, Andy Shevchenko wrote:
> > On Tue, Nov 22, 2022 at 09:12:41PM +0800, Yang Yingliang wrote:
> > > On 2022/11/22 20:54, Andy Shevchenko wrote:
> > > > On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:

...

> > > > It seems too complicated for the simple fix.
> > > > 
> > > > As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
> > > > branch. This will allow you to drop if (prev) at the end.
> > > fwnode is const, fwnode_handle_get doesn't accept this type.
> > I'm talking about parent.
> You suggested this:
> 
> "Instead you might consider to replace
> 
> 	parent = fwnode;
> 
> by
> 
> 	parent = fwnode_handle_get(fwnode);"
> 
> 
> It has compile warning:
> drivers/base/property.c: In function ‘fwnode_graph_get_next_endpoint’:
> drivers/base/property.c:1004:30: warning: passing argument 1 of ‘fwnode_handle_get’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
>    parent = fwnode_handle_get(fwnode);
>                               ^~~~~~
> drivers/base/property.c:809:63: note: expected ‘struct fwnode_handle *’ but argument is of type ‘const struct fwnode_handle *’
>  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
> 
> ~~~~~~~~~~~~~~~~~~~~~~^~~~~~

I see what you mean. Thank you for clarification.

So, it seems a bit twisted.

If prev == NULL, can the

        ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, NULL);

return NULL?

If no, we may move this case directly to the 'else' branch and return from there.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 04:07:10PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 09:41:28PM +0800, Yang Yingliang wrote:
> > On 2022/11/22 21:16, Andy Shevchenko wrote:
> > > On Tue, Nov 22, 2022 at 09:12:41PM +0800, Yang Yingliang wrote:
> > > > On 2022/11/22 20:54, Andy Shevchenko wrote:
> > > > > On Tue, Nov 22, 2022 at 08:00:39PM +0800, Yang Yingliang wrote:

...

> > > > > It seems too complicated for the simple fix.
> > > > > 
> > > > > As I said, just drop const qualifier and add fwnode_handle_get() in the 'else'
> > > > > branch. This will allow you to drop if (prev) at the end.
> > > > fwnode is const, fwnode_handle_get doesn't accept this type.
> > > I'm talking about parent.
> > You suggested this:
> > 
> > "Instead you might consider to replace
> > 
> > 	parent = fwnode;
> > 
> > by
> > 
> > 	parent = fwnode_handle_get(fwnode);"
> > 
> > 
> > It has compile warning:
> > drivers/base/property.c: In function ‘fwnode_graph_get_next_endpoint’:
> > drivers/base/property.c:1004:30: warning: passing argument 1 of ‘fwnode_handle_get’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >    parent = fwnode_handle_get(fwnode);
> >                               ^~~~~~
> > drivers/base/property.c:809:63: note: expected ‘struct fwnode_handle *’ but argument is of type ‘const struct fwnode_handle *’
> >  struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode)
> > 
> > ~~~~~~~~~~~~~~~~~~~~~~^~~~~~
> 
> I see what you mean. Thank you for clarification.
> 
> So, it seems a bit twisted.
> 
> If prev == NULL, can the
> 
>         ep = fwnode_call_ptr_op(parent, graph_get_next_endpoint, NULL);
> 
> return NULL?
> 
> If no, we may move this case directly to the 'else' branch and return from there.

Answering to my own question: unfortunately it's the case when we have no
endpoints for the fwnode, but might have for the secondary one.

Okay, let's proceed with your slightly modified version 2 (label) for now.

-- 
With Best Regards,
Andy Shevchenko