Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path

[Muchun Song]

> On Apr 1, 2026, at 02:34, Donet Tom <donettom@xxxxxxxxxxxxx> wrote:
> 
> Hi Muchun

Hi,

> 
> On 3/31/26 5:07 PM, Muchun Song wrote:
>> sparse_init_nid() is careful to leave alone every section whose vmemmap
>> has already been set up by sparse_vmemmap_init_nid_early(); it only
>> clears section_mem_map for the rest:
>> 
>>         if (!preinited_vmemmap_section(ms))
>>                 ms->section_mem_map = 0;
>> 
>> A leftover line after that conditional block
>> 
>>         ms->section_mem_map = 0;
>> 
>> was supposed to be deleted but was missed in the failure path, causing the
>> field to be overwritten for all sections when memory allocation fails,
>> effectively destroying the pre-initialization check.
>> 
>> Drop the stray assignment so that preinited sections retain their
>> already valid state.
>> 
>> Fixes: d65917c42373 ("mm/sparse: allow for alternate vmemmap section init at boot")
>> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
>> ---
>>  mm/sparse.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index c2eb36bfb86d..3a14b733bf71 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -584,7 +584,6 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
>>   ms = __nr_to_section(pnum);
>>   if (!preinited_vmemmap_section(ms))
>>   ms->section_mem_map = 0;
>> - ms->section_mem_map = 0;
> 
> 
> This looks correct to me.
> 
> I have a couple of questions:
> 
> 1. As I understand, section_mem_map initially stores the nid
>    during early boot, and later it stores a pointer to an
>    array of struct page. In sparse_init_nid(), the struct page
>    array is stored in section_mem_map via
>    sparse_init_early_section(). If
>    __populate_section_memmap() fails, we are  clearing the nid
>    stored in section_mem_map right?

Right.

> 
> 2. Another question: if sparse_init_nid() fails for some
>    sections, there is no retry mechanism to add them again,
>    correct?

Right.

> 
> 3. when ms->section_mem_map is set to 0
>    for a pre-initialized section, does it only affect the
>    pre-initialization check, or could it lead to other issues?
> 

Only affect pre-initialization. No other issues.

Thanks.

> 
> - Donet
> 
>>   }
>>  }