[PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[Xu Yang]

This series fixes two issues in the fwnode child iteration logic when
a secondary fwnode is present.

The first patch addresses a refcount imbalance in
software_node_get_next_child(). When a software node is used as a
secondary fwnode, the iteration code may incorrectly decrement the
refcount of child nodes that do not belong to the software node
hierarchy. This results in refcount underflow and possible use-after-free.

The second patch fixes an infinite loop in
fwnode_for_each_child_node(), caused by improper handling of iteration
state across primary and secondary fwnodes. When iterating over children
from both primary and secondary fwnodes, the code may incorrectly
resume iteration from the primary fwnode even when the current child
belongs to the secondary, leading to repeated traversal and a loop.

Both issues are triggered when mixing different fwnode types through the
secondary mechanism, and stem from incorrect assumptions about ownership
and traversal context of child nodes.

---
Changes in v2:
- use __free() to cleanup parent fwnode
- Link to v1: https://lore.kernel.org/r/20260525-fixes_fwnode_iteration-v1-0-a12903fb2919@nxp.com

---
Xu Yang (2):
      software node: fix refcount leak in software_node_get_next_child()
      device property: fix infinite loop in fwnode_for_each_child_node()

 drivers/base/property.c | 18 +++++++++++++++---
 drivers/base/swnode.c   | 14 +++++++-------
 2 files changed, 22 insertions(+), 10 deletions(-)
---
base-commit: b7bee4ca5688e30ca50fbc87b1b8f7eed7006c17
change-id: 20260525-fixes_fwnode_iteration-baf62d861305

Best regards,
--  
Xu Yang <xu.yang_2@nxp.com>

[PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

When a swnode acts as a secondary fwnode and is participates in child
iteration, a refcount leak occurs for the last child of the primary
fwnode's children.

                   Parent      Child
  (Primary fwnode)   FW:   {FW1, FW2, FW3}
(Secondary fwnode)   SW:   {}

In this case, FW3's refcount is decremented twice during iteration:

 fwnode_get_next_child_node(FW, FW3)
  1. fwnode_call_ptr_op(FW, get_next_child_node, FW3) returns NULL and
     decrements FW3's refcount
  2. fwnode_call_ptr_op(SW, get_next_child_node, FW3) returns NULL and
     decrements FW3's refcount again

The same double-decrement issue occurs when SW has children.

The kernel dump as below:

[   25.435805] OF: ERROR: of_node_release() detected bad of_node_put() on /soc/usb@4c010010/usb@4c100000
[   25.445072] CPU: 0 UID: 0 PID: 617 Comm: sh Not tainted 7.1.0-rc4-next-20260522-00011-g7376b330abca #210 PREEMPT
[   25.445080] Hardware name: NXP i.MX95 19X19 board (DT)
[   25.445083] Call trace:
[   25.445086]  show_stack+0x18/0x30 (C)
[   25.445101]  dump_stack_lvl+0x60/0x80
[   25.445108]  dump_stack+0x18/0x24
[   25.445113]  of_node_release+0x158/0x194
[   25.445122]  kobject_put+0xa0/0x120
[   25.445129]  of_node_put+0x18/0x28
[   25.445134]  of_fwnode_put+0x38/0x58
[   25.445141]  software_node_get_next_child+0x54/0x15c
[   25.445150]  fwnode_get_next_child_node+0x70/0x94
[   25.445156]  fwnode_get_next_available_child_node+0x34/0x88
[   25.445162]  device_links_driver_bound+0x2f4/0x334
[   25.445168]  driver_bound+0x68/0xb0
                ...
[   25.445258] OF: ERROR: next of_node_put() on this node will result in a kobject warning 'refcount_t: underflow; use-after-free.'

Fix this by ensuring software_node_get_next_child() does not decrement
the child's refcount when:
- The parent has no children, OR
- The parent has children but the input child is not a swnode

This prevents the refcount from being incorrectly decremented for
fwnodes that don't belong to the software node hierarchy.

Fixes: fb5ec981adf0 ("media: software_node: Fix refcounts in software_node_get_next_child()")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - no changes
---
 drivers/base/swnode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 869228a65cb3..507de464d387 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -474,18 +474,18 @@ software_node_get_next_child(const struct fwnode_handle *fwnode,
 	struct swnode *p = to_swnode(fwnode);
 	struct swnode *c = to_swnode(child);
 
-	if (!p || list_empty(&p->children) ||
-	    (c && list_is_last(&c->entry, &p->children))) {
-		fwnode_handle_put(child);
+	if (!p || list_empty(&p->children))
 		return NULL;
-	}
 
-	if (c)
+	if (c) {
+		fwnode_handle_put(child);
+		if (list_is_last(&c->entry, &p->children))
+			return NULL;
 		c = list_next_entry(c, entry);
-	else
+	} else {
 		c = list_first_entry(&p->children, struct swnode, entry);
+	}
 
-	fwnode_handle_put(child);
 	return fwnode_handle_get(&c->fwnode);
 }
 

-- 
2.34.1

[PATCH v2 2/2] device property: fix infinite loop in fwnode_for_each_child_node()

[Xu Yang]

From: Xu Yang <xu.yang_2@nxp.com>

When iterate over children of a fwnode that has a secondary fwnode,
fwnode_get_next_child_node() can enter an infinite loop if the secondary
fwnode has more than one child.

                       Parent        Child
      (Primary fwnode)   FWa:   {FWa1, FWa2, FWa3}
    (Secondary fwnode)   FWb:   {FWb1, FWb2}

In this case:

 ┌─> fwnode_get_next_child_node(FWa, FWa1)
 │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa1) returns FWa2
 │
 │   ...
 │
 │   fwnode_get_next_child_node(FWa, FWa3)
 │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa3) returns NULL
 │    - fwnode_call_ptr_op(FWb, get_next_child_node, FWa3) returns FWb1
 │
 │   fwnode_get_next_child_node(FWa, FWb1)
 │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWb1) returns FWa1
 └────┘

