[PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify()

[PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify()

[Haren Myneni]

of_changeset_apply() interface adds nodes for action
OF_RECONFIG_ATTACH_NODE and calls notifiers for each attached node.
Then the caller uses of_detach_node() to release each node and
also calls the corresponding notifier function. In the case of
failure from the add notifier for any node, of_changeset_apply()
returns failure after adding nodes. The current implementation
does not the provide any interface to the caller to remove nodes
from the DT without calling notifiers. It may end up having nodes
in DT even though ADD is not successful.

This patch introduces of_detach_node_no_notify() interface for the
caller to only remove node if it is not attached.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
 drivers/of/dynamic.c | 16 ++++++++++++++++
 include/linux/of.h   |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 744ce0e1eb24..fc6f22371a97 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -318,6 +318,22 @@ int of_detach_node(struct device_node *np)
 }
 EXPORT_SYMBOL_GPL(of_detach_node);
 
+/**
+ * of_detach_node_no_notify() - "Unplug" a node from the device
+ * and return without running notifiers.
+ * @np:	Pointer to the caller's Device Node
+ */
+int of_detach_node_no_notify(struct device_node *np)
+{
+	mutex_lock(&of_mutex);
+	if (!of_node_check_flag(np, OF_DETACHED))
+		__of_detach_node(np);
+	mutex_unlock(&of_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_detach_node_no_notify);
+
 void __of_prop_free(struct property *prop)
 {
 	kfree(prop->name);
diff --git a/include/linux/of.h b/include/linux/of.h
index b920aac6b975..7a509341e998 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -442,6 +442,7 @@ extern int of_update_property(struct device_node *np, struct property *newprop);
 
 extern int of_attach_node(struct device_node *);
 extern int of_detach_node(struct device_node *);
+extern int of_detach_node_no_notify(struct device_node *);
 
 #define of_match_ptr(_ptr)	(_ptr)
 
-- 
2.55.0

[PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier

[Haren Myneni]

For CPU ADD, the device tree entries are retrieved with
configure-connector RTAS call and attached to the device tree.
Then the CPU is added as part of DT node notification. If the CPU
ADD notifier returns failure, the corresponding CPU node entries
should be deleted from the device-tree. See pseries_add_processor()
for the possible failure cases.

The current code does not remove CPU node entries during CPU ADD
notifier failure and causing the following issues:
- Can not add this CPU later since already present in the
  device-tree.
- Trying to free memory allocated to CPU node without detaching
  the node and it causes freeing its sibling node (existing CPU
  nodes).

This patch fixes this issue by calling of_detach_node_no_notify()
for the failure from CPU ADD notifier which removes CPU node
entries without calling CPU REMOVE notifier.

Signed-off-by: Haren Myneni <haren@linux.ibm.com>
---
v2:
- Check dn before detach node as pointed by Sashiko AI review.

 arch/powerpc/platforms/pseries/dlpar.c       | 9 ++++++---
 arch/powerpc/platforms/pseries/hotplug-cpu.c | 6 +++---
 arch/powerpc/platforms/pseries/mobility.c    | 2 +-
 arch/powerpc/platforms/pseries/pmem.c        | 2 +-
 arch/powerpc/platforms/pseries/pseries.h     | 2 +-
 5 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index f4d33b8dffd8..01a9849773cd 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -247,15 +247,18 @@ int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
 	return 0;
 }
 
-int dlpar_detach_node(struct device_node *dn)
+int dlpar_detach_node(struct device_node *dn, bool notify)
 {
 	struct device_node *child;
 	int rc;
 
 	for_each_child_of_node(dn, child)
-		dlpar_detach_node(child);
+		dlpar_detach_node(child, notify);
 
-	rc = of_detach_node(dn);
+	if (notify)
+		rc = of_detach_node(dn);
+	else
+		rc = of_detach_node_no_notify(dn);
 	if (rc)
 		return rc;
 
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index bc6926dbf148..15122aab05b2 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -602,8 +602,8 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
 			dn, rc, drc_index);
 
 		rc = dlpar_release_drc(drc_index);
-		if (!rc)
-			dlpar_free_cc_nodes(dn);
+		if (!rc && dn)
+			dlpar_detach_node(dn, false);
 
 		return saved_rc;
 	}
@@ -616,7 +616,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
 		pr_warn("Failed to online cpu %pOFn, rc: %d, drc index: %x\n",
 			dn, rc, drc_index);
 
-		rc = dlpar_detach_node(dn);
+		rc = dlpar_detach_node(dn, true);
 		if (!rc)
 			dlpar_release_drc(drc_index);
 
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index b5c2abd12432..e0e4ef04c679 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -112,7 +112,7 @@ static int delete_dt_node(struct device_node *dn)
 	}
 
 	pr_debug("removing node %pOFfp\n", dn);
-	dlpar_detach_node(dn);
+	dlpar_detach_node(dn, true);
 	return 0;
 }
 
