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

[Lance Yang <lance.yang@linux.dev>]

On 2026/10/1 00:46, David Hildenbrand (Arm) 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)

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

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

Thank you ;)

> 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