This cause fwnode_for_each_child_node() to loop indefinitely, reapeatedly
output {FWa1, FWa2, FWa3, FWb1, FWa1, ...}.

The root cause is that when the current child (FWb1) belongs to the
secondary fwnode, calling get_next_child_node() on the parimary fwnode
incorrectly returns the first child (FWa1) again instead of NULL.

Fix this by dynamically checking the parent fwnode of the current child
before calling get_next_child_node(). This approach follows the pattern
established in commit b5b41ab6b0c1 ("device property: Check
fwnode->secondary in fwnode_graph_get_next_endpoint()").

Fixes: 2692c614f8f0 ("device property: Allow secondary lookup in fwnode_get_next_child_node()")
Cc: stable@vger.kernel.org
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>

---
Changes in v2:
 - use __free() to put parent fwnode
---
 drivers/base/property.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index e08eadd66f4f..f51087065bf6 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -808,17 +808,29 @@ fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
 			   struct fwnode_handle *child)
 {
 	struct fwnode_handle *next;
+	const struct fwnode_handle *parent;
+	struct fwnode_handle *child_parent __free(fwnode_handle) = NULL;
 
 	if (IS_ERR_OR_NULL(fwnode))
 		return NULL;
+	/*
+	 * If this function is in a loop and the previous iteration returned
+	 * an child from fwnode->secondary, then we need to use the secondary
+	 * as parent rather than @fwnode.
+	 */
+	if (child) {
+		child_parent = fwnode_get_parent(child);
+		parent = child_parent;
+	} else {
+		parent = fwnode;
+	}
 
-	/* Try to find a child in primary fwnode */
-	next = fwnode_call_ptr_op(fwnode, get_next_child_node, child);
+	next = fwnode_call_ptr_op(parent, 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);
+	return fwnode_call_ptr_op(parent->secondary, get_next_child_node, NULL);
 }
 EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
 

-- 
2.34.1

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Andy Shevchenko]

On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:

> When a swnode acts as a secondary fwnode and is participates in child
> iteration, a refcount leak occurs for the last child of the primary
> fwnode's children.
> 
>                    Parent      Child
>   (Primary fwnode)   FW:   {FW1, FW2, FW3}
> (Secondary fwnode)   SW:   {}
> 
> In this case, FW3's refcount is decremented twice during iteration:
> 
>  fwnode_get_next_child_node(FW, FW3)
>   1. fwnode_call_ptr_op(FW, get_next_child_node, FW3) returns NULL and
>      decrements FW3's refcount
>   2. fwnode_call_ptr_op(SW, get_next_child_node, FW3) returns NULL and
>      decrements FW3's refcount again
> 
> The same double-decrement issue occurs when SW has children.
> 
> The kernel dump as below:
> 
> [   25.435805] OF: ERROR: of_node_release() detected bad of_node_put() on /soc/usb@4c010010/usb@4c100000
> [   25.445072] CPU: 0 UID: 0 PID: 617 Comm: sh Not tainted 7.1.0-rc4-next-20260522-00011-g7376b330abca #210 PREEMPT
> [   25.445080] Hardware name: NXP i.MX95 19X19 board (DT)
> [   25.445083] Call trace:
> [   25.445086]  show_stack+0x18/0x30 (C)
> [   25.445101]  dump_stack_lvl+0x60/0x80
> [   25.445108]  dump_stack+0x18/0x24
> [   25.445113]  of_node_release+0x158/0x194
> [   25.445122]  kobject_put+0xa0/0x120
> [   25.445129]  of_node_put+0x18/0x28
> [   25.445134]  of_fwnode_put+0x38/0x58
> [   25.445141]  software_node_get_next_child+0x54/0x15c
> [   25.445150]  fwnode_get_next_child_node+0x70/0x94
> [   25.445156]  fwnode_get_next_available_child_node+0x34/0x88
> [   25.445162]  device_links_driver_bound+0x2f4/0x334
> [   25.445168]  driver_bound+0x68/0xb0
>                 ...
> [   25.445258] OF: ERROR: next of_node_put() on this node will result in a kobject warning 'refcount_t: underflow; use-after-free.'
> 
> Fix this by ensuring software_node_get_next_child() does not decrement
> the child's refcount when:
> - The parent has no children, OR
> - The parent has children but the input child is not a swnode
> 
> This prevents the refcount from being incorrectly decremented for
> fwnodes that don't belong to the software node hierarchy.

