Re: [PATCH 1/2] device property: do not leak child nodes when using NULL/error pointers

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Dec 02, 2024 at 09:49:06PM -0800, Dmitry Torokhov wrote:
> On Sat, Nov 30, 2024 at 11:44:04PM +0200, Andy Shevchenko wrote:
> > 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.
> 
> Ah, I see. No, I do not think this is a good idea: it will make the code
> harder to understand for a casual reader: "Why do we check node validity
> only after we used it for the first time?"

Theare a re already a few API calls there that are hard to understand, I spent
some time on them to get it through and still got it wrong as this series
shows. So, I don't think we anyhow change this.

> For the code not in a hot path there is a lot of value in simplicity.

If you really want to go to this rabbit hole, think how we can get rid of
repetitive checks of the secondary or more if any in the future nodes in the
list.

So the basic idea is to have this all hidden (to some extent) behind the macro
or alike. In the code it would be something as

  for node in primary, secondary, ...
    call the API
    if (okay)
	return result

  return error

This will indeed help.

-- 
With Best Regards,
Andy Shevchenko