Re: [PATCH v5 v5 2/6] mm/memory_hotplug: Fix incorrect altmap passing in error path

David Hildenbrand (Arm)
2026-04-23T12:28:50Z

On 4/23/26 14:18, Muchun Song wrote:
>
>
>> On Apr 23, 2026, at 18:38, David Hildenbrand (Arm) <david@kernel.org> wrote:
>>
>> On 4/23/26 09:19, Muchun Song wrote:
>>> In create_altmaps_and_memory_blocks(), when arch_add_memory() succeeds
>>> with memmap_on_memory enabled, the vmemmap pages are allocated from
>>> params.altmap. If create_memory_block_devices() subsequently fails, the
>>> error path calls arch_remove_memory() with a NULL altmap instead of
>>> params.altmap.
>>>
>>> This is a bug that could lead to memory corruption. Since altmap is
>>> NULL, vmemmap_free() falls back to freeing the vmemmap pages into the
>>> system buddy allocator via free_pages() instead of the altmap.
>>> arch_remove_memory() then immediately destroys the physical linear
>>> mapping for this memory. This injects unowned pages into the buddy
>>> allocator, causing machine checks or memory corruption if the system
>>> later attempts to allocate and use those freed pages.
>>>
>>> Fix this by passing params.altmap to arch_remove_memory() in the error
>>> path.
>>>
>>> Fixes: 6b8f0798b85a ("mm/memory_hotplug: split memmap_on_memory requests across memblocks")
>>> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>>> ---
>>> mm/memory_hotplug.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 2a943ec57c85..0bad2aed2bde 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1468,7 +1468,7 @@ static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
>>> ret = create_memory_block_devices(cur_start, memblock_size, nid,
>>> params.altmap, group);
>>> if (ret) {
>>> - arch_remove_memory(cur_start, memblock_size, NULL);
>>> + arch_remove_memory(cur_start, memblock_size, params.altmap);
>>> kfree(params.altmap);
>>> goto out;
>>> }
>>
>> Yeah, that's nasty. We should CC stable.
>
> Make sense.
>
>>
>> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>
> Thanks.
>
>>
>>
>>
>> Should we extend the safety checks we already have on the other path?
>
> Better to have.
>
>>
>>
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 2a943ec57c85..1c304468af08 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1402,6 +1402,12 @@ bool mhp_supports_memmap_on_memory(void)
>> }
>> EXPORT_SYMBOL_GPL(mhp_supports_memmap_on_memory);
>>
>> +static void altmap_free(struct vmemmap_altmap *altmap)
>> +{
>> + WARN(altmap->alloc, "Altmap not fully unmapped");
>
> Should we change it to WARN_ONCE?

Was debating with myself, and yes, I think so.

>
>> + kfree(altmap);
>> +}
>> +
>> static void remove_memory_blocks_and_altmaps(u64 start, u64 size)
>> {
>> unsigned long memblock_size = memory_block_size_bytes();
>> @@ -1426,10 +1432,7 @@ static void remove_memory_blocks_and_altmaps(u64 start, u64 size)
>> remove_memory_block_devices(cur_start, memblock_size);
>>
>> arch_remove_memory(cur_start, memblock_size, altmap);
>> -
>> - /* Verify that all vmemmap pages have actually been freed. */
>> - WARN(altmap->alloc, "Altmap not fully unmapped");
>> - kfree(altmap);
>> + altmap_free(altmap);
>> }
>> }
>>
>> @@ -1460,7 +1463,7 @@ static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
>> /* call arch's memory hotadd */
>> ret = arch_add_memory(nid, cur_start, memblock_size, &params);
>> if (ret < 0) {
>> - kfree(params.altmap);
>> + altmap_free(params.altmap);
>> goto out;
>> }
>>
>> @@ -1469,13 +1472,12 @@ static int create_altmaps_and_memory_blocks(int nid, struct memory_group *group,
>> params.altmap, group);
>> if (ret) {
>> arch_remove_memory(cur_start, memblock_size, NULL);
>> - kfree(params.altmap);
>> + altmap_free(params.altmap);
>> goto out;
>> }
>> }
>>
>> return 0;
>> -out:
>> if (ret && cur_start != start)
>> remove_memory_blocks_and_altmaps(start, cur_start - start);
>> return ret;
>>
>>
>> Maybe the helper should even go into altmap code? Not sure.
>
> I think the current changes look great as they are. While I believe this is valuable
> as a standalone cleanup, what do you think?
Makes sense. Could you do me the favor and follow up with that, on top of the fixes?

--
Cheers,

David