* [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() @ 2026-09-21 3:31 Haren Myneni 2026-09-21 3:31 ` [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier Haren Myneni 2026-09-21 3:44 ` [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() sashiko-bot 0 siblings, 2 replies; 6+ messages in thread From: Haren Myneni @ 2026-09-21 3:31 UTC (permalink / raw) To: linuxppc-dev Cc: devicetree, maddy, robh, mpe, npiggin, saravanak, ritesh.list, tyreld, hbabu, haren 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier 2026-09-21 3:31 [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() Haren Myneni @ 2026-09-21 3:31 ` Haren Myneni 2026-09-21 3:47 ` sashiko-bot 2026-09-21 3:44 ` [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() sashiko-bot 1 sibling, 1 reply; 6+ messages in thread From: Haren Myneni @ 2026-09-21 3:31 UTC (permalink / raw) To: linuxppc-dev Cc: devicetree, maddy, robh, mpe, npiggin, saravanak, ritesh.list, tyreld, hbabu, haren 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier 2026-09-21 3:31 ` [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier Haren Myneni @ 2026-09-21 3:47 ` sashiko-bot 2026-09-22 23:32 ` Haren Myneni 0 siblings, 1 reply; 6+ messages in thread From: sashiko-bot @ 2026-09-21 3:47 UTC (permalink / raw) To: Haren Myneni; +Cc: robh, conor+dt, devicetree 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier 2026-09-21 3:47 ` sashiko-bot @ 2026-09-22 23:32 ` Haren Myneni 0 siblings, 0 replies; 6+ messages in thread From: Haren Myneni @ 2026-09-22 23:32 UTC (permalink / raw) To: sashiko-reviews; +Cc: robh, conor+dt, devicetree 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; > > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() 2026-09-21 3:31 [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() Haren Myneni 2026-09-21 3:31 ` [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier Haren Myneni @ 2026-09-21 3:44 ` sashiko-bot 2026-09-22 18:02 ` Haren Myneni 1 sibling, 1 reply; 6+ messages in thread From: sashiko-bot @ 2026-09-21 3:44 UTC (permalink / raw) To: Haren Myneni; +Cc: robh, conor+dt, devicetree 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() 2026-09-21 3:44 ` [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() sashiko-bot @ 2026-09-22 18:02 ` Haren Myneni 0 siblings, 0 replies; 6+ messages in thread From: Haren Myneni @ 2026-09-22 18:02 UTC (permalink / raw) To: sashiko-reviews; +Cc: robh, conor+dt, devicetree 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(). ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-22 23:32 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-21 3:31 [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() Haren Myneni 2026-09-21 3:31 ` [PATCH v2 2/2] powerpc/pseries/dlpar: Remove DT entries if failure from CPU ADD notifier Haren Myneni 2026-09-21 3:47 ` sashiko-bot 2026-09-22 23:32 ` Haren Myneni 2026-09-21 3:44 ` [PATCH v2 1/2] drivers/of: Add of_detach_node_no_notify() sashiko-bot 2026-09-22 18:02 ` Haren Myneni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).