| Message ID | 20221122120039.760773-1-yangyingliang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] device property: fix of node refcount leak in fwnode_graph_get_next_endpoint() | expand |
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.
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.
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. >
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.
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.
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);
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(-)