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

On 8/6/26 17:51, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

Not correct, will handle MEMBLOCK_NODUMP flag in patch8.

> 
> [ ... ]
> 
>> @@ -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;
>     }

This is indeed an issue, but it was not introduced by this patch.
A separate patch will be sent to fix it.

> 
> 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?
>