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

[Muchun Song <muchun.song@linux.dev>]

> On Apr 1, 2026, at 15:25, David Hildenbrand (Arm) <david@kernel.org> wrote:
> 
> On 4/1/26 04:41, Muchun Song wrote:
>> 
>> 
>>> On Apr 1, 2026, at 04:42, David Hildenbrand (Arm) <david@kernel.org> wrote:
>>> 
>>> On 3/31/26 13:37, 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;
>>> 
>>> 
>>> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>> 
>> Thanks.
>> 
>>> 
>>> I have some cleanup patches lying around that cleanup that code heavily.
>>> I think I get rid of this questionable "failed to allocate" case entirely.
>> 
>> It's truly a coincidence — I also have a piece of code locally
>> that does something similar. Since allocation failure would also
>> affect subsequent startup processes, I simply made it panic when
>> allocation fails.
> 
> Don't use BUG_ON, use actual panic(). :)

Got it.

> 
> And yes, we should rip out that handling.

Agree.

> 
> -- 
> Cheers,
> 
> David