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

[Muchun Song <muchun.song@linux.dev>]

> On Apr 23, 2026, at 20:28, David Hildenbrand (Arm) <david@kernel.org> wrote:
> 
> 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?

No problem.

> 
> -- 
> Cheers,
> 
> David