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