...

>  	struct swnode *p = to_swnode(fwnode);
>  	struct swnode *c = to_swnode(child);
>  
> -	if (!p || list_empty(&p->children) ||
> -	    (c && list_is_last(&c->entry, &p->children))) {
> -		fwnode_handle_put(child);

Wouldn't be better to use swnode_get() / swnode_put() instead?
*Yes, we might need to add some NULL checks there.

> +	if (!p || list_empty(&p->children))
>  		return NULL;
> -	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[Andy Shevchenko]

On Wed, Jun 03, 2026 at 04:44:30PM +0800, Xu Yang wrote:
> This series fixes two issues in the fwnode child iteration logic when
> a secondary fwnode is present.
> 
> The first patch addresses a refcount imbalance in
> software_node_get_next_child(). When a software node is used as a
> secondary fwnode, the iteration code may incorrectly decrement the
> refcount of child nodes that do not belong to the software node
> hierarchy. This results in refcount underflow and possible use-after-free.
> 
> The second patch fixes an infinite loop in
> fwnode_for_each_child_node(), caused by improper handling of iteration
> state across primary and secondary fwnodes. When iterating over children
> from both primary and secondary fwnodes, the code may incorrectly
> resume iteration from the primary fwnode even when the current child
> belongs to the secondary, leading to repeated traversal and a loop.
> 
> Both issues are triggered when mixing different fwnode types through the
> secondary mechanism, and stem from incorrect assumptions about ownership
> and traversal context of child nodes.

Please, Cc Bart who is heavily working on software nodes these days.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] device property: fix infinite loop in fwnode_for_each_child_node()

[Andy Shevchenko]

On Wed, Jun 03, 2026 at 04:44:32PM +0800, Xu Yang wrote:

