public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
@ 2026-03-31 11:37 Muchun Song
  2026-03-31 18:34 ` Donet Tom
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Muchun Song @ 2026-03-31 11:37 UTC (permalink / raw)
  To: Andrew Morton, David Hildenbrand
  Cc: Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Frank van der Linden, linux-mm,
	linux-kernel, muchun.song, Muchun Song

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.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 11:37 [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path Muchun Song
@ 2026-03-31 18:34 ` Donet Tom
  2026-04-01  2:28   ` Muchun Song
  2026-03-31 20:10 ` Andrew Morton
  2026-03-31 20:42 ` David Hildenbrand (Arm)
  2 siblings, 1 reply; 13+ messages in thread
From: Donet Tom @ 2026-03-31 18:34 UTC (permalink / raw)
  To: Muchun Song, Andrew Morton, David Hildenbrand
  Cc: Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Frank van der Linden, linux-mm,
	linux-kernel, muchun.song

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

>   	}
>   }
>   

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 11:37 [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path Muchun Song
  2026-03-31 18:34 ` Donet Tom
@ 2026-03-31 20:10 ` Andrew Morton
  2026-03-31 21:06   ` David Hildenbrand (Arm)
  2026-04-01  2:37   ` Muchun Song
  2026-03-31 20:42 ` David Hildenbrand (Arm)
  2 siblings, 2 replies; 13+ messages in thread
From: Andrew Morton @ 2026-03-31 20:10 UTC (permalink / raw)
  To: Muchun Song
  Cc: David Hildenbrand, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel, muchun.song

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 11:37 [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path Muchun Song
  2026-03-31 18:34 ` Donet Tom
  2026-03-31 20:10 ` Andrew Morton
@ 2026-03-31 20:42 ` David Hildenbrand (Arm)
  2026-04-01  2:41   ` Muchun Song
  2 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand (Arm) @ 2026-03-31 20:42 UTC (permalink / raw)
  To: Muchun Song, Andrew Morton
  Cc: Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Frank van der Linden, linux-mm,
	linux-kernel, muchun.song

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>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 20:10 ` Andrew Morton
@ 2026-03-31 21:06   ` David Hildenbrand (Arm)
  2026-04-01  2:37   ` Muchun Song
  1 sibling, 0 replies; 13+ messages in thread
From: David Hildenbrand (Arm) @ 2026-03-31 21:06 UTC (permalink / raw)
  To: Andrew Morton, Muchun Song
  Cc: Lorenzo Stoakes, Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Frank van der Linden, linux-mm,
	linux-kernel, muchun.song

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 18:34 ` Donet Tom
@ 2026-04-01  2:28   ` Muchun Song
  0 siblings, 0 replies; 13+ messages in thread
From: Muchun Song @ 2026-04-01  2:28 UTC (permalink / raw)
  To: Donet Tom
  Cc: Muchun Song, Andrew Morton, David Hildenbrand, Lorenzo Stoakes,
	Liam R. Howlett, Vlastimil Babka, Mike Rapoport,
	Suren Baghdasaryan, Michal Hocko, Frank van der Linden, linux-mm,
	linux-kernel



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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 20:10 ` Andrew Morton
  2026-03-31 21:06   ` David Hildenbrand (Arm)
@ 2026-04-01  2:37   ` Muchun Song
  1 sibling, 0 replies; 13+ messages in thread
From: Muchun Song @ 2026-04-01  2:37 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Muchun Song, David Hildenbrand, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel



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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-03-31 20:42 ` David Hildenbrand (Arm)
@ 2026-04-01  2:41   ` Muchun Song
  2026-04-01  7:25     ` David Hildenbrand (Arm)
  0 siblings, 1 reply; 13+ messages in thread
From: Muchun Song @ 2026-04-01  2:41 UTC (permalink / raw)
  To: David Hildenbrand (Arm)
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel



> 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



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-04-01  2:41   ` Muchun Song
@ 2026-04-01  7:25     ` David Hildenbrand (Arm)
  2026-04-01  7:28       ` Muchun Song
  2026-04-02  7:37       ` Donet Tom
  0 siblings, 2 replies; 13+ messages in thread
From: David Hildenbrand (Arm) @ 2026-04-01  7:25 UTC (permalink / raw)
  To: Muchun Song
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-04-01  7:25     ` David Hildenbrand (Arm)
@ 2026-04-01  7:28       ` Muchun Song
  2026-04-02  7:37       ` Donet Tom
  1 sibling, 0 replies; 13+ messages in thread
From: Muchun Song @ 2026-04-01  7:28 UTC (permalink / raw)
  To: David Hildenbrand (Arm)
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel



> 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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-04-01  7:25     ` David Hildenbrand (Arm)
  2026-04-01  7:28       ` Muchun Song
@ 2026-04-02  7:37       ` Donet Tom
  2026-04-02  7:56         ` David Hildenbrand (Arm)
  1 sibling, 1 reply; 13+ messages in thread
From: Donet Tom @ 2026-04-02  7:37 UTC (permalink / raw)
  To: David Hildenbrand (Arm), Muchun Song
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel


On 4/1/26 12:55 PM, David Hildenbrand (Arm) 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(). :)


