Re: [PATCH] powerpc/crash: Add only the onlined CPU's node to the crash FDT

[Vishal Chourasia <vishalc@linux.ibm.com>]

On 04/11/26 11:22, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] powerpc/crash: detached DLPAR CPU nodes accumulate in crash FDT in update_crash_fdt()
> - [High] powerpc/crash: incomplete CPU subnode left in FDT on fdt_setprop() failure in update_crash_fdt()
> --
>
> commit f91f7035e04400925af984e21ed910579118bae0
> Author: Vishal Chourasia <vishalc@linux.ibm.com>
>
> powerpc/crash: Add only the onlined CPU's node to the crash FDT
>
> This patch updates crash FDT handling on CPU online events to pass the onlined
> CPU number to arch_crash_handle_hotplug_event() and insert only that CPU's
> device node into the crash FDT if it is not already present, rather than
> rebuilding all CPU nodes via update_cpus_node().
>
>> diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
>> index 775895f310379..ca37006bddeae 100644
>> --- a/arch/powerpc/kexec/crash.c
>> +++ b/arch/powerpc/kexec/crash.c
> [ ... ]
>> @@ -584,32 +585,71 @@ static int get_fdt_index(struct kimage *image)
>>   }
>>   
>>   /**
>> - * update_crash_fdt - updates the cpus node of the crash FDT.
>> + * update_crash_fdt - add the node of the onlined CPU to the crash FDT.
>>    *
>>    * @image: a pointer to kexec_crash_image
>> + * @cpu: the CPU that was onlined
>> + *
>> + * The node is added as the first child of /cpus, the same position a node
>> + * attached to the live device tree takes. Nothing is done if the FDT already
>> + * has the node, for example when an offlined CPU comes back.
>>    */
>> -static void update_crash_fdt(struct kimage *image)
>> +static void update_crash_fdt(struct kimage *image, unsigned int cpu)
>>   {
>> +	struct device_node *dn;
>> +	struct property *pp;
>>   	void *fdt;
>> -	int fdt_index;
>> +	int fdt_index, cpus_offset, node_offset;
>> +
>> +	dn = of_get_cpu_node(cpu, NULL);
>> +	if (!dn) {
>> +		pr_err("No device node found for CPU %u\n", cpu);
>> +		return;
>> +	}
>>   
>>   	fdt_index = get_fdt_index(image);
>>   	if (fdt_index < 0) {
>>   		pr_err("Unable to locate FDT segment.\n");
>> -		return;
>> +		goto put;
>>   	}
>>   
>>   	fdt = __va((void *)image->segment[fdt_index].mem);
>>   
>> -	/* Temporarily invalidate the crash image while it is replaced */
>> +	cpus_offset = fdt_path_offset(fdt, "/cpus");
>> +	if (cpus_offset < 0) {
>> +		pr_err("Malformed device tree: error reading /cpus node: %s\n",
>> +		       fdt_strerror(cpus_offset));
>> +		goto put;
>> +	}
>> +
>> +	/* Node already present, e.g. an offlined CPU came back online */
>> +	if (fdt_subnode_offset(fdt, cpus_offset, dn->full_name) >= 0)
>> +		goto put;
>> +
>> +	/* Temporarily invalidate the crash image while it is updated */
>>   	xchg(&kexec_crash_image, NULL);
>>   
>> -	/* update FDT to reflect changes in CPU resources */
>> -	if (update_cpus_node(fdt))
>> -		pr_err("Failed to update crash FDT");
>> +	node_offset = fdt_add_subnode(fdt, cpus_offset, dn->full_name);
> [Severity: High]
> What happens across DLPAR CPU removal and addition when update_crash_fdt()
> only appends missing CPU nodes and never removes detached ones?
>
> On pSeries, dlpar_cpu_remove() offlines the CPU threads (where
> KEXEC_CRASH_HP_REMOVE_CPU is a no-op in arch_crash_handle_hotplug_event()),
> pseries_cpuhp_detach_nodes() detaches the CPU device_node from /cpus in the
> live device tree, and pseries_remove_processor() clears its bits in
> cpu_present_mask so those logical CPU IDs can be reused.
>
> Previously, update_cpus_node() deleted all existing cpu subnodes under /cpus
> in the crash FDT and rebuilt them from the live /cpus tree, purging any
> detached DLPAR CPU nodes. With this change, detached CPU subnodes remain in
> the crash FDT indefinitely:

As mentioned in the previous paragraph, during the CPU offline path,
crash handler doesn't update the crash image and returns early.
update_cpus_node() is not invoked.

>
> 1. kdump_extra_fdt_size_ppc64() in arch/powerpc/kexec/file_load_64.c only
>     budgets extra FDT space for possible_cpu_nodes concurrent CPU nodes:
>
> 	if (image->type == KEXEC_TYPE_CRASH) {
> 		possible_cpu_nodes = num_possible_cpus() / threads_per_core;
> 		if (possible_cpu_nodes > cpu_nodes)
> 			extra_size += (possible_cpu_nodes - cpu_nodes) * cpu_node_size();
> 	}
>
>     When a CPU is DLPAR-removed and a CPU with a different DRC index is
>     DLPAR-added, retaining the detached node alongside the newly added node
>     exceeds possible_cpu_nodes and causes fdt_add_subnode() or fdt_setprop() to
>     fail with -FDT_ERR_NOSPACE.
Behaviour is same as before.
>
> 2. Because detached nodes remain under /cpus in the crash FDT,
>     smp_setup_cpu_maps() in the kdump kernel still iterates over them and
>     assigns logical CPU IDs up to nr_cpu_ids, which can push valid present
>     CPUs past nr_cpu_ids.
>
> 3. If the same DRC index is later DLPAR-added again via
>     dlpar_configure_connector() with updated properties (such as
>     ibm,associativity), fdt_subnode_offset(fdt, cpus_offset, dn->full_name) >= 0
>     matches the stale node and skips updating its properties.
>
>> +	if (node_offset < 0) {
>> +		pr_err("Unable to add %s subnode: %s\n", dn->full_name,
>> +		       fdt_strerror(node_offset));
>> +		goto out;
>> +	}
>>   
>> +	for_each_property_of_node(dn, pp) {
>> +		int ret = fdt_setprop(fdt, node_offset, pp->name, pp->value, pp->length);
>> +
>> +		if (ret < 0) {
>> +			pr_err("Unable to add %s property: %s\n", pp->name,
>> +			       fdt_strerror(ret));
>> +			break;
>> +		}
>> +	}
> [Severity: High]
> If fdt_setprop() fails partway through copying the properties in
> update_crash_fdt(), should node_offset be removed with fdt_del_node(fdt,
> node_offset) before breaking out?
>
> Leaving the partially populated subnode under /cpus has two consequences:
>
> 1. Any subsequent update_crash_fdt() call for sibling SMT threads of the same
>     core (or a later re-online of the CPU) sees
>     fdt_subnode_offset(fdt, cpus_offset, dn->full_name) >= 0 and returns early
>     without populating the missing properties.
>
> 2. If the device_type = "cpu" property was written before fdt_setprop() failed
>     on ibm,ppc-interrupt-server#s and reg, early_init_dt_scan_cpus() in
>     arch/powerpc/kernel/prom.c will match the partial CPU node during kdump
>     boot, get NULL for intserv with len left uninitialized, and dereference
>     intserv[i]:
>
> 	intserv = of_get_flat_dt_prop(node, "ibm,ppc-interrupt-server#s", &len);
> 	if (!intserv)
> 		intserv = of_get_flat_dt_prop(node, "reg", &len);
>
> 	nthreads = len / sizeof(int);
>
> 	for (i = 0; i < nthreads; i++) {
> 		if (be32_to_cpu(intserv[i]) ==
> 			fdt_boot_cpuid_phys(initial_boot_params)) {

Similar errors on update_cpus_node() can live the crash fdt
inconsistent and result in crash kernel boot hangs.

>