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

[Haren Myneni <haren@linux.ibm.com>]

On Wed, 2026-09-16 at 04:47 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] of: kernel panic in __of_detach_node() due to unattached
> node
> - [High] powerpc/pseries: sibling device nodes leaked on hotplug
> error path
> --
> 
> commit 4b977223f26c877c63148563cad8a84f1583e398
> 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, this patch calls
> of_detach_node_no_notify()
> instead of dlpar_free_cc_nodes() to prevent device tree issues.
> 
> > 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: High]
> Does this code leak sibling device nodes?
> 
> In the error handling path of dlpar_cpu_add(), when
> pseries_cpuhp_attach_nodes() fails, dlpar_detach_node(dn, false) is
> called
> to clean up. However, the old dlpar_free_cc_nodes() used to
> recursively
> free dn->sibling. This new implementation only iterates through
> children and
> completely ignores dn->sibling.
> 
> If siblings were returned by configure-connector but never
> successfully
> attached, or if they were attached but later notification fails, are
> those
> sibling nodes permanently leaked?

of_attach_node() sets the one of current CPU nodes as sibling to the
new CPU node.
Since the sibling nodes are the existing CPU nodes, should not be
freed.
The current code (dlpar_free_cc_nodes(dn)) removes sibling nodes which
is a bug and causes an issue when trying to add new CPU in future.. 

> 
> >  
> > -	rc = of_detach_node(dn);
> > +	if (notify)
> > +		rc = of_detach_node(dn);
> > +	else
> > +		rc = of_detach_node_no_notify(dn);
> 
> [Severity: High]
> Can this cause a NULL pointer dereference leading to a kernel panic?
> 
> If of_changeset_attach_node() fails in pseries_cpuhp_attach_nodes()
> (for
> instance, due to an allocation failure), the dn node is never
> attached to
> the device tree's sibling list, and its OF_DETACHED flag remains 0.
> 
> If dlpar_cpu_add() then attempts to clean up by calling
> dlpar_detach_node(dn, false), this code forwards the unattached node
> to
> of_detach_node_no_notify(dn). Since OF_DETACHED is 0, it calls
> __of_detach_node(np).
> 
> Inside __of_detach_node(), the node is not found in parent->child,
> causing
> the prevsib pointer to traverse past the end of the list to NULL,
> leading to
> a crash at prevsib->sibling = np->sibling.

will change:

if (rc && dn)  /* in case of memory allocation failure */
	dlpar_detach_node(dn, false);


> 
> >  	if (rc)
> >  		return rc;
> >  
> > diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > index bc6926dbf1489..db359fa3f9224 100644
> > --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> > @@ -603,7 +603,7 @@ static ssize_t dlpar_cpu_add(u32 drc_index)
> >  
> >  		rc = dlpar_release_drc(drc_index);
> >  		if (!rc)
> > -			dlpar_free_cc_nodes(dn);
> > +			dlpar_detach_node(dn, false);
> >  
> >  		return saved_rc;
> >  	}