> When iterate over children of a fwnode that has a secondary fwnode,
> fwnode_get_next_child_node() can enter an infinite loop if the secondary
> fwnode has more than one child.
> 
>                        Parent        Child
>       (Primary fwnode)   FWa:   {FWa1, FWa2, FWa3}
>     (Secondary fwnode)   FWb:   {FWb1, FWb2}
> 
> In this case:
> 
>  ┌─> fwnode_get_next_child_node(FWa, FWa1)
>  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa1) returns FWa2
>  │
>  │   ...
>  │
>  │   fwnode_get_next_child_node(FWa, FWa3)
>  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa3) returns NULL
>  │    - fwnode_call_ptr_op(FWb, get_next_child_node, FWa3) returns FWb1
>  │
>  │   fwnode_get_next_child_node(FWa, FWb1)
>  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWb1) returns FWa1
>  └────┘
> 
> This cause fwnode_for_each_child_node() to loop indefinitely, reapeatedly
> output {FWa1, FWa2, FWa3, FWb1, FWa1, ...}.
> 
> The root cause is that when the current child (FWb1) belongs to the
> secondary fwnode, calling get_next_child_node() on the parimary fwnode
> incorrectly returns the first child (FWa1) again instead of NULL.
> 
> Fix this by dynamically checking the parent fwnode of the current child
> before calling get_next_child_node(). This approach follows the pattern
> established in commit b5b41ab6b0c1 ("device property: Check
> fwnode->secondary in fwnode_graph_get_next_endpoint()").

...

TBH, this code becomes twisted and complicated. Can we add some test cases to
show the problem? Also we need to add other possible combinations (somewhat
about ~5-6) of the different types of fwnode in a relationship.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[Xu Yang]

On Wed, Jun 03, 2026 at 12:43:06PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 03, 2026 at 04:44:30PM +0800, Xu Yang wrote:
> > This series fixes two issues in the fwnode child iteration logic when
> > a secondary fwnode is present.
> > 
> > The first patch addresses a refcount imbalance in
> > software_node_get_next_child(). When a software node is used as a
> > secondary fwnode, the iteration code may incorrectly decrement the
> > refcount of child nodes that do not belong to the software node
> > hierarchy. This results in refcount underflow and possible use-after-free.
> > 
> > The second patch fixes an infinite loop in
> > fwnode_for_each_child_node(), caused by improper handling of iteration
> > state across primary and secondary fwnodes. When iterating over children
> > from both primary and secondary fwnodes, the code may incorrectly
> > resume iteration from the primary fwnode even when the current child
> > belongs to the secondary, leading to repeated traversal and a loop.
> > 
> > Both issues are triggered when mixing different fwnode types through the
> > secondary mechanism, and stem from incorrect assumptions about ownership
> > and traversal context of child nodes.
> 
> Please, Cc Bart who is heavily working on software nodes these days.

Ah, the Cc list is generated by B4. Will Cc Bart in the future.

Thanks,
Xu Yang

Re: [PATCH v2 2/2] device property: fix infinite loop in fwnode_for_each_child_node()

[Xu Yang]

On Wed, Jun 03, 2026 at 12:51:50PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 03, 2026 at 04:44:32PM +0800, Xu Yang wrote:
> 
> > When iterate over children of a fwnode that has a secondary fwnode,
> > fwnode_get_next_child_node() can enter an infinite loop if the secondary
> > fwnode has more than one child.
> > 
> >                        Parent        Child
> >       (Primary fwnode)   FWa:   {FWa1, FWa2, FWa3}
> >     (Secondary fwnode)   FWb:   {FWb1, FWb2}
> > 
> > In this case:
> > 
> >  ┌─> fwnode_get_next_child_node(FWa, FWa1)
> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa1) returns FWa2
> >  │
> >  │   ...
> >  │
> >  │   fwnode_get_next_child_node(FWa, FWa3)
> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa3) returns NULL
> >  │    - fwnode_call_ptr_op(FWb, get_next_child_node, FWa3) returns FWb1
> >  │
> >  │   fwnode_get_next_child_node(FWa, FWb1)
> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWb1) returns FWa1
> >  └────┘
> > 
> > This cause fwnode_for_each_child_node() to loop indefinitely, reapeatedly
> > output {FWa1, FWa2, FWa3, FWb1, FWa1, ...}.
> > 
> > The root cause is that when the current child (FWb1) belongs to the
> > secondary fwnode, calling get_next_child_node() on the parimary fwnode
> > incorrectly returns the first child (FWa1) again instead of NULL.
> > 
> > Fix this by dynamically checking the parent fwnode of the current child
> > before calling get_next_child_node(). This approach follows the pattern
> > established in commit b5b41ab6b0c1 ("device property: Check
> > fwnode->secondary in fwnode_graph_get_next_endpoint()").
> 
> ...
> 
> TBH, this code becomes twisted and complicated. Can we add some test cases to
> show the problem? Also we need to add other possible combinations (somewhat
> about ~5-6) of the different types of fwnode in a relationship.

I agree that adding test cases would be helpful. But It's not straightforward to
get swnode refcount as swnode is an internal structure. Any suggestions on this?

Thanks,
Xu Yang

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Xu Yang]

On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:
> 
> > When a swnode acts as a secondary fwnode and is participates in child
> > iteration, a refcount leak occurs for the last child of the primary
> > fwnode's children.
> > 
> >                    Parent      Child
> >   (Primary fwnode)   FW:   {FW1, FW2, FW3}
> > (Secondary fwnode)   SW:   {}
> > 
> > In this case, FW3's refcount is decremented twice during iteration:
> > 
> >  fwnode_get_next_child_node(FW, FW3)
> >   1. fwnode_call_ptr_op(FW, get_next_child_node, FW3) returns NULL and
> >      decrements FW3's refcount
> >   2. fwnode_call_ptr_op(SW, get_next_child_node, FW3) returns NULL and
> >      decrements FW3's refcount again
> > 
> > The same double-decrement issue occurs when SW has children.
> > 
> > The kernel dump as below:
> > 
> > [   25.435805] OF: ERROR: of_node_release() detected bad of_node_put() on /soc/usb@4c010010/usb@4c100000
> > [   25.445072] CPU: 0 UID: 0 PID: 617 Comm: sh Not tainted 7.1.0-rc4-next-20260522-00011-g7376b330abca #210 PREEMPT
> > [   25.445080] Hardware name: NXP i.MX95 19X19 board (DT)
> > [   25.445083] Call trace:
> > [   25.445086]  show_stack+0x18/0x30 (C)
> > [   25.445101]  dump_stack_lvl+0x60/0x80
> > [   25.445108]  dump_stack+0x18/0x24
> > [   25.445113]  of_node_release+0x158/0x194
> > [   25.445122]  kobject_put+0xa0/0x120
> > [   25.445129]  of_node_put+0x18/0x28
> > [   25.445134]  of_fwnode_put+0x38/0x58
> > [   25.445141]  software_node_get_next_child+0x54/0x15c
> > [   25.445150]  fwnode_get_next_child_node+0x70/0x94
> > [   25.445156]  fwnode_get_next_available_child_node+0x34/0x88
> > [   25.445162]  device_links_driver_bound+0x2f4/0x334
> > [   25.445168]  driver_bound+0x68/0xb0
> >                 ...
> > [   25.445258] OF: ERROR: next of_node_put() on this node will result in a kobject warning 'refcount_t: underflow; use-after-free.'
> > 
> > Fix this by ensuring software_node_get_next_child() does not decrement
> > the child's refcount when:
> > - The parent has no children, OR
> > - The parent has children but the input child is not a swnode
> > 
> > This prevents the refcount from being incorrectly decremented for
> > fwnodes that don't belong to the software node hierarchy.
> 
> ...
> 
> >  	struct swnode *p = to_swnode(fwnode);
> >  	struct swnode *c = to_swnode(child);
> >  
> > -	if (!p || list_empty(&p->children) ||
> > -	    (c && list_is_last(&c->entry, &p->children))) {
> > -		fwnode_handle_put(child);
> 
> Wouldn't be better to use swnode_get() / swnode_put() instead?
> *Yes, we might need to add some NULL checks there.

It's not newly added by me. The software_node_get_next_child() has been using
fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
be fine since they do the same thing here for a swnode.

Thanks,
Xu Yang

> 
> > +	if (!p || list_empty(&p->children))
> >  		return NULL;
> > -	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[Bartosz Golaszewski]

On Thu, 4 Jun 2026 12:58:41 +0200, Xu Yang <xu.yang_2@oss.nxp.com> said:
> On Wed, Jun 03, 2026 at 12:43:06PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 03, 2026 at 04:44:30PM +0800, Xu Yang wrote:
>> > This series fixes two issues in the fwnode child iteration logic when
>> > a secondary fwnode is present.
>> >
>> > The first patch addresses a refcount imbalance in
>> > software_node_get_next_child(). When a software node is used as a
>> > secondary fwnode, the iteration code may incorrectly decrement the
>> > refcount of child nodes that do not belong to the software node
>> > hierarchy. This results in refcount underflow and possible use-after-free.
>> >
>> > The second patch fixes an infinite loop in
>> > fwnode_for_each_child_node(), caused by improper handling of iteration
>> > state across primary and secondary fwnodes. When iterating over children
>> > from both primary and secondary fwnodes, the code may incorrectly
>> > resume iteration from the primary fwnode even when the current child
>> > belongs to the secondary, leading to repeated traversal and a loop.
>> >
>> > Both issues are triggered when mixing different fwnode types through the
>> > secondary mechanism, and stem from incorrect assumptions about ownership
>> > and traversal context of child nodes.
>>
>> Please, Cc Bart who is heavily working on software nodes these days.
>

Should I propose myself as reviewer? We can't demand people to Cc random
addresses otherwise.

Bart

Re: [PATCH v2 2/2] device property: fix infinite loop in fwnode_for_each_child_node()

[Bartosz Golaszewski]

On Thu, 4 Jun 2026 13:05:23 +0200, Xu Yang <xu.yang_2@oss.nxp.com> said:
> On Wed, Jun 03, 2026 at 12:51:50PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 03, 2026 at 04:44:32PM +0800, Xu Yang wrote:
>>
>> > When iterate over children of a fwnode that has a secondary fwnode,
>> > fwnode_get_next_child_node() can enter an infinite loop if the secondary
>> > fwnode has more than one child.
>> >
>> >                        Parent        Child
>> >       (Primary fwnode)   FWa:   {FWa1, FWa2, FWa3}
>> >     (Secondary fwnode)   FWb:   {FWb1, FWb2}
>> >
>> > In this case:
>> >
>> >  ┌─> fwnode_get_next_child_node(FWa, FWa1)
>> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa1) returns FWa2
>> >  │
>> >  │   ...
>> >  │
>> >  │   fwnode_get_next_child_node(FWa, FWa3)
>> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa3) returns NULL
>> >  │    - fwnode_call_ptr_op(FWb, get_next_child_node, FWa3) returns FWb1
>> >  │
>> >  │   fwnode_get_next_child_node(FWa, FWb1)
>> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWb1) returns FWa1
>> >  └────┘
>> >
>> > This cause fwnode_for_each_child_node() to loop indefinitely, reapeatedly
>> > output {FWa1, FWa2, FWa3, FWb1, FWa1, ...}.
>> >
>> > The root cause is that when the current child (FWb1) belongs to the
>> > secondary fwnode, calling get_next_child_node() on the parimary fwnode
>> > incorrectly returns the first child (FWa1) again instead of NULL.
>> >
>> > Fix this by dynamically checking the parent fwnode of the current child
>> > before calling get_next_child_node(). This approach follows the pattern
>> > established in commit b5b41ab6b0c1 ("device property: Check
>> > fwnode->secondary in fwnode_graph_get_next_endpoint()").
>>
>> ...
>>
>> TBH, this code becomes twisted and complicated. Can we add some test cases to
>> show the problem? Also we need to add other possible combinations (somewhat
>> about ~5-6) of the different types of fwnode in a relationship.
>
> I agree that adding test cases would be helpful. But It's not straightforward to
> get swnode refcount as swnode is an internal structure. Any suggestions on this?
>

You should be able to replicate the problem with the firmware node API without
accessing the internal swnode structure. You can use dummy OF nodes as the
primary fwnodes.

Bart

Re: [PATCH v2 2/2] device property: fix infinite loop in fwnode_for_each_child_node()

[Xu Yang]

On Thu, Jun 04, 2026 at 06:43:38AM -0700, Bartosz Golaszewski wrote:
> On Thu, 4 Jun 2026 13:05:23 +0200, Xu Yang <xu.yang_2@oss.nxp.com> said:
> > On Wed, Jun 03, 2026 at 12:51:50PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jun 03, 2026 at 04:44:32PM +0800, Xu Yang wrote:
> >>
> >> > When iterate over children of a fwnode that has a secondary fwnode,
> >> > fwnode_get_next_child_node() can enter an infinite loop if the secondary
> >> > fwnode has more than one child.
> >> >
> >> >                        Parent        Child
> >> >       (Primary fwnode)   FWa:   {FWa1, FWa2, FWa3}
> >> >     (Secondary fwnode)   FWb:   {FWb1, FWb2}
> >> >
> >> > In this case:
> >> >
> >> >  ┌─> fwnode_get_next_child_node(FWa, FWa1)
> >> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa1) returns FWa2
> >> >  │
> >> >  │   ...
> >> >  │
> >> >  │   fwnode_get_next_child_node(FWa, FWa3)
> >> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWa3) returns NULL
> >> >  │    - fwnode_call_ptr_op(FWb, get_next_child_node, FWa3) returns FWb1
> >> >  │
> >> >  │   fwnode_get_next_child_node(FWa, FWb1)
> >> >  │    - fwnode_call_ptr_op(FWa, get_next_child_node, FWb1) returns FWa1
> >> >  └────┘
> >> >
> >> > This cause fwnode_for_each_child_node() to loop indefinitely, reapeatedly
> >> > output {FWa1, FWa2, FWa3, FWb1, FWa1, ...}.
> >> >
> >> > The root cause is that when the current child (FWb1) belongs to the
> >> > secondary fwnode, calling get_next_child_node() on the parimary fwnode
> >> > incorrectly returns the first child (FWa1) again instead of NULL.
> >> >
> >> > Fix this by dynamically checking the parent fwnode of the current child
> >> > before calling get_next_child_node(). This approach follows the pattern
> >> > established in commit b5b41ab6b0c1 ("device property: Check
> >> > fwnode->secondary in fwnode_graph_get_next_endpoint()").
> >>
> >> ...
> >>
> >> TBH, this code becomes twisted and complicated. Can we add some test cases to
> >> show the problem? Also we need to add other possible combinations (somewhat
> >> about ~5-6) of the different types of fwnode in a relationship.
> >
> > I agree that adding test cases would be helpful. But It's not straightforward to
> > get swnode refcount as swnode is an internal structure. Any suggestions on this?
> >
> 
> You should be able to replicate the problem with the firmware node API without
> accessing the internal swnode structure. You can use dummy OF nodes as the
> primary fwnodes.

Got it. I thought it's the refcount leak one. Then no needs to get refcount.

Thanks,
Xu Yang

> 
> Bart

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Andy Shevchenko]