Apologies if I’m missing something, but would it make sense to check the 
section validity in memmap_init() instead of panicking? Is there a 
reason this is avoided?

-Donet


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-04-02  7:37       ` Donet Tom
@ 2026-04-02  7:56         ` David Hildenbrand (Arm)
  2026-04-02 12:12           ` Donet Tom
  0 siblings, 1 reply; 13+ messages in thread
From: David Hildenbrand (Arm) @ 2026-04-02  7:56 UTC (permalink / raw)
  To: Donet Tom, Muchun Song
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel

On 4/2/26 09:37, Donet Tom wrote:
> 
> On 4/1/26 12:55 PM, David Hildenbrand (Arm) wrote:
>> On 4/1/26 04:41, Muchun Song wrote:
>>>
>>> Thanks.
>>>
>>> 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(). :)
> 
> 
> Apologies if I’m missing something, but would it make sense to check the
> section validity in memmap_init() instead of panicking? Is there a
> reason this is avoided?

Not sure I understand your question, but trying to handle memmap
allocations during early boot should be removed. If that ever happens
something is fundamentally flawed.

-- 
Cheers,

David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path
  2026-04-02  7:56         ` David Hildenbrand (Arm)
@ 2026-04-02 12:12           ` Donet Tom
  0 siblings, 0 replies; 13+ messages in thread
From: Donet Tom @ 2026-04-02 12:12 UTC (permalink / raw)
  To: David Hildenbrand (Arm), Muchun Song
  Cc: Muchun Song, Andrew Morton, Lorenzo Stoakes, Liam R. Howlett,
	Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
	Frank van der Linden, linux-mm, linux-kernel


On 4/2/26 1:26 PM, David Hildenbrand (Arm) wrote:
> On 4/2/26 09:37, Donet Tom wrote:
>> On 4/1/26 12:55 PM, David Hildenbrand (Arm) wrote:
>>> On 4/1/26 04:41, Muchun Song wrote:
>>>> Thanks.
>>>>
>>>> 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(). :)
>>
>> Apologies if I’m missing something, but would it make sense to check the
>> section validity in memmap_init() instead of panicking? Is there a
>> reason this is avoided?
> Not sure I understand your question, but trying to handle memmap
> allocations during early boot should be removed. If that ever happens
> something is fundamentally flawed.


Thank you for the clarification—I understand now.

-Donet

>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2026-04-02 12:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-31 11:37 [PATCH] mm/sparse: fix preinited section_mem_map clobbering on failure path Muchun Song
2026-03-31 18:34 ` Donet Tom
2026-04-01  2:28   ` Muchun Song
2026-03-31 20:10 ` Andrew Morton
2026-03-31 21:06   ` David Hildenbrand (Arm)
2026-04-01  2:37   ` Muchun Song
2026-03-31 20:42 ` David Hildenbrand (Arm)
2026-04-01  2:41   ` Muchun Song
2026-04-01  7:25     ` David Hildenbrand (Arm)
2026-04-01  7:28       ` Muchun Song
2026-04-02  7:37       ` Donet Tom
2026-04-02  7:56         ` David Hildenbrand (Arm)
2026-04-02 12:12           ` Donet Tom

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox