Re: [PATCH v6 09/10] of: reserved_mem: mark /memreserve/ entries as MEMBLOCK_NODUMP

[Wandun <chenwandun1@gmail.com>]

On 9/18/26 06:07, Rob Herring wrote:
> On Wed, Sep 02, 2026 at 09:02:11AM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> - [High] FDT /memreserve/ entries marked as MEMBLOCK_NODUMP can be later freed to the buddy allocator, causing active kernel memory to be silently omitted from crash dumps.
>> - [Medium] 64-bit FDT reserve entries are checked for overlap in 64-bit space but implicitly truncated to phys_addr_t when marked as NODUMP, potentially corrupting dumpable regions on 32-bit systems.
>> --
>>
>> commit f6e6bcd14fb6f4946f258fc34089bbbdcd52f624
>> Author: Wandun Chen <chenwandun@lixiang.com>
>>
>> of: reserved_mem: mark /memreserve/ entries as MEMBLOCK_NODUMP
>>
>> This patch marks /memreserve/ entries in the device tree as MEMBLOCK_NODUMP
>> so they are excluded from kernel crash dumps. It explicitly leaves regions
>> that overlap with a dumpable region unmarked.
>>
>>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>>> index f29a07e8fb23e..a42cf6eac55b5 100644
>>> --- a/drivers/of/of_reserved_mem.c
>>> +++ b/drivers/of/of_reserved_mem.c
>> [ ... ]
>>> @@ -251,6 +251,39 @@ static void __init __rmem_check_for_overlap(void)
>>>  	}
>>>  }
>>>  
>>> +void __init fdt_mark_memreserve_nodump(void)
>>> +{
>>> +	u64 base, size;
>>> +	int n;
>>> +	const void *fdt = initial_boot_params;
>>> +
>>> +	if (!IS_ENABLED(CONFIG_CRASH_DUMP))
>>> +		return;
>>> +
>>> +	if (!fdt)
>>> +		return;
>>> +
>>> +	for (n = 0; ; n++) {
>>> +		int i;
>>> +
>>> +		if (fdt_get_mem_rsv(fdt, n, &base, &size))
>>> +			break;
>>> +		if (!size)
>>> +			break;
>>> +
>>> +		for (i = 0; i < reserved_mem_count; i++) {
>>> +			struct reserved_mem *rmem = &reserved_mem[i];
>>> +
>>> +			if (rmem->dumpable &&
>>> +			    base < (u64)rmem->base + (u64)rmem->size &&
>>> +			    (u64)rmem->base < base + size)
>>> +				break;
>>> +		}
>>> +		if (i == reserved_mem_count)
>>> +			memblock_mark_nodump(base, size);
>>
>> [Severity: Medium]
>> Will this corrupt dumpable regions on 32-bit systems?
>>
>> base and size are u64, so the overlap check evaluates safely in 64-bit
>> space. However, memblock_mark_nodump() takes phys_addr_t. On 32-bit
>> systems, could a 64-bit FDT reserve entry > 4GB correctly pass the overlap
>> check above, but then get implicitly truncated to a 32-bit address when
>> passed to memblock_mark_nodump(), incorrectly marking a lower 32-bit
>> physical address as NODUMP without having checked it for overlap?
> 
> Unlikely. Such a system would probably enable LPAE or equivalent.
>  
>> [Severity: High]
>> Can this cause active kernel memory to be silently omitted from crash dumps?
>>
>> If the bootloader specifies a /memreserve/ entry for memory that the kernel
>> later frees (such as the initrd), this memory is returned to the buddy
>> allocator.
>>
>> Since the MEMBLOCK_NODUMP flag is never cleared, will this cause reused
>> pages to be incorrectly skipped and omitted from the crash dump later on?
> 
> This seems like an issue. Both the FDT and initrd are typically in 
> /memreserve/ and can be moved.

Thanks for the review, I will fix it in next version.

Best regards,
Wandun>
> Rob