On Thu, Jun 04, 2026 at 07:15:26PM +0800, Xu Yang wrote:
> On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:

...

> > >  	struct swnode *p = to_swnode(fwnode);
> > >  	struct swnode *c = to_swnode(child);
> > >  
> > > -	if (!p || list_empty(&p->children) ||
> > > -	    (c && list_is_last(&c->entry, &p->children))) {
> > > -		fwnode_handle_put(child);
> > 
> > Wouldn't be better to use swnode_get() / swnode_put() instead?
> > *Yes, we might need to add some NULL checks there.
> 
> It's not newly added by me. The software_node_get_next_child() has been using
> fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
> be fine since they do the same thing here for a swnode.

It doesn't matter who added that. But according to the point of this patch
(correct me if I am wrong) is to avoid bumping or dropping reference count for
the nodes that are *not* of swnode type. Moving away from fwnode_handle_*()
loop we make the point clear.

See the of_get_next_status_child() implementation, it does *not* use
fwnode_handle_*() at all. So, making it here to use same approach should
fix your issue, no?

> > > +	if (!p || list_empty(&p->children))
> > >  		return NULL;
> > > -	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] device property: fix child iteration issues with secondary fwnodes

[Andy Shevchenko]

On Thu, Jun 04, 2026 at 06:38:50AM -0700, Bartosz Golaszewski wrote:
> On Thu, 4 Jun 2026 12:58:41 +0200, Xu Yang <xu.yang_2@oss.nxp.com> said:
> > On Wed, Jun 03, 2026 at 12:43:06PM +0300, Andy Shevchenko wrote:
> >> On Wed, Jun 03, 2026 at 04:44:30PM +0800, Xu Yang wrote:
> >> > This series fixes two issues in the fwnode child iteration logic when
> >> > a secondary fwnode is present.
> >> >
> >> > The first patch addresses a refcount imbalance in
> >> > software_node_get_next_child(). When a software node is used as a
> >> > secondary fwnode, the iteration code may incorrectly decrement the
> >> > refcount of child nodes that do not belong to the software node
> >> > hierarchy. This results in refcount underflow and possible use-after-free.
> >> >
> >> > The second patch fixes an infinite loop in
> >> > fwnode_for_each_child_node(), caused by improper handling of iteration
> >> > state across primary and secondary fwnodes. When iterating over children
> >> > from both primary and secondary fwnodes, the code may incorrectly
> >> > resume iteration from the primary fwnode even when the current child
> >> > belongs to the secondary, leading to repeated traversal and a loop.
> >> >
> >> > Both issues are triggered when mixing different fwnode types through the
> >> > secondary mechanism, and stem from incorrect assumptions about ownership
> >> > and traversal context of child nodes.
> >>
> >> Please, Cc Bart who is heavily working on software nodes these days.
> 
> Should I propose myself as reviewer? We can't demand people to Cc random
> addresses otherwise.

