| Message ID | 20241128053937.4076797-1-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] device property: do not leak child nodes when using NULL/error pointers | expand |
On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > The documentation to various API calls that locate children for a given > fwnode (such as fwnode_get_next_available_child_node() or > device_get_next_child_node()) states that the reference to the node > passed in "child" argument is dropped unconditionally, however the > change that added checks for the main node to be NULL or error pointer > broke this promise. > > Add missing fwnode_handle_put() calls to restore the documented > behavior. > > Fixes: 002752af7b89 ("device property: Allow error pointer to be passed to fwnode APIs") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/base/property.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 837d77e3af2b..696ba43b8e8a 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -759,6 +759,12 @@ struct fwnode_handle * > fwnode_get_next_child_node(const struct fwnode_handle *fwnode, > struct fwnode_handle *child) > { > + if (IS_ERR_OR_NULL(fwnode) || > + !fwnode_has_op(fwnode, get_next_child_node)) { > + fwnode_handle_put(child); > + return NULL; > + } > + > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); > } > EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); > @@ -778,9 +784,6 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, > { > struct fwnode_handle *next_child = child; > > - if (IS_ERR_OR_NULL(fwnode)) > - return NULL; > - > do { > next_child = fwnode_get_next_child_node(fwnode, next_child); > if (!next_child) > @@ -806,8 +809,10 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > const struct fwnode_handle *fwnode = dev_fwnode(dev); > struct fwnode_handle *next; > > - if (IS_ERR_OR_NULL(fwnode)) > + if (IS_ERR_OR_NULL(fwnode)) { > + fwnode_handle_put(child); > return NULL; > + } > > /* Try to find a child in primary fwnode */ > next = fwnode_get_next_child_node(fwnode, child); > -- > 2.47.0.338.g60cca15819-goog > Hi, This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree. You are receiving this message because of the following common error(s) as indicated below: - You have marked a patch with a "Fixes:" tag for a commit that is in an older released kernel, yet you do not have a cc: stable line in the signed-off-by area at all, which means that the patch will not be applied to any older kernel releases. To properly fix this, please follow the documented rules in the Documentation/process/stable-kernel-rules.rst file for how to resolve this. If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers. thanks, greg k-h's patch email bot
On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > The documentation to various API calls that locate children for a given > fwnode (such as fwnode_get_next_available_child_node() or > device_get_next_child_node()) states that the reference to the node > passed in "child" argument is dropped unconditionally, however the > change that added checks for the main node to be NULL or error pointer > broke this promise. This commit message doesn't explain a use case. Hence it might be just a documentation issue, please elaborate. > Add missing fwnode_handle_put() calls to restore the documented > behavior. ... While at it, please fix the kernel-doc (missing Return section). > { > + if (IS_ERR_OR_NULL(fwnode) || Unneeded check as fwnode_has_op() has it already. > + !fwnode_has_op(fwnode, get_next_child_node)) { > + fwnode_handle_put(child); > + return NULL; > + } > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); Now it's useless to call the macro, you can simply take the direct call. > } ... > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > const struct fwnode_handle *fwnode = dev_fwnode(dev); > struct fwnode_handle *next; > - if (IS_ERR_OR_NULL(fwnode)) > + if (IS_ERR_OR_NULL(fwnode)) { > + fwnode_handle_put(child); > return NULL; > + } > /* Try to find a child in primary fwnode */ > next = fwnode_get_next_child_node(fwnode, child); So, why not just moving the original check (w/o dropping the reference) here? Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()?
On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > The documentation to various API calls that locate children for a given > > fwnode (such as fwnode_get_next_available_child_node() or > > device_get_next_child_node()) states that the reference to the node > > passed in "child" argument is dropped unconditionally, however the > > change that added checks for the main node to be NULL or error pointer > > broke this promise. > > This commit message doesn't explain a use case. Hence it might be just > a documentation issue, please elaborate. I do not have a specific use case in mind, however the implementation behavior does not match the stated one, and so it makes sense to get it fixed. Otherwise callers would have to add checks to conditionally drop the reference to "child" argument in certain cases, which will complicate caller's code. > > > Add missing fwnode_handle_put() calls to restore the documented > > behavior. > > ... > > While at it, please fix the kernel-doc (missing Return section). OK. > > > { > > + if (IS_ERR_OR_NULL(fwnode) || > > Unneeded check as fwnode_has_op() has it already. Yes, it has, but that is not obvious nor it is a documented behavior of fwnode_has_op(). It also different semantics: it checks whether a fwnode implements a given operation, not whether fwnode is valid. That check is incidental in fwnode_has_op(). They all are macros so compiler should collapse duplicate checks, but if you feel really strongly about it I can drop IS_ERR_OR_NULL() check. > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > + fwnode_handle_put(child); > > + return NULL; > > + } > > > return fwnode_call_ptr_op(fwnode, get_next_child_node, child); > > Now it's useless to call the macro, you can simply take the direct call. OK, will change to a direct call. > > > } > > ... > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > struct fwnode_handle *next; > > > - if (IS_ERR_OR_NULL(fwnode)) > > + if (IS_ERR_OR_NULL(fwnode)) { > > + fwnode_handle_put(child); > > return NULL; > > + } > > > /* Try to find a child in primary fwnode */ > > next = fwnode_get_next_child_node(fwnode, child); > > So, why not just moving the original check (w/o dropping the reference) here? > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? Because if you rely on check in fwnode_get_next_child_node() you would not know if it returned NULL because there are no more children or because the node is invalid. In the latter case you can't dereference fwnode->secondary. Thanks.
On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > > The documentation to various API calls that locate children for a given > > > fwnode (such as fwnode_get_next_available_child_node() or > > > device_get_next_child_node()) states that the reference to the node > > > passed in "child" argument is dropped unconditionally, however the > > > change that added checks for the main node to be NULL or error pointer > > > broke this promise. > > > > This commit message doesn't explain a use case. Hence it might be just > > a documentation issue, please elaborate. > > I do not have a specific use case in mind, however the implementation > behavior does not match the stated one, and so it makes sense to get it > fixed. Otherwise callers would have to add checks to conditionally drop > the reference to "child" argument in certain cases, which will > complicate caller's code. Perhaps this should be somewhere between the cover letter / commit message? > > > Add missing fwnode_handle_put() calls to restore the documented > > > behavior. ... > > > { > > > + if (IS_ERR_OR_NULL(fwnode) || > > > > Unneeded check as fwnode_has_op() has it already. > > Yes, it has, but that is not obvious nor it is a documented behavior of > fwnode_has_op(). Would like to document that then? > It also different semantics: it checks whether a fwnode > implements a given operation, not whether fwnode is valid. That check is > incidental in fwnode_has_op(). I kinda disagree on this. The invalid fwnode may not have any operations, so it's implied and will always be like that. > They all are macros so compiler should collapse duplicate checks, but if > you feel really strongly about it I can drop IS_ERR_OR_NULL() check. Yes, please drop it and rather we want fwnode_has_op() to be documented with main purpose and guaranteed side effect (the latter makes no need of duplication that I pointed out). > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > > + fwnode_handle_put(child); > > > + return NULL; > > > + } ... > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > struct fwnode_handle *next; > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > + fwnode_handle_put(child); > > > return NULL; > > > + } > > > > > /* Try to find a child in primary fwnode */ > > > next = fwnode_get_next_child_node(fwnode, child); > > > > So, why not just moving the original check (w/o dropping the reference) here? > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > Because if you rely on check in fwnode_get_next_child_node() you would > not know if it returned NULL because there are no more children or > because the node is invalid. In the latter case you can't dereference > fwnode->secondary. Yes, so, how does it contradict my proposal?
On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: > > > > The documentation to various API calls that locate children for a given > > > > fwnode (such as fwnode_get_next_available_child_node() or > > > > device_get_next_child_node()) states that the reference to the node > > > > passed in "child" argument is dropped unconditionally, however the > > > > change that added checks for the main node to be NULL or error pointer > > > > broke this promise. > > > > > > This commit message doesn't explain a use case. Hence it might be just > > > a documentation issue, please elaborate. > > > > I do not have a specific use case in mind, however the implementation > > behavior does not match the stated one, and so it makes sense to get it > > fixed. Otherwise callers would have to add checks to conditionally drop > > the reference to "child" argument in certain cases, which will > > complicate caller's code. > > Perhaps this should be somewhere between the cover letter / commit message? OK, I thought that it was pretty obvious, but I will expand the commit message to include this. > > > > > Add missing fwnode_handle_put() calls to restore the documented > > > > behavior. > > ... > > > > > { > > > > + if (IS_ERR_OR_NULL(fwnode) || > > > > > > Unneeded check as fwnode_has_op() has it already. > > > > Yes, it has, but that is not obvious nor it is a documented behavior of > > fwnode_has_op(). > > Would like to document that then? > > > It also different semantics: it checks whether a fwnode > > implements a given operation, not whether fwnode is valid. That check is > > incidental in fwnode_has_op(). > > I kinda disagree on this. The invalid fwnode may not have any operations, > so it's implied and will always be like that. Yeah, it is clear that we disagree. I agree that invalid fwnode will not have an operation defined, still checking whether an operation is supported and whether a node is invalid or not are 2 different operations to me. But we do not need to argue further. > > > They all are macros so compiler should collapse duplicate checks, but if > > you feel really strongly about it I can drop IS_ERR_OR_NULL() check. > > Yes, please drop it and rather we want fwnode_has_op() to be documented with > main purpose and guaranteed side effect (the latter makes no need of > duplication that I pointed out). OK. > > > > > + !fwnode_has_op(fwnode, get_next_child_node)) { > > > > + fwnode_handle_put(child); > > > > + return NULL; > > > > + } > > ... > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > struct fwnode_handle *next; > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > + fwnode_handle_put(child); > > > > return NULL; > > > > + } > > > > > > > /* Try to find a child in primary fwnode */ > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > not know if it returned NULL because there are no more children or > > because the node is invalid. In the latter case you can't dereference > > fwnode->secondary. > > Yes, so, how does it contradict my proposal? I guess I misunderstood your proposal then. Could you please explain it in more detail? Thanks.
On Fri, Nov 29, 2024 at 11:16:54PM -0800, Dmitry Torokhov wrote: > On Fri, Nov 29, 2024 at 04:50:15PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 28, 2024 at 03:04:50PM -0800, Dmitry Torokhov wrote: > > > On Thu, Nov 28, 2024 at 03:13:16PM +0200, Andy Shevchenko wrote: > > > > On Wed, Nov 27, 2024 at 09:39:34PM -0800, Dmitry Torokhov wrote: ... > > > > > @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, > > > > > const struct fwnode_handle *fwnode = dev_fwnode(dev); > > > > > struct fwnode_handle *next; > > > > > > > > > - if (IS_ERR_OR_NULL(fwnode)) > > > > > + if (IS_ERR_OR_NULL(fwnode)) { > > > > > + fwnode_handle_put(child); > > > > > return NULL; > > > > > + } > > > > > > > > > /* Try to find a child in primary fwnode */ > > > > > next = fwnode_get_next_child_node(fwnode, child); > > > > > > > > So, why not just moving the original check (w/o dropping the reference) here? > > > > Wouldn't it have the same effect w/o explicit call to the fwnode_handle_put()? > > > > > > Because if you rely on check in fwnode_get_next_child_node() you would > > > not know if it returned NULL because there are no more children or > > > because the node is invalid. In the latter case you can't dereference > > > fwnode->secondary. > > > > Yes, so, how does it contradict my proposal? > > I guess I misunderstood your proposal then. Could you please explain it > in more detail? Current code (in steps): if (IS_ERR_OR_NULL()) check trying primary trying secondary if previous is NULL My proposal trying primary return if not NULL if (IS_ERR_OR_NULL()) check in its current form (no put op) trying secondary After your first patch IIUC this is possible as trying primary will put child uncoditionally.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 837d77e3af2b..696ba43b8e8a 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -759,6 +759,12 @@ struct fwnode_handle * fwnode_get_next_child_node(const struct fwnode_handle *fwnode, struct fwnode_handle *child) { + if (IS_ERR_OR_NULL(fwnode) || + !fwnode_has_op(fwnode, get_next_child_node)) { + fwnode_handle_put(child); + return NULL; + } + return fwnode_call_ptr_op(fwnode, get_next_child_node, child); } EXPORT_SYMBOL_GPL(fwnode_get_next_child_node); @@ -778,9 +784,6 @@ fwnode_get_next_available_child_node(const struct fwnode_handle *fwnode, { struct fwnode_handle *next_child = child; - if (IS_ERR_OR_NULL(fwnode)) - return NULL; - do { next_child = fwnode_get_next_child_node(fwnode, next_child); if (!next_child) @@ -806,8 +809,10 @@ struct fwnode_handle *device_get_next_child_node(const struct device *dev, const struct fwnode_handle *fwnode = dev_fwnode(dev); struct fwnode_handle *next; - if (IS_ERR_OR_NULL(fwnode)) + if (IS_ERR_OR_NULL(fwnode)) { + fwnode_handle_put(child); return NULL; + } /* Try to find a child in primary fwnode */ next = fwnode_get_next_child_node(fwnode, child);
The documentation to various API calls that locate children for a given fwnode (such as fwnode_get_next_available_child_node() or device_get_next_child_node()) states that the reference to the node passed in "child" argument is dropped unconditionally, however the change that added checks for the main node to be NULL or error pointer broke this promise. Add missing fwnode_handle_put() calls to restore the documented behavior. Fixes: 002752af7b89 ("device property: Allow error pointer to be passed to fwnode APIs") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/base/property.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)