Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, May 29, 2024 at 06:19:33PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> > > > >  	}
> > > > >  
> > > > >  	/* Iterate through each output port to discover topology */
> > > > > -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > > > +	for_each_endpoint_of_node(parent, ep) {
> > > > >  		/*
> > > > >  		 * Legacy binding mixes input/output ports under the
> > > > >  		 * same parent. So, skip the input ports if we are dealing
> > > > 
> > > > I think there's a bug below. The loop contains
> > > > 
> > > > 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > > > 		if (ret)
> > > > 			return ret;
> > > > 
> > > > which leaks the reference to ep. This is not introduced by this patch,
> > > 
> > > Someone should create for_each_endpoint_of_node_scoped().
> > > 
> > > #define for_each_endpoint_of_node_scoped(parent, child) \
> > >         for (struct device_node *child __free(device_node) =           \
> > >              of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
> > >              child = of_graph_get_next_endpoint(parent, child))
> > 
> > I was thinking about that too :-) I wondered if we should then bother
> > taking and releasing references, given that references to the children
> > can't be leaked out of the loop. My reasoning was that the parent
> > device_node is guaranteed to be valid throughout the loop, so borrowing
> > references to children instead of creating new ones within the loop
> > should be fine. This assumes that endpoints and ports can't vanish while
> > the parent is there. Thinking further about it, it may not be a safe
> > assumption for the future. As we anyway use functions internally that
> > create new references, we can as well handle them correctly.
> > 
> > Using this new macro, the loop body would need to call of_node_get() if
> > it wants to get a reference out of the loop.
> 
> The child pointer is declared local to just the loop so you'd need
> create a different function scoped variable.  If it's not local to the
> loop then we'd end up taking a reference on each iteration and never
> releasing anything except on error paths.
> 
> > That's the right thing to
> > do, and I think it would be less error-prone than having to drop
> > references when exiting from the loop as we do today. It would still be
> > nice if we could have an API that allows catching this missing
> > of_node_get() automatically, but I don't see a simple way to do so at
> > the moment.
> 
> That's an interesting point.
> 
> If we did "function_scope_var = ep;" here then we'd need to take a
> second reference as you say.

Yes, that's what I meant above, sorry if that wasn't clear.

> With other cleanup stuff like kfree() it's
> very hard to miss it if we forget to call "no_free_ptr(&ep)" because
> it's on the success path.  It leads to an immediate crash in testing.
> But here it's just ref counting so possibly we might miss that sort of
> bug.

All this calls for std::shared_ptr<struct device_node> :-D

Jokes aside, I think for_each_endpoint_of_node_scoped() would still be
safer, as the number of cases where we would have to pass a reference to
the outer scope should be quite smaller than the number of cases where
we break from for_each_endpoint_of_node() loops today.

On the other hand, the consequence of a bug with
for_each_endpoint_of_node_scoped() would be a dangling reference,
instead of a reference leak with for_each_endpoint_of_node(), so it may
be more dangerous the same way that UAF is more dangerous than memory
leaks.

-- 
Regards,

Laurent Pinchart