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

[sashiko-bot@kernel.org]

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?

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

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

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