diff --git a/arch/powerpc/platforms/pseries/pmem.c b/arch/powerpc/platforms/pseries/pmem.c
index 0f1d45f32e4a..b60a01745137 100644
--- a/arch/powerpc/platforms/pseries/pmem.c
+++ b/arch/powerpc/platforms/pseries/pmem.c
@@ -85,7 +85,7 @@ static ssize_t pmem_drc_remove_node(u32 drc_index)
 	pr_debug("Attempting to remove %pOF, drc index: %x\n", dn, drc_index);
 
 	/* * NB: tears down the ibm,pmemory device as a side-effect */
-	rc = dlpar_detach_node(dn);
+	rc = dlpar_detach_node(dn, true);
 	if (rc)
 		return rc;
 
diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
index 3968a6970fa8..6f80e58e6186 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -50,7 +50,7 @@ extern void dlpar_free_cc_property(struct property *);
 extern struct device_node *dlpar_configure_connector(__be32,
 						struct device_node *);
 extern int dlpar_attach_node(struct device_node *, struct device_node *);
-extern int dlpar_detach_node(struct device_node *);
+extern int dlpar_detach_node(struct device_node *, bool);
 extern int dlpar_acquire_drc(u32 drc_index);
 extern int dlpar_release_drc(u32 drc_index);
 extern int dlpar_unisolate_drc(u32 drc_index);
-- 
2.55.0

Re: [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] of: resource leak when using of_detach_node_no_notify() for rollback
--

commit 32bc441a65c0469af8bda8ae3a3847daa98db314
Author: Haren Myneni <haren@linux.ibm.com>

drivers/of: Add of_detach_node_no_notify()

This commit introduces the of_detach_node_no_notify() interface to allow
callers to remove a device tree node without triggering the corresponding
detach notifiers. This is intended to handle rollback scenarios where
adding nodes in a changeset fails after partially attaching them.

> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> index 744ce0e1eb24a..fc6f22371a97c 100644
> --- a/drivers/of/dynamic.c
> +++ b/drivers/of/dynamic.c

[ ... ]

> @@ -318,6 +318,22 @@ int of_detach_node(struct device_node *np)
>  }
>  EXPORT_SYMBOL_GPL(of_detach_node);
>  
> +/**
> + * of_detach_node_no_notify() - "Unplug" a node from the device
> + * and return without running notifiers.
> + * @np:	Pointer to the caller's Device Node
> + */
> +int of_detach_node_no_notify(struct device_node *np)
> +{
> +	mutex_lock(&of_mutex);
> +	if (!of_node_check_flag(np, OF_DETACHED))
> +		__of_detach_node(np);
> +	mutex_unlock(&of_mutex);
> +
> +	return 0;
> +}

[Severity: Medium]
Does bypassing the OF_RECONFIG_DETACH_NODE notifier during error rollback
cause resource leaks?

If a caller attempts to dynamically add a device tree node and the
OF_RECONFIG_ATTACH_NODE notifier chain fails midway, calling this new API
to roll back the attachment will skip the detach notifiers.

This fundamentally breaks subsystem teardown symmetry, which means any
previously successful attach notifiers in the chain might permanently leak
their allocated resources, such as platform_device instances.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921033156.991481-1-haren@linux.ibm.com?part=1

Re: [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] powerpc/pseries: NULL pointer deref in __of_detach_node on error path
- [Medium] powerpc/pseries: memory leak of unattached sibling device nodes
--

commit 79a5af10ba412eed58ff43cc8d6339b8b6eee45f
Author: Haren Myneni <haren@linux.ibm.com>

powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier

For CPU ADD, the device tree entries are retrieved with
configure-connector RTAS call and attached to the device tree.
Then the CPU is added as part of DT node notification. If the CPU
ADD notifier returns failure, the corresponding CPU node entries
should be deleted from the device-tree.

> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index f4d33b8dffd85..01a9849773cd1 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -247,15 +247,18 @@ int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
>  	return 0;
>  }
>  
> -int dlpar_detach_node(struct device_node *dn)
> +int dlpar_detach_node(struct device_node *dn, bool notify)
>  {
>  	struct device_node *child;
>  	int rc;
>  
>  	for_each_child_of_node(dn, child)
> -		dlpar_detach_node(child);
> +		dlpar_detach_node(child, notify);

[Severity: Medium]
Will this leak unattached sibling device nodes on the error path?

dlpar_configure_connector() can return a chain of device nodes linked via
dn->sibling. The error path in dlpar_cpu_add() previously called
dlpar_free_cc_nodes(dn), which handles sibling traversal.

By replacing it with dlpar_detach_node(dn, false), which recursively
processes children but explicitly does not traverse dn->sibling, do we
permanently leak any unattached sibling nodes returned by the connector?

>  
> -	rc = of_detach_node(dn);
> +	if (notify)
> +		rc = of_detach_node(dn);
> +	else
> +		rc = of_detach_node_no_notify(dn);

[ ... ]

> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index bc6926dbf1489..15122aab05b24 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -602,8 +602,8 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
>  			dn, rc, drc_index);
>  
>  		rc = dlpar_release_drc(drc_index);
> -		if (!rc)
> -			dlpar_free_cc_nodes(dn);
> +		if (!rc && dn)
> +			dlpar_detach_node(dn, false);

[Severity: High]
Does this code trigger a NULL pointer dereference when
pseries_cpuhp_attach_nodes() fails?

In dlpar_cpu_add(), if pseries_cpuhp_attach_nodes() fails, the device node
dn remains unattached from the device tree. This error path then calls
dlpar_detach_node(dn, false), which invokes __of_detach_node().

Because dlpar_parse_cc_node() does not set the OF_DETACHED flag on
allocation, __of_detach_node() assumes the node is attached and iterates
over its parent's children to unlink it:

drivers/of/dynamic.c:__of_detach_node() {
	...
		for (prevsib = np->parent->child;
		     prevsib->sibling != np;
		     prevsib = prevsib->sibling)
			;
		prevsib->sibling = np->sibling;
	...
}

Since the node was never linked, it is not in the list, and the traversal
pointer prevsib reaches NULL.  Will this cause a guaranteed NULL pointer
dereference (prevsib->sibling) and kernel panic?

>  
>  		return saved_rc;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921033156.991481-1-haren@linux.ibm.com?part=2

Re: [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify()

[Haren Myneni]

On Mon, 2026-09-21 at 03:44 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> - [Medium] of: resource leak when using of_detach_node_no_notify()
> for rollback
> --
> 
> commit 32bc441a65c0469af8bda8ae3a3847daa98db314
> Author: Haren Myneni <haren@linux.ibm.com>
> 
> drivers/of: Add of_detach_node_no_notify()
> 
> This commit introduces the of_detach_node_no_notify() interface to
> allow
> callers to remove a device tree node without triggering the
> corresponding
> detach notifiers. This is intended to handle rollback scenarios where
> adding nodes in a changeset fails after partially attaching them.
> 
> > diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
> > index 744ce0e1eb24a..fc6f22371a97c 100644
> > --- a/drivers/of/dynamic.c
> > +++ b/drivers/of/dynamic.c
> 
> [ ... ]
> 
> > @@ -318,6 +318,22 @@ int of_detach_node(struct device_node *np)
> >  }
> >  EXPORT_SYMBOL_GPL(of_detach_node);
> >  
> > +/**
> > + * of_detach_node_no_notify() - "Unplug" a node from the device
> > + * and return without running notifiers.
> > + * @np:	Pointer to the caller's Device Node
> > + */
> > +int of_detach_node_no_notify(struct device_node *np)
> > +{
> > +	mutex_lock(&of_mutex);
> > +	if (!of_node_check_flag(np, OF_DETACHED))
> > +		__of_detach_node(np);
> > +	mutex_unlock(&of_mutex);
> > +
> > +	return 0;
> > +}
> 
> [Severity: Medium]
> Does bypassing the OF_RECONFIG_DETACH_NODE notifier during error
> rollback
> cause resource leaks?
> 
> If a caller attempts to dynamically add a device tree node and the
> OF_RECONFIG_ATTACH_NODE notifier chain fails midway, calling this new
> API
> to roll back the attachment will skip the detach notifiers.
> 
> This fundamentally breaks subsystem teardown symmetry, which means
> any
> previously successful attach notifiers in the chain might permanently
> leak
> their allocated resources, such as platform_device instances.

of_detach_node_no_notify() is called per node and only if the attached
node notifier is failed. So up to the caller whether to use this or
of_detach_node().

