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

["David Hildenbrand (Arm)" <david@kernel.org>]

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.

Acked-by: David Hildenbrand (Arm) <david@kernel.org>



Should we extend the safety checks we already have on the other path?


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");
+       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.


-- 
Cheers,

David