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

On Sat, Jul 18, 2026 at 12:33 AM Andy Shevchenko
<[email protected]> wrote:
>
> 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
> > <[email protected]> wrote:
> > > On Thu, Jul 16, 2026 at 04:52:27PM +0800, Chen-Yu Tsai wrote:
> > > > On Wed, Jul 15, 2026 at 5:09 PM <[email protected]> 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!):
> > > [email protected]
> >
> > 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?

Sashiko is arguing that the kernel-doc says "reference to @prev is _always_
put". But if you look at all the *_get_next_*() functions, they all just
return directly if @node is NULL.

I'm saying that this is implicit across the full API, and adding text
explaining it in just one of the functions creates a discrepancy. I can
add a patch before this to clarify the existing documentation, and change
the text in this patch as well, to make Sashiko happy.

Does that make sense?

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

Actually Sakari is already in the list of recipients.

I'm running get_maintainers.pl on all the files I changed, which is not
the formatted patches, but I don't think that affects the output much.
It's a bit less automated, but I sometimes trim the recipient list
slightly as I know they probably won't look at it.

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

Thanks for sharing!


ChenYu