Skip to contentSkip to conversation

mm/memory_hotplug: fix missing rollback in __add_pages()

PATCH Applied Lance Yang 3 participants 8 messages Sep 30 – Oct 1

Subsystems (2)
Memory management Memory hot(un)plug
lore
Share & tools
lore

Lance Yang

author
#0 lore raw

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

__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/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

---
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(-)
mm/memory_hotplug.c +5 -1
index 796af1028ee2..ca4656698148 100644
@@ -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();
Signature · 1 line
2.39.3
maintainer
#1 lore raw
Quoted 37 lines · 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/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

---
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(-)
mm/memory_hotplug.c +1 -0
index 796af1028ee2..ca4656698148 100644
@@ -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,

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 10 lines
mm/memory_hotplug.c +4 -1
 					 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?

Signature · 3 lines
Cheers,

David
#2 lore raw
Quoted 62 lines · David Hildenbrand (Arm)
Quoted 37 lines · 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/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

---
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(-)
mm/memory_hotplug.c +1 -0
index 796af1028ee2..ca4656698148 100644
@@ -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,

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 10 lines
mm/memory_hotplug.c +4 -1
 					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).

Quoted 8 lines

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

Quoted 5 lines
Signature · 4 lines
-- 
Cheers,

David
maintainer
#3 lore raw
Quoted 78 lines · Muchun Song
Quoted 62 lines · David Hildenbrand (Arm)
Quoted 37 lines · 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/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

---
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(-)
mm/memory_hotplug.c +1 -0
index 796af1028ee2..ca4656698148 100644
@@ -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,

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 10 lines
mm/memory_hotplug.c +4 -1
 					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).

Quoted 8 lines

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.

Quoted 6 lines

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.

We do not want to WARN_ON in any situation that can legitimately happen. So this should be sorted out (and Lance is on it IIUC :) )

Quoted 7 lines

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.

I'd assume the end-of-memory variables are not a problem, as we usually do not adjust them during memory unplug either. But yeah, ideally we wouldn't update them (like other architectures do IIRC).

Signature · 3 lines
Cheers,

David
author
#4 lore raw
In reply to David Hildenbrand (Arm)
Quoted 51 lines · David Hildenbrand (Arm)
Quoted 37 lines · 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/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

---
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(-)
mm/memory_hotplug.c +1 -0
index 796af1028ee2..ca4656698148 100644
@@ -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,

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 10 lines
mm/memory_hotplug.c +4 -1
  					 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.

Quoted 3 lines

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)

Hmm ... I'd leave stable out for now. David, could you add the Fixes tag Muchun suggested?

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

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

Thank you ;)

Quoted 8 lines

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?

That would be a separate fix, I guess :) I'll have a look at the other architectures while I'm at it :)

Cheers, Lance

Acked-by on v1 1/1
#5 lore raw
In reply to Lance Yang
Quoted 17 lines · 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.

Quoted 5 lines
Link https://lore.kernel.org/all/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

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

Quoted 19 lines

mm/memory_hotplug.c | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)
mm/memory_hotplug.c +1 -0
index 796af1028ee2..ca4656698148 100644
@@ -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,

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 10 lines
mm/memory_hotplug.c +4 -1
 					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 <[email protected]>

Thanks,
Muchun

Quoted 3 lines
Signature · 1 line
2.39.3
author
#6 lore raw
Quoted 41 lines · Muchun Song
Quoted 17 lines · 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.

Quoted 5 lines
Link https://lore.kernel.org/all/[email protected]/ open here
Suggested-by Muchun Song <[email protected]>
Signed-off-by Lance Yang <[email protected]>

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

Quoted 4 lines

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.

Thanks, Muchun!

Quoted 23 lines
Quoted 23 lines

mm/memory_hotplug.c | 6 +++++-

1 file changed, 5 insertions(+), 1 deletion(-)
mm/memory_hotplug.c +3 -1
index 796af1028ee2..ca4656698148 100644
@@ -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,
Quoted 8 lines

mm/memory_hotplug.c int __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, · line 424

Quoted 6 lines
mm/memory_hotplug.c +2 -0
+       				params->pgmap);
 			break;
+ 		}
 		cond_resched();
 	}
 	vmemmap_populate_print_last();
Acked-by Muchun Song <[email protected]>

Cheers! Lance

maintainer Applied
#7 lore raw
In reply to Lance Yang
Quoted 9 lines · 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.

[...]

Applied, thanks!

tree: mm/core.git
branch: memhp-7.4.misc

Applied patches will appear soon in mm-next, followed by linux-next.

Please report any outstanding bugs that were missed during review, so we can either drop the patch series or decide how to fix it up.

It is encouraged to provide Acked-bys, Reviewed-bys, and Tested-bys even though the patches have already been applied. If possible, patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. Further, this is not a guarantee that the patches will go upstream. If in doubt, please check the listed branch.

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

      https://git.kernel.org/mm/core/c/c31bb839
Signature · 3 lines
Cheers,

David