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

[David Hildenbrand (Arm)]

On 10/1/26 04:33, Muchun Song wrote:
> 
> 
>> On Oct 1, 2026, at 00:46, David Hildenbrand (Arm) <david@xxxxxxxxxx> 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@xxxxxxxxx/
>>> Suggested-by: Muchun Song <muchun.song@xxxxxxxxx>
>>> Signed-off-by: Lance Yang <lance.yang@xxxxxxxxx>
>>> ---
>>> 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.

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 :) )

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

-- 
Cheers,

David