[PATCH 1/1] mm/memory_hotplug: fix missing rollback in __add_pages()

[PATCH 1/1] mm/memory_hotplug: fix missing rollback in __add_pages()

[Lance Yang]

__add_pages() returns on a sparse_add_section() failure without removing
the sections already added in the same request.

For memremap_pages(), the failed range is not counted in pgmap->nr_range,
so memunmap_pages() skips it. The sections already added in that range
retain their vmemmap mappings and subsection bits. Retrying a
section-aligned range can then fail with -EEXIST.

Save the initial PFN and remove [start_pfn, pfn) on failure. For a vmemmap
population failure, section_activate() already cleans up the current
section, so the rollback excludes it. If the first section fails,
__remove_pages() receives an empty range and does nothing.

Link: https://lore.kernel.org/all/BAD58999-1EDD-4A37-ABA0-DB1BD8AB3453@linux.dev/
Suggested-by: Muchun Song <muchun.song@linux.dev>
Signed-off-by: Lance Yang <lance.yang@linux.dev>
---
No Fixes tag, as I couldn't identify the commit that introduced this issue.

 mm/memory_hotplug.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 796af1028ee2..ca4656698148 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -380,6 +380,7 @@ EXPORT_SYMBOL_GPL(pfn_to_online_page);
 int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 		struct mhp_params *params)
 {
+	const unsigned long start_pfn = pfn;
 	const unsigned long end_pfn = pfn + nr_pages;
 	unsigned long cur_nr_pages;
 	int err;
@@ -413,8 +414,11 @@ int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 				   SECTION_ALIGN_UP(pfn + 1) - pfn);
 		err = sparse_add_section(nid, pfn, cur_nr_pages, altmap,
 					 params->pgmap);
-		if (err)
+		if (err) {
+			__remove_pages(start_pfn, pfn - start_pfn, altmap,
+				       params->pgmap);
 			break;
+		}
 		cond_resched();
 	}
 	vmemmap_populate_print_last();
-- 
2.39.3

Re: [PATCH 1/1] mm/memory_hotplug: fix missing rollback in __add_pages()

[David Hildenbrand (Arm)]

On 9/30/26 18:04, Lance Yang wrote:
> __add_pages() returns on a sparse_add_section() failure without removing
> the sections already added in the same request.
> 
> For memremap_pages(), the failed range is not counted in pgmap->nr_range,
> so memunmap_pages() skips it. The sections already added in that range
> retain their vmemmap mappings and subsection bits. Retrying a
> section-aligned range can then fail with -EEXIST.
> 
> Save the initial PFN and remove [start_pfn, pfn) on failure. For a vmemmap
> population failure, section_activate() already cleans up the current
> section, so the rollback excludes it. If the first section fails,
> __remove_pages() receives an empty range and does nothing.
> 
> Link: https://lore.kernel.org/all/BAD58999-1EDD-4A37-ABA0-DB1BD8AB3453@linux.dev/
> Suggested-by: Muchun Song <muchun.song@linux.dev>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
> No Fixes tag, as I couldn't identify the commit that introduced this issue.
> 
>  mm/memory_hotplug.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 796af1028ee2..ca4656698148 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -380,6 +380,7 @@ EXPORT_SYMBOL_GPL(pfn_to_online_page);
>  int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  		struct mhp_params *params)
>  {
> +	const unsigned long start_pfn = pfn;
>  	const unsigned long end_pfn = pfn + nr_pages;
>  	unsigned long cur_nr_pages;
>  	int err;
> @@ -413,8 +414,11 @@ int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  				   SECTION_ALIGN_UP(pfn + 1) - pfn);
>  		err = sparse_add_section(nid, pfn, cur_nr_pages, altmap,
>  					 params->pgmap);
> -		if (err)
> +		if (err) {
> +			__remove_pages(start_pfn, pfn - start_pfn, altmap,
> +				       params->pgmap);
>  			break;
> +		}
>  		cond_resched();
>  	}
>  	vmemmap_populate_print_last();

Makes sense and LGTM.

Do we have a Fixes: tag? It probably dates back quite a while ... not sure about
stable, we never saw this in practice. But if it's easy, we should just do it?
(not sure if we ever had __remove_pages be limited to hotunplug support)

I'm planning on picking this up and sending it for the next merge window (so not
as a hotfix).

Looking at this ...

x86 does not really expect add_pages to fail:

	ret = __add_pages(nid, start_pfn, nr_pages, params);
	WARN_ON_ONCE(ret);

That's probably something to clean up as well?

-- 
Cheers,

David

Re: [PATCH 1/1] mm/memory_hotplug: fix missing rollback in __add_pages()

[Muchun Song]

> On Oct 1, 2026, at 00:04, Lance Yang <lance.yang@linux.dev> wrote:
> 
> __add_pages() returns on a sparse_add_section() failure without removing
> the sections already added in the same request.
> 
> For memremap_pages(), the failed range is not counted in pgmap->nr_range,
> so memunmap_pages() skips it. The sections already added in that range
> retain their vmemmap mappings and subsection bits. Retrying a
> section-aligned range can then fail with -EEXIST.
> 
> Save the initial PFN and remove [start_pfn, pfn) on failure. For a vmemmap
> population failure, section_activate() already cleans up the current
> section, so the rollback excludes it. If the first section fails,
> __remove_pages() receives an empty range and does nothing.
> 
> Link: https://lore.kernel.org/all/BAD58999-1EDD-4A37-ABA0-DB1BD8AB3453@linux.dev/
> Suggested-by: Muchun Song <muchun.song@linux.dev>
> Signed-off-by: Lance Yang <lance.yang@linux.dev>
> ---
> No Fixes tag, as I couldn't identify the commit that introduced this issue.