If you are interested, I welcome this decision, although I don't know what
maintainers and other peers (current reviewers) think of it. Send a patch
and prepare for any type of responses :-) Mine will be positive for sure.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Xu Yang]

On Thu, Jun 04, 2026 at 08:50:16PM +0300, Andy Shevchenko wrote:
> On Thu, Jun 04, 2026 at 07:15:26PM +0800, Xu Yang wrote:
> > On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:
> 
> ...
> 
> > > >  	struct swnode *p = to_swnode(fwnode);
> > > >  	struct swnode *c = to_swnode(child);
> > > >  
> > > > -	if (!p || list_empty(&p->children) ||
> > > > -	    (c && list_is_last(&c->entry, &p->children))) {
> > > > -		fwnode_handle_put(child);
> > > 
> > > Wouldn't be better to use swnode_get() / swnode_put() instead?
> > > *Yes, we might need to add some NULL checks there.
> > 
> > It's not newly added by me. The software_node_get_next_child() has been using
> > fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
> > be fine since they do the same thing here for a swnode.
> 
> It doesn't matter who added that. But according to the point of this patch
> (correct me if I am wrong) is to avoid bumping or dropping reference count for
> the nodes that are *not* of swnode type. Moving away from fwnode_handle_*()
> loop we make the point clear.

Yes.

> 
> See the of_get_next_status_child() implementation, it does *not* use
> fwnode_handle_*() at all. So, making it here to use same approach should
> fix your issue, no?

You are right. I had also noticed this before. Actually, the difference between
OF node and swnode is that OF node uses to_of_node() to filter out non-OF type
fwnodes. Similarly, swnode uses to_swnode() to filter out non-swnode type fwnodes.
So replace fwnode_handle_get() / fwnode_handle_put() with software_node_get() /
software_node_put() does fix the issue.

When I reviewed patch #1 again, I found it already fixes the refcount leak issue
because when it switches to the secondary fwnode, it no longer passes the primary
child to secondary fwnode. So the patch #1 is not needed anymore. I will remove
it in v3.

Thanks for your review!

Thanks,
Xu Yang

> 
> > > > +	if (!p || list_empty(&p->children))
> > > >  		return NULL;
> > > > -	}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Andy Shevchenko]

On Fri, Jun 05, 2026 at 05:16:32PM +0800, Xu Yang wrote:
> On Thu, Jun 04, 2026 at 08:50:16PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 04, 2026 at 07:15:26PM +0800, Xu Yang wrote:
> > > On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:

...

