mm/sparse.c | 1 - 1 file changed, 1 deletion(-)
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;
}
}
--
2.20.1On 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>
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.
--
Cheers,
David
> 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.
mm: panic on memory allocation failure in sparse_init_nid()
When vmemmap pages allocation or usemap allocation fails, sparse_init_nid()
currently only marks the corresponding section as non-present. However,
subsequent code like memmap_init() iterating over PFNs does not check for
non-present sections, leading to invalid memory access (additional,
subsection_map_init() accessing the unallocated usemap as well).
It is complex to audit and fix all boot-time PFN iterators to handle these
partially initialized sections correctly. Since vmemmap and usemap allocation
failures are extremely rare during early boot, the more appropriate approach
is to expose the problem as early as possible.
Therefore, use BUG_ON() to panic immediately if allocation fails, instead of
attempting a partial recovery that leads to obscure crashes later.
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
diff --git a/mm/sparse.c b/mm/sparse.c
index 1b017026925e..bb0b86a4f3ef 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -354,19 +354,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
unsigned long map_count)
{
unsigned long pnum;
- struct page *map;
- struct mem_section *ms;
-
- if (sparse_usage_init(nid, map_count)) {
- pr_err("%s: node[%d] usemap allocation failed", __func__, nid);
- goto failed;
- }
+ BUG_ON(sparse_usage_init(nid, map_count));
sparse_buffer_init(map_count * section_map_size(), nid);
sparse_vmemmap_init_nid_early(nid);
for_each_present_section_nr(pnum_begin, pnum) {
+ struct mem_section *ms;
unsigned long pfn = section_nr_to_pfn(pnum);
if (pnum >= pnum_end)
@@ -374,16 +369,11 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
ms = __nr_to_section(pnum);
if (!preinited_vmemmap_section(ms)) {
+ struct page *map;
+
map = __populate_section_memmap(pfn, PAGES_PER_SECTION,
- nid, NULL, NULL);
- if (!map) {
- pr_err("%s: node[%d] memory map backing failed. Some memory will not be available.",
- __func__, nid);
- pnum_begin = pnum;
- sparse_usage_fini();
- sparse_buffer_fini();
- goto failed;
- }
+ nid, NULL, NULL);
+ BUG_ON(!map);
memmap_boot_pages_add(DIV_ROUND_UP(PAGES_PER_SECTION * sizeof(struct page),
PAGE_SIZE));
sparse_init_early_section(nid, map, pnum, 0);
@@ -391,19 +381,6 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
}
sparse_usage_fini();
sparse_buffer_fini();
- return;
-failed:
- /*
- * We failed to allocate, mark all the following pnums as not present,
- * except the ones already initialized earlier.
- */
- for_each_present_section_nr(pnum_begin, pnum) {
- if (pnum >= pnum_end)
- break;
- ms = __nr_to_section(pnum);
- if (!preinited_vmemmap_section(ms))
- ms->section_mem_map = 0;
- }
}
/*
>
> --
> Cheers,
>
> David
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(). :)
And yes, we should rip out that handling.
--
Cheers,
David
> 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
On Tue, 31 Mar 2026 19:37:24 +0800 Muchun Song <songmuchun@bytedance.com> 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. Here I go again ;) Are there userspace impacts? AI review thinks it found a different bug: https://sashiko.dev/#/patchset/20260331113724.2080833-1-songmuchun@bytedance.com
> On Apr 1, 2026, at 04:10, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 31 Mar 2026 19:37:24 +0800 Muchun Song <songmuchun@bytedance.com> 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. > > Here I go again ;) Are there userspace impacts? Those pre-inited sections (HugeTLB pages) are not activated. However, such failures are extremely rare, so I don't see any major issues. > > AI review thinks it found a different bug: > https://sashiko.dev/#/patchset/20260331113724.2080833-1-songmuchun@bytedance.com I don't think the issue reported by AI is a real problem, because the allocation of sparse_usagebuf has already taken these hugetlb sections into account. Thanks.
On 3/31/26 22:10, Andrew Morton wrote: > On Tue, 31 Mar 2026 19:37:24 +0800 Muchun Song <songmuchun@bytedance.com> 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. > > Here I go again ;) Are there userspace impacts? > > AI review thinks it found a different bug: > https://sashiko.dev/#/patchset/20260331113724.2080833-1-songmuchun@bytedance.com It speculates about a BUG_ON that we should easily hit with hugetlb vmemmap optimization during boot I think (which we don't)? That naturally make me skeptical. But sounds like something to double check independently of this patch here. -- Cheers, David
Hi Muchun
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?
2. Another question: if sparse_init_nid() fails for some
sections, there is no retry mechanism to add them again,
correct?
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?
- Donet
> }
> }
>
> 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.
>
> - Donet
>
>> }
>> }
© 2016 - 2026 Red Hat, Inc.