Re: [PATCH v2 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] 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