Re: [PATCH v5 02/16] device property: Add fwnode_graph_get_next_port_endpoint()

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

On Fri, Jul 17, 2026 at 07:05:56PM +0800, Chen-Yu Tsai wrote:
> On Fri, Jul 17, 2026 at 5:04 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Jul 16, 2026 at 04:52:27PM +0800, Chen-Yu Tsai wrote:
> > > On Wed, Jul 15, 2026 at 5:09 PM <sashiko-bot@kernel.org> wrote:

...

> > > > > +struct fwnode_handle *fwnode_graph_get_next_port_endpoint(const struct fwnode_handle *port,
> > > > > +                                                       struct fwnode_handle *prev)
> > > > > +{
> > > > > +     do {
> > > > > +             prev = fwnode_get_next_child_node(port, prev);
> > > >
> > > > [Severity: High]
> > > > This is a pre-existing issue, but iterating over endpoints using
> > > > fwnode_get_next_child_node() can cause an infinite loop and a use-after-free
> > > > if a device has secondary fwnodes attached.
> > > >
> > > > If a device port has both primary children and a secondary fwnode, the primary
> > > > ops finish and drop the reference to the last primary child, returning NULL.
> > > > Then, fwnode_get_next_child_node() incorrectly passes this same dropped child
> > > > pointer to the secondary ops:
> > > >
> > > > drivers/base/property.c:fwnode_get_next_child_node() {
> > > >         /* Try to find a child in primary fwnode */
> > > >         next = fwnode_call_ptr_op(fwnode, get_next_child_node, child);
> > > >         if (next)
> > > >                 return next;
> > > >
> > > >         /* When no more children in primary, continue with secondary */
> > > >         return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
> > > > }
> > > >
> > > > This can result in a double-put. Furthermore, when traversing secondary
> > > > children, the primary ops don't recognize the software node, treat it as NULL,
> > > > and return the first primary child again, creating an infinite loop.
> >
> > Yeah, it has to be implemented in a way how fwnode_graph_get_next_endpoint() is done.
> 
> If your patch series addresses the issue, I guess we can consider it
> covered? Sashiko labeled this a pre-existing.

I noticed that it addresses only main get_next case, but get_next_available is
still with the same issue. It's not that critical, we don't use much that API,
but still will be good to fix at some point.

> > > > [Severity: Medium]
> > > > Does this fail to unconditionally put the prev reference if the provided port
> > > > is invalid?
> > > >
> > > > If a caller passes a valid prev node but port is NULL or an error pointer,
> > > > fwnode_get_next_child_node() returns early without dropping prev:
> > > >
> > > > drivers/base/property.c:fwnode_get_next_child_node() {
> > > >         struct fwnode_handle *next;
> > > >
> > > >         if (IS_ERR_OR_NULL(fwnode))
> > > >                 return NULL;
> > > >     ...
> > > > }
> > > >
> > > > Could this lead to a reference count leak, violating the documented API
> > > > contract that prev is unconditionally put?
> > >
> > > This seems like what the other fwnode_get_next_*() functions are doing.
> > > I'd say this is a pre-existing issue as well.
> >
> > See this series (not yet applied, you even have chance to review it!):
> > 20260611203537.1786399-1-andriy.shevchenko@linux.intel.com
> 
> I believe that series addresses the first issue, not this one, even
> though it partially mitigates it.
> 
> The contract says "puts a reference to @prev unconditionally." What it
> doesn't say is that it requires @node to be valid, i.e. the function
> is a no-op if node == NULL.
> 
> "node != NULL" being an implicit requirement seems to be the norm for
> the fwnode_* and of_* APIs. It's not explicitly spelled out and Sashiko
> lacks the context for it.
> 
> I'm not sure this should be changed within this patch. It would end up
> as a mismatch between the various functions and possibly lead to more
> confusion.

How does node become invalid after that patch?

> > > > > +             if (fwnode_name_eq(prev, "endpoint"))
> > > >
> > > > [Severity: High]
> > > > Does hardcoding this string match break the fwnode graph abstraction for
> > > > non-DeviceTree platforms?
> > > >
> > > > By strictly matching the "endpoint" prefix, this bypasses the provider-specific
> > > > fwnode_operations, which could silently ignore valid ACPI and software node
> > > > endpoints that don't follow this exact naming scheme. Shouldn't this rely on
> > > > the backend-specific graph_get_next_endpoint operations instead?
> > >
> > > From drivers/acpi/property.c it seems that ACPI graphs follow the same
> > > structure. I don't have visibility into ACPI implementations though.
> >
> > Sashiko might be right. ACPI has device and data nodes, for device nodes the
> > name will be FourCC, so never longer than 4 characters. For data nodes, it
> > takes their names, which are arbitrary strings and seems should follow the given
> > schema. You need Sakari Ailus to review this patch.
> 
> OK. Will add Sakari in the next version.

Hmm... This is interesting. For any fwnode API changes you should add the
respective reviewers. Do you use tools or doing that manually? You should use
tools. I know that `b4` is capable of doing that, but I use a script [1] I
wrote a few years ago.

> > > We also have the following in include/linux/fwnode.h:
> > >
> > > #define SWNODE_GRAPH_PORT_NAME_FMT              "port@%u"
> > > #define SWNODE_GRAPH_ENDPOINT_NAME_FMT          "endpoint@%u"
> > >
> > > So this should not be a problem.
> > >
> > > > > +                     break;
> > > > > +     } while (prev);
> > > > > +
> > > > > +     return prev;
> > > > > +}

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko