Re: [PATCH v5 3/8] of: reserved_mem: mark /reserved-memory entries with MEMBLOCK_NODUMP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Marking reserved memory as MEMBLOCK_NODUMP causes a kernel panic by splitting memblock arrays before resizing is permitted.
- [High] The MEMBLOCK_NODUMP flag is inert dead code that fails to exclude regions from kdump.

Pre-existing issues:
- [High] Physical memory for dynamically allocated 'no-map' regions is leaked on driver initialization failure.
- [High] Error path unconditionally un-reserves statically placed memory, potentially corrupting overlapping critical reservations.
--

--- Patch [3]: [PATCH v5 3/8] of: reserved_mem: mark /reserved-memory entries with MEMBLOCK_NODUMP ---
commit 2e35731dcf703e597205458fb3ba8ad782294cfa
Author: Wandun Chen <chenwandun@lixiang.com>

of: reserved_mem: mark /reserved-memory entries with MEMBLOCK_NODUMP

This commit marks non-dumpable reserved-memory regions with MEMBLOCK_NODUMP
so kdump can omit them from the vmcore. The marking is guarded by
CONFIG_CRASH_DUMP so non-kdump kernels do not pay the cost of splitting
memblock.memory entries at NODUMP boundaries.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 7b1616b9c6c21..f38d11df6f152 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -674,6 +674,9 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>  			rmem->name ? rmem->name : "unknown");
>  	}
>  
> +	if (IS_ENABLED(CONFIG_CRASH_DUMP) && !rmem->dumpable)
> +		memblock_mark_nodump(rmem->base, rmem->size);

[Severity: Critical]
Does this memblock_mark_nodump() call trigger a boot-time panic?

fdt_init_reserved_mem_node() runs very early in the boot sequence.
When memblock_mark_nodump() is called, it splits existing memory regions to
isolate the nodump regions.

Since efi and e820 maps already consume many entries of the static 128-entry
memblock array, these new splits can quickly exhaust the array. When
memblock_double_array() is invoked to allocate more slots, it checks
memblock_can_resize, which is still 0 at this early stage.

Will this result in an immediate panic("memblock: cannot resize %s array\n")
for any platform booting with device tree reserved-memory nodes that exceed
the static array limits?

[Severity: High]
Does the MEMBLOCK_NODUMP flag actually exclude regions from kdump?

A full tree search reveals that this flag is never checked by
should_skip_region(), /proc/iomem exporters, or any kdump filtering logic.

Architecture setups like request_standard_resources() in arm64 only check
memblock_is_nomap(), meaning nodump regions are still exported unconditionally
to /proc/iomem as 'System RAM'.

Since the flag is never read by the kernel, will kexec-tools parse these
regions and include them in the vmcore anyway, defeating the purpose of
the patch?

> +
>  	reserved_mem_count++;
>  }

[Severity: High]
This is a pre-existing issue, but does fdt_init_reserved_mem_node() leak
physical memory for dynamically allocated 'no-map' regions on driver
initialization failure?

If __reserved_mem_init_node() returns an error, the error handling path
attempts to clean up the reservation:

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
	if (err != 0 && err != -ENODEV) {
		pr_info("node %s compatible matching fail\n", rmem->name);
		rmem->name = NULL;

		if (nomap)
			memblock_clear_nomap(rmem->base, rmem->size);
		else
			memblock_phys_free(rmem->base, rmem->size);
		return;
	}
    ...
}

For regions with the no-map property, it calls memblock_clear_nomap(), which
only removes the flag. It does not call memblock_phys_free().

Since dynamically placed regions were allocated via memblock_phys_alloc_range()
and added to memblock.reserved, doesn't this leave the physical memory
permanently in memblock.reserved without returning it to the buddy allocator?

[Severity: High]
This is also a pre-existing issue in the same error path, but does calling
memblock_phys_free() for statically placed memory unconditionally un-reserve
memory, potentially corrupting overlapping critical reservations?

For statically placed reserved-memory nodes, early_init_dt_reserve_memory()
calls memblock_reserve(), which successfully merges the region even if it
overlaps with an existing critical reservation like kernel text.

If __reserved_mem_init_node() later fails for this node, the error path
calls memblock_phys_free():

drivers/of/of_reserved_mem.c:fdt_init_reserved_mem_node() {
    ...
		else
			memblock_phys_free(rmem->base, rmem->size);
    ...
}

This translates to memblock_remove_range(&memblock.reserved, ...), which
blindly un-reserves the entire range.

If this overlapping critical memory loses its reservation, won't the buddy
allocator later hand those pages out for general use, corrupting the critical
data?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806093519.2907771-1-chenwandun1@gmail.com?part=3