Re: [PATCH v5 1/8] memblock: introduce MEMBLOCK_NODUMP flag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch completely omits the filtering logic required to exclude MEMBLOCK_NODUMP regions from kdump vmcores.
- [Medium] The MEMBLOCK_NODUMP flag extends the flagname array but leaves an uninitialized hole for MEMBLOCK_RSRV_HUGETLB, leading to a NULL format argument.
--

--- Patch [1]: [PATCH v5 1/8] memblock: introduce MEMBLOCK_NODUMP flag ---
commit b30a0bfd2a4e00657ccc3f49bdfa4a7ca5fdfa1d
Author: Wandun Chen <chenwandun@lixiang.com>

memblock: introduce MEMBLOCK_NODUMP flag

Add MEMBLOCK_NODUMP to mark regions that should be excluded from kdump vmcores.
The flag is meant for reserved memory that carries no data useful for crash
analysis. Subsequent patches wire this up for /reserved-memory and
/memreserve/ entries.

> diff --git a/mm/memblock.c b/mm/memblock.c
> index 9ce86349a29fa..79231634c25b0 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1204,6 +1204,21 @@ __init int memblock_clear_kho_scratch(phys_addr_t base, phys_addr_t size)
>  				    MEMBLOCK_KHO_SCRATCH);
>  }
>  
> +/**
> + * memblock_mark_nodump - Mark a memory region with flag MEMBLOCK_NODUMP.
> + * @base: the base phys addr of the region
> + * @size: the size of the region
> + *
> + * Mark the region as not to be included in crash dumps.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +int __init_memblock memblock_mark_nodump(phys_addr_t base, phys_addr_t size)
> +{
> +	return memblock_setclr_flag(&memblock.memory, base, size, 1,
> +				    MEMBLOCK_NODUMP);
> +}
> +

[Severity: High]
Is it intentional that the filtering logic for this new flag was omitted?

Without updating should_skip_region() in this file to filter out regions
marked with MEMBLOCK_NODUMP, the flag appears to have no functional effect.

When architecture kdump setup iterates over memory ranges (e.g., using
for_each_mem_range()) to build the crash dump list, these regions will still
be included because should_skip_region() does not know to skip them.

[ ... ]

> @@ -2886,6 +2901,7 @@ static const char * const flagname[] = {
>  	[ilog2(MEMBLOCK_RSRV_NOINIT)] = "RSV_NIT",
>  	[ilog2(MEMBLOCK_RSRV_KERN)] = "RSV_KERN",
>  	[ilog2(MEMBLOCK_KHO_SCRATCH)] = "KHO_SCRATCH",
> +	[ilog2(MEMBLOCK_NODUMP)] = "NODUMP",
>  };
>  
>  static int memblock_debug_show(struct seq_file *m, void *private)

[Severity: Medium]
Will this expansion of the array leave a NULL pointer hole for the
MEMBLOCK_RSRV_HUGETLB flag?

Prior to this patch, the max index was 6. MEMBLOCK_RSRV_HUGETLB is bit 7,
which safely fell back to printing "UNKNOWN". 

Adding MEMBLOCK_NODUMP (bit 8) expands the array size to 9. Due to designated
initialization, index 7 becomes NULL.

When memblock_debug_show() prints a region with MEMBLOCK_RSRV_HUGETLB set,
it will read the NULL pointer and pass it to seq_printf():

    if (reg->flags & (1U << j)) {
        seq_printf(m, "%s\n", flagname[j]);
        break;
    }

While seq_printf() will safely print "(null)", this will break out of the
printing loop early and omit any other flags. Could MEMBLOCK_RSRV_HUGETLB
be added to the array as well to avoid this hole?

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