Hi Lance,

LLMs are quite good at tracing this kind of code history, so I used one
to go through the relevant commits and identify the correct Fixes tag.

Fixes: ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")

Before that commit, __add_pages() ignored -EEXIST and continued with
the remaining sections. After a partial failure, a retry could therefore
reuse the vmemmap of sections added by the failed attempt and continue
with the later sections.

Commit ba72b4c8cf60 made -EEXIST a hard error because
sparse_add_section() began using it to report an actual subsection
collision. That semantic change was correct, but without rolling back
the sections added earlier in the request, stale subsection bits make the
retry stop at the first previously added section.

The vmemmap removal infrastructure had already been added by commit
0197518cd367 ("memory-hotplug: remove memmap of sparse-vmemmap").
Therefore, ba72b4c8cf60 appears to be the commit that made this bug
observable in the way described by this patch.

> 
> mm/memory_hotplug.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 796af1028ee2..ca4656698148 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -380,6 +380,7 @@ EXPORT_SYMBOL_GPL(pfn_to_online_page);
> int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> struct mhp_params *params)
> {
> + 	const unsigned long start_pfn = pfn;
> 	const unsigned long end_pfn = pfn + nr_pages;
> 	unsigned long cur_nr_pages;
> int err;
> @@ -413,8 +414,11 @@ int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>   SECTION_ALIGN_UP(pfn + 1) - pfn);
> 		err = sparse_add_section(nid, pfn, cur_nr_pages, altmap,
> 					params->pgmap);
> - 		if (err)
> + 		if (err) {
> + 			__remove_pages(start_pfn, pfn - start_pfn, altmap,
> +       				params->pgmap);
> 			break;
> + 		}
> 		cond_resched();
> 	}
> 	vmemmap_populate_print_last();

Acked-by: Muchun Song <muchun.song@linux.dev>

Thanks,
Muchun

> -- 
> 2.39.3
> 

Re: [PATCH 1/1] mm/memory_hotplug: fix missing rollback in __add_pages()

[Muchun Song]

> On Oct 1, 2026, at 00:46, David Hildenbrand (Arm) <david@kernel.org> wrote:
> 
> On 9/30/26 18:04, Lance Yang wrote:
>> __add_pages() returns on a sparse_add_section() failure without removing
>> the sections already added in the same request.
>> 
>> For memremap_pages(), the failed range is not counted in pgmap->nr_range,
>> so memunmap_pages() skips it. The sections already added in that range
>> retain their vmemmap mappings and subsection bits. Retrying a
>> section-aligned range can then fail with -EEXIST.
>> 
>> Save the initial PFN and remove [start_pfn, pfn) on failure. For a vmemmap
>> population failure, section_activate() already cleans up the current
>> section, so the rollback excludes it. If the first section fails,
>> __remove_pages() receives an empty range and does nothing.
>> 
>> Link: https://lore.kernel.org/all/BAD58999-1EDD-4A37-ABA0-DB1BD8AB3453@linux.dev/
>> Suggested-by: Muchun Song <muchun.song@linux.dev>
>> Signed-off-by: Lance Yang <lance.yang@linux.dev>
>> ---
>> No Fixes tag, as I couldn't identify the commit that introduced this issue.
>> 
>> mm/memory_hotplug.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 796af1028ee2..ca4656698148 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -380,6 +380,7 @@ EXPORT_SYMBOL_GPL(pfn_to_online_page);
>> int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>> struct mhp_params *params)
>> {
>> + 	const unsigned long start_pfn = pfn;
>> 	const unsigned long end_pfn = pfn + nr_pages;
>> 	unsigned long cur_nr_pages;
>> 	int err;
>> @@ -413,8 +414,11 @@ int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>>    SECTION_ALIGN_UP(pfn + 1) - pfn);
>> 		err = sparse_add_section(nid, pfn, cur_nr_pages, altmap,
>>  					params->pgmap);
>> - 		if (err)
>> + 		if (err) {
>> + 			__remove_pages(start_pfn, pfn - start_pfn, altmap,
>> + 				       params->pgmap);
>> 			break;
>> + 		}
>> 		cond_resched();
>> 	}
>> 	vmemmap_populate_print_last();
> 
> Makes sense and LGTM.
> 
> Do we have a Fixes: tag? It probably dates back quite a while ... not sure about
> stable, we never saw this in practice. But if it's easy, we should just do it?
> (not sure if we ever had __remove_pages be limited to hotunplug support)
> 
> I'm planning on picking this up and sending it for the next merge window (so not
> as a hotfix).
> 
> Looking at this ...
> 
> x86 does not really expect add_pages to fail:
> 
> ret = __add_pages(nid, start_pfn, nr_pages, params);
> WARN_ON_ONCE(ret);
> 
> That's probably something to clean up as well?

The warning is a historical leftover. The original code
printed an error when __add_pages() failed. Commit
10f22dde556d accidentally turned that conditional printk
into an unconditional WARN_ON(1), and commit fe8b868eccb9
subsequently changed it to WARN_ON_ONCE(ret) to avoid
warning on successful memory hot-add.

There is no no-failure contract here: __add_pages() can
legitimately return errors such as -ENOMEM, and those
errors are already propagated to the caller. Since
WARN_ON_ONCE(ret) has no effect on control flow or error
handling, it can be safely removed without changing the
failure semantics.

This also reveals a potential bug introduced by commit
ea0854170c952: when update_end_of_memory_vars() was
added, it was called without checking that ret == 0, so the
end-of-memory variables may be updated even when
__add_pages() fails. Returning immediately on error fixes
that as well.

Thanks,
Muchun

> 
> -- 
> Cheers,
> 
> David