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

[Donet Tom <donettom@linux.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 2459 bytes --]


On 4/1/26 7:58 AM, Muchun Song wrote:
>
>> On Apr 1, 2026, at 02:34, Donet Tom<donettom@linux.ibm.com> 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@bytedance.com>
>>> ---
>>>   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 for the clarification.

This looks good to me.

Reviewed by: Donet Tom <donettom@linux.ibm.com>



>
> Thanks.
>
>> - Donet
>>
>>>    }
>>>   }
>

[-- Attachment #2: Type: text/html, Size: 4688 bytes --]