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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] of: reserved_mem: unaligned memblock split in fdt_mark_reserve_mem_nodump()
- [Medium] of: reserved_mem: fdt_mark_reserve_mem_nodump() marks failed reservations
--

commit a452b9d1fad838de511c25fe48e4541c58357355
Author: Wandun Chen <chenwandun@lixiang.com>

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

This commit adds fdt_mark_reserve_mem_nodump() to mark non-dumpable
reserved-memory regions in memblock.memory so kdump omits them from the
vmcore when crash dump support is enabled.

> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8c9d6395d6a39..db2bf76000d23 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -677,6 +677,24 @@ static void __init fdt_init_reserved_mem_node(unsigned long node, const char *un
>  	reserved_mem_count++;
>  }
>  
> +void __init fdt_mark_reserve_mem_nodump(void)
> +{
> +	struct reserved_mem *rmem;
> +	int i;
> +
> +	if (!IS_ENABLED(CONFIG_CRASH_DUMP))
> +		return;
> +
> +	if (!IS_ENABLED(CONFIG_KEXEC_FILE) && !IS_ENABLED(CONFIG_CRASH_HOTPLUG))
> +		return;
> +
> +	for (i = 0; i < reserved_mem_count; i++) {
> +		rmem = &reserved_mem[i];
> +		if (rmem->size && !rmem->dumpable)
> +			memblock_mark_nodump(rmem->base, rmem->size);
> +	}

[Severity: High]
What happens here when rmem->base or rmem->size is not page-aligned, or
when rmem->size is smaller than a page?

Previously, mapped reserved-memory regions were only recorded in
memblock.reserved and did not split memblock.memory. Calling
memblock_mark_nodump() splits memblock.memory at rmem->base and
rmem->base + rmem->size, but reserved-memory regions are not guaranteed to
be page-aligned:

drivers/of/of_reserved_mem.c:early_init_dt_alloc_reserved_memory_arch() {
	end = !end ? MEMBLOCK_ALLOC_ANYWHERE : end;
	align = !align ? SMP_CACHE_BYTES : align;
	base = memblock_phys_alloc_range(size, align, start, end);
}

Static device tree regions without no-map can also be smaller than a page
or unaligned to the page size (for example, cfe-stub@0 in
arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi with a 0x1000 size under
16k or 64k page configurations).

When memblock.memory is split at an unaligned or sub-page boundary,
__next_mem_pfn_range() rounds each region inward to page boundaries and
skips sub-page regions:

mm/memblock.c:__next_mem_pfn_range() {
	while (++*idx < type->cnt) {
		r = &type->regions[*idx];
		r_nid = memblock_get_region_node(r);

		if (PFN_UP(r->base) >= PFN_DOWN(r->base + r->size))
			continue;
}

This drops any page frame straddling an unaligned split boundary from
for_each_mem_pfn_range() and the page allocator.

In addition, request_standard_resources() in arch/arm64/kernel/setup.c and
init_resources() in arch/riscv/kernel/setup.c register each mapped
memblock.memory region using memblock_region_memory_base_pfn() and
memblock_region_memory_end_pfn():

arch/riscv/kernel/setup.c:init_resources() {
		res->start = __pfn_to_phys(memblock_region_memory_base_pfn(region));
		res->end = __pfn_to_phys(memblock_region_memory_end_pfn(region)) - 1;

		ret = add_resource(&iomem_resource, res);
		if (ret < 0)
			goto error;
}

For a sub-page region where PFN_UP(base) >= PFN_DOWN(base + size), res->end
becomes smaller than res->start (or underflows when end_pfn is 0), causing
add_resource() on riscv to fail and tear down iomem_resource, and producing
an inverted or underflowed resource on arm64.

Splitting memblock.memory also splits hyp_memory[] on arm64 pkvm, where
check_range_allowed_memory() in arch/arm64/kvm/hyp/nvhe/mem_protect.c
requires checked ranges to fall within a single region:

arch/arm64/kvm/hyp/nvhe/mem_protect.c:check_range_allowed_memory() {
	reg = find_mem_range(start, &range);
	if (!is_in_mem_range(end - 1, &range))
		return -EINVAL;
}

Could memblock_mark_nodump() corrupt resource registration, drop straddling
pages, or break pkvm range checks when regions are not page-aligned?

[Severity: Medium]
Can fdt_mark_reserve_mem_nodump() also end up marking active kernel memory
as non-dumpable if an early static reservation failed?

During early boot in fdt_scan_reserved_mem(), __reserved_mem_reserve_reg()
logs an error if early_init_dt_reserve_memory() fails (for example, if a
static region overlaps the kernel image or initrd), but still returns 0:

drivers/of/of_reserved_mem.c:__reserved_mem_reserve_reg() {
	if (size && early_init_dt_reserve_memory(base, size, nomap) == 0) {
		fdt_fixup_reserved_mem_node(node, base, size);
		pr_debug("Reserved memory: reserved region for node '%s': base %pa, size %lu MiB\n",
			 uname, &base, (unsigned long)(size / SZ_1M));
	} else {
		pr_err("Reserved memory: failed to reserve memory for node '%s': base %pa, size %lu MiB\n",
		       uname, &base, (unsigned long)(size / SZ_1M));
	}
	return 0;
}

Later, fdt_scan_reserved_mem_late() scans all available static nodes in
/reserved-memory and passes them to fdt_init_reserved_mem_node(), recording
them in reserved_mem[] regardless of whether the early reservation
succeeded:

drivers/of/of_reserved_mem.c:fdt_scan_reserved_mem_late() {
		if (size) {
			uname = fdt_get_name(fdt, child, NULL);
			fdt_init_reserved_mem_node(child, uname, base, size);
		}
}

Wouldn't fdt_mark_reserve_mem_nodump() then mark that unreserved range in
memblock.memory, causing kdump to omit memory that is actually in use by
the kernel?

> +}
> +
>  struct rmem_assigned_device {
>  	struct device *dev;
>  	struct reserved_mem *rmem;

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