> > > > >  	struct swnode *p = to_swnode(fwnode);
> > > > >  	struct swnode *c = to_swnode(child);
> > > > >  
> > > > > -	if (!p || list_empty(&p->children) ||
> > > > > -	    (c && list_is_last(&c->entry, &p->children))) {
> > > > > -		fwnode_handle_put(child);
> > > > 
> > > > Wouldn't be better to use swnode_get() / swnode_put() instead?
> > > > *Yes, we might need to add some NULL checks there.
> > > 
> > > It's not newly added by me. The software_node_get_next_child() has been using
> > > fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
> > > be fine since they do the same thing here for a swnode.
> > 
> > It doesn't matter who added that. But according to the point of this patch
> > (correct me if I am wrong) is to avoid bumping or dropping reference count for
> > the nodes that are *not* of swnode type. Moving away from fwnode_handle_*()
> > loop we make the point clear.
> 
> Yes.
> 
> > See the of_get_next_status_child() implementation, it does *not* use
> > fwnode_handle_*() at all. So, making it here to use same approach should
> > fix your issue, no?
> 
> You are right. I had also noticed this before. Actually, the difference between
> OF node and swnode is that OF node uses to_of_node() to filter out non-OF type
> fwnodes. Similarly, swnode uses to_swnode() to filter out non-swnode type fwnodes.
> So replace fwnode_handle_get() / fwnode_handle_put() with software_node_get() /
> software_node_put() does fix the issue.
> 
> When I reviewed patch #1 again, I found it already fixes the refcount leak issue
> because when it switches to the secondary fwnode, it no longer passes the primary
> child to secondary fwnode. So the patch #1 is not needed anymore. I will remove
> it in v3.

I'm lost in here. My expectation that patch 1 should fix the issue as it won't
let the fwnode_handle_*() be called against wrong type of fwnode. What did I
miss?

> > > > > +	if (!p || list_empty(&p->children))
> > > > >  		return NULL;
> > > > > -	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] software node: fix refcount leak in software_node_get_next_child()

[Xu Yang]

On Fri, Jun 05, 2026 at 06:20:52PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 05, 2026 at 05:16:32PM +0800, Xu Yang wrote:
> > On Thu, Jun 04, 2026 at 08:50:16PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jun 04, 2026 at 07:15:26PM +0800, Xu Yang wrote:
> > > > On Wed, Jun 03, 2026 at 12:40:27PM +0300, Andy Shevchenko wrote:
> > > > > On Wed, Jun 03, 2026 at 04:44:31PM +0800, Xu Yang wrote:
> 
> ...
> 
> > > > > >  	struct swnode *p = to_swnode(fwnode);
> > > > > >  	struct swnode *c = to_swnode(child);
> > > > > >  
> > > > > > -	if (!p || list_empty(&p->children) ||
> > > > > > -	    (c && list_is_last(&c->entry, &p->children))) {
> > > > > > -		fwnode_handle_put(child);
> > > > > 
> > > > > Wouldn't be better to use swnode_get() / swnode_put() instead?
> > > > > *Yes, we might need to add some NULL checks there.
> > > > 
> > > > It's not newly added by me. The software_node_get_next_child() has been using
> > > > fwnode_handle_get() / fwnode_handle_put() before. In my opinion, this should
> > > > be fine since they do the same thing here for a swnode.
> > > 
> > > It doesn't matter who added that. But according to the point of this patch
> > > (correct me if I am wrong) is to avoid bumping or dropping reference count for
> > > the nodes that are *not* of swnode type. Moving away from fwnode_handle_*()
> > > loop we make the point clear.
> > 
> > Yes.
> > 
> > > See the of_get_next_status_child() implementation, it does *not* use
> > > fwnode_handle_*() at all. So, making it here to use same approach should
> > > fix your issue, no?
> > 
> > You are right. I had also noticed this before. Actually, the difference between
> > OF node and swnode is that OF node uses to_of_node() to filter out non-OF type
> > fwnodes. Similarly, swnode uses to_swnode() to filter out non-swnode type fwnodes.
> > So replace fwnode_handle_get() / fwnode_handle_put() with software_node_get() /
> > software_node_put() does fix the issue.
> > 
> > When I reviewed patch #1 again, I found it already fixes the refcount leak issue
> > because when it switches to the secondary fwnode, it no longer passes the primary
> > child to secondary fwnode. So the patch #1 is not needed anymore. I will remove
> > it in v3.
> 
> I'm lost in here. My expectation that patch 1 should fix the issue as it won't
> let the fwnode_handle_*() be called against wrong type of fwnode. What did I
> miss?

Sorry, I meant "When I reviewed patch #2 again, ..."

Let me clarify the issues here, patch #1 fixes refcount leak issue and patch #2 fixes
the infinite loop issue. Although replacing fwnode_handle_*() with software_node_*()
in patch #1 can fix refcount issue in another way, it can not fix the infinite loop
issue. So patch #2 is still required. When patch #2 changes:

return fwnode_call_ptr_op(fwnode->secondary, get_next_child_node, child);
                                                                   |
to                                                                 |
                                                                   v
return fwnode_call_ptr_op(parent->secondary, get_next_child_node, NULL);

the secondary fwnode will no longer deal with primary fwnode's child. So patch #2 has
already fixed the refcount leak issue. Therefore, patch #1 can be removed.

Thanks,
Xu Yang