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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Dec 03, 2024 at 03:27:31PM +0200, Andy Shevchenko wrote:
> 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.

The fact that some code is confusing does not mean that we should add
more confusing code. We will not fix everything at once, but we can make
things better bit by bit.

Look, the check where it is now makes total sense, you added it there
yourself! It checks that we are dealing with a valid node and returns
early. The intent is very easy to understand and the only thing that is
missing is that "put" operation to satisfy the documented behavior.
Anything more just makes things more complex for no good reason.

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

I think this will indeed help if we ever going to have more than primary
and secondary nodes. It is also tricky if you want to transition
seamlessly between different types of nodes (i.e. you have ACPI primary
with OF overlay secondary with swnode as tertiary etc). And you probably
want to add support for references between different typesof nodes
(i.e. swnode being able to reference OF device node for example).

This kind of rework is however out of scope of what I have time to do at
the moment.

Thanks.

-- 
Dmitry