Re: [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier

[Haren Myneni]

On Mon, 2026-09-21 at 03:47 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] powerpc/pseries: NULL pointer deref in __of_detach_node on
> error path
> - [Medium] powerpc/pseries: memory leak of unattached sibling device
> nodes
> --
> 
> commit 79a5af10ba412eed58ff43cc8d6339b8b6eee45f
> Author: Haren Myneni <haren@linux.ibm.com>
> 
> powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD
> notifier
> 
> For CPU ADD, the device tree entries are retrieved with
> configure-connector RTAS call and attached to the device tree.
> Then the CPU is added as part of DT node notification. If the CPU
> ADD notifier returns failure, the corresponding CPU node entries
> should be deleted from the device-tree.
> 
> > diff --git a/arch/powerpc/platforms/pseries/dlpar.c
> > b/arch/powerpc/platforms/pseries/dlpar.c
> > index f4d33b8dffd85..01a9849773cd1 100644
> > --- a/arch/powerpc/platforms/pseries/dlpar.c
> > +++ b/arch/powerpc/platforms/pseries/dlpar.c
> > @@ -247,15 +247,18 @@ int dlpar_attach_node(struct device_node *dn,
> > struct device_node *parent)
> >  	return 0;
> >  }
> >  
> > -int dlpar_detach_node(struct device_node *dn)
> > +int dlpar_detach_node(struct device_node *dn, bool notify)
> >  {
> >  	struct device_node *child;
> >  	int rc;
> >  
> >  	for_each_child_of_node(dn, child)
> > -		dlpar_detach_node(child);
> > +		dlpar_detach_node(child, notify);
> 
> [Severity: Medium]
> Will this leak unattached sibling device nodes on the error path?
> 
> dlpar_configure_connector() can return a chain of device nodes linked
> via
> dn->sibling. The error path in dlpar_cpu_add() previously called
> dlpar_free_cc_nodes(dn), which handles sibling traversal.
> 
> By replacing it with dlpar_detach_node(dn, false), which recursively
> processes children but explicitly does not traverse dn->sibling, do
> we
> permanently leak any unattached sibling nodes returned by the
> connector?

No leak here. dlpar_configure_connector() can also return sibling
nodes, but this interface should detach node and its children. Same use
case as before dlpar_detach_node(). We have bug in the previous use of
dlpar_free_cc_nodes() in dlpar_cpu_add(). Example: For CPU node, No
sibling node is returned from dlpar_configure_connector() and the other
existing CPU node is attached as sibling to the new CPU node
(of_attach_node()). 

> 
> >  
> > -	rc = of_detach_node(dn);
> > +	if (notify)
> > +		rc = of_detach_node(dn);
> > +	else
> > +		rc = of_detach_node_no_notify(dn);
> 
> [ ... ]
> 
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > index bc6926dbf1489..15122aab05b24 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > @@ -602,8 +602,8 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
> >  			dn, rc, drc_index);
> >  
> >  		rc = dlpar_release_drc(drc_index);
> > -		if (!rc)
> > -			dlpar_free_cc_nodes(dn);
> > +		if (!rc && dn)
> > +			dlpar_detach_node(dn, false);
> 
> [Severity: High]
> Does this code trigger a NULL pointer dereference when
> pseries_cpuhp_attach_nodes() fails?
> 
> In dlpar_cpu_add(), if pseries_cpuhp_attach_nodes() fails, the device
> node
> dn remains unattached from the device tree. This error path then
> calls
> dlpar_detach_node(dn, false), which invokes __of_detach_node().
> 
> Because dlpar_parse_cc_node() does not set the OF_DETACHED flag on
> allocation, __of_detach_node() assumes the node is attached and
> iterates
> over its parent's children to unlink it:
> 
> drivers/of/dynamic.c:__of_detach_node() {
> 	...
> 		for (prevsib = np->parent->child;
> 		     prevsib->sibling != np;
> 		     prevsib = prevsib->sibling)
> 			;
> 		prevsib->sibling = np->sibling;
> 	...
> }
> 
> Since the node was never linked, it is not in the list, and the
> traversal
> pointer prevsib reaches NULL.  Will this cause a guaranteed NULL
> pointer
> dereference (prevsib->sibling) and kernel panic?

No, pseries_cpuhp_attach_nodes() returns failure in 3 cases:
- dn allocation is failed from of_changeset_attach_node(). 'if (!rc &&
dn)' prevents the NULL pointer access.
- if dn is not attached, 'if (!of_node_check_flag(np, OF_DETACHED))'
check prevents 
- if the dn notifier is failed, fixing this issue with the new
interface. of_detach_node_no_notify()

> 
> >  
> >  		return saved_rc;
> >  	}