| Message ID | 20221123022542.2999510-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [v3] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint() | expand |
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 >
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 >>
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 >>> > .
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>
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); >
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?
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
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!
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);
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(-)