On 04/11/26 11:22, [email protected] 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 <[email protected]>
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.
