Re: [PATCH] x86/mm: fix freeing of PMD-sized vmemmap pages

[Lance Yang]

On Tue, Apr 28, 2026 at 12:29:36PM +0200, David Hildenbrand (Arm) wrote:
>In commit bf9e4e30f353 ("x86/mm: use pagetable_free()"), we switched
>from freeing non-boot page tables through __free_pages() to
>pagetable_free().
>
>However, the function is also called to free vmemmap pages.
>
>Given that vmemmap pages are not page tables, already the page_ptdesc(page)
>is wrong. But worse, pagetable_free() calls
>
>	__free_pages(page, compound_order(page));
>
>As vmemmap pages are not compound pages (see vmemmap_alloc_block()) --
>except for HVO, which doesn't apply here -- we will only free the first
>page when freeing a PMD-sized vmemmap page, leaking the other ones.
>
>Fix it by properly decoupling pagetable and vmemmap freeing.
>free_pagetable() no longer has to mess with SECTION_INFO, as only the
>vmemmap is marked like that in register_page_bootmem_memmap().
>
>While at it, just wire up the altmap parameter for remove_pte_table().
>Also, the indentation in remove_pmd_table() is messed up, let's fix that
>while touching it.

One thing I'm not sure about is passing altmap down into
remove_pte_table().

Do we actually know that a non-NULL altmap means that the vmemmap
backing page came from that altmap?

On x86 we still have in vmemmap_populate():

	if (end - start < PAGES_PER_SECTION * sizeof(struct page))
		err = vmemmap_populate_basepages(start, end, node, NULL);

So for smaller-than-section vmemmap ranges, even if the caller has an
altmap, the backing pages are allocated from normal memory. But with
this fix the PTE removal path would now call vmem_altmap_free() just
because altmap is non-NULL, and would not free the actual backing page,
IIUC :)

Maybe free_vmemmap_pages() should first check that the backing page is
really inside the altmap range before using vmem_altmap_free()?

Hopefully I didn't miss anything :)

Thanks,
Lance

>Note that we'll try to get rid of that bootmem info handling soon. For
>now, we'll handle it similar to free_pagetable(), just avoiding the
>ifdef.
>
>Fixes: bf9e4e30f353 ("x86/mm: use pagetable_free()")
>Cc: stable@xxxxxxxxxxxxxxx
>Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>---
>Reproduced and tested with a simple VM with a virtio-mem device,
>repeatedly adding and removing memory.
>
>Found by code inspection while working on bootmem_info removal.
>---
> arch/x86/mm/init_64.c | 43 +++++++++++++++++++++++++++----------------
> 1 file changed, 27 insertions(+), 16 deletions(-)
>
>diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>index df2261fa4f98..8d03e44a7fb9 100644
>--- a/arch/x86/mm/init_64.c
>+++ b/arch/x86/mm/init_64.c
>@@ -1014,7 +1014,7 @@ static void __meminit free_pagetable(struct page *page, int order)
> #ifdef CONFIG_HAVE_BOOTMEM_INFO_NODE
> 		enum bootmem_type type = bootmem_type(page);
> 
>-		if (type == SECTION_INFO || type == MIX_SECTION_INFO) {
>+		if (type == MIX_SECTION_INFO) {
> 			while (nr_pages--)
> 				put_page_bootmem(page++);
> 		} else {
>@@ -1028,13 +1028,24 @@ static void __meminit free_pagetable(struct page *page, int order)
> 	}
> }
> 
>-static void __meminit free_hugepage_table(struct page *page,
>+static void __meminit free_vmemmap_pages(struct page *page, unsigned int order,
> 		struct vmem_altmap *altmap)
> {
>-	if (altmap)
>-		vmem_altmap_free(altmap, PMD_SIZE / PAGE_SIZE);
>-	else
>-		free_pagetable(page, get_order(PMD_SIZE));
>+	if (altmap) {
>+		vmem_altmap_free(altmap, 1u << order);
>+	} else if (PageReserved(page)) {
>+		unsigned long nr_pages = 1 << order;
>+
>+		if (IS_ENABLED(CONFIG_HAVE_BOOTMEM_INFO_NODE) &&
>+		    bootmem_type(page) == SECTION_INFO) {
>+			while (nr_pages--)
>+				put_page_bootmem(page++);
>+		} else {
>+			free_reserved_pages(page, nr_pages);
>+		}
>+	} else {
>+		__free_pages(page, order);
>+	}
> }
> 
> static void __meminit free_pte_table(pte_t *pte_start, pmd_t *pmd)
>@@ -1093,7 +1104,7 @@ static void __meminit free_pud_table(pud_t *pud_start, p4d_t *p4d)
> 
> static void __meminit
> remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end,
>-		 bool direct)
>+		 bool direct, struct vmem_altmap *altmap)
> {
> 	unsigned long next, pages = 0;
> 	pte_t *pte;
>@@ -1118,7 +1129,7 @@ remove_pte_table(pte_t *pte_start, unsigned long addr, unsigned long end,
> 			return;
> 
> 		if (!direct)
>-			free_pagetable(pte_page(*pte), 0);
>+			free_vmemmap_pages(pte_page(*pte), 0, altmap);
> 
> 		spin_lock(&init_mm.page_table_lock);
> 		pte_clear(&init_mm, addr, pte);
>@@ -1153,25 +1164,25 @@ remove_pmd_table(pmd_t *pmd_start, unsigned long addr, unsigned long end,
> 			if (IS_ALIGNED(addr, PMD_SIZE) &&
> 			    IS_ALIGNED(next, PMD_SIZE)) {
> 				if (!direct)
>-					free_hugepage_table(pmd_page(*pmd),
>-							    altmap);
>+					free_vmemmap_pages(pmd_page(*pmd),
>+							   PMD_ORDER, altmap);
> 
> 				spin_lock(&init_mm.page_table_lock);
> 				pmd_clear(pmd);
> 				spin_unlock(&init_mm.page_table_lock);
> 				pages++;
> 			} else if (vmemmap_pmd_is_unused(addr, next)) {
>-					free_hugepage_table(pmd_page(*pmd),
>-							    altmap);
>-					spin_lock(&init_mm.page_table_lock);
>-					pmd_clear(pmd);
>-					spin_unlock(&init_mm.page_table_lock);
>+				free_vmemmap_pages(pmd_page(*pmd), PMD_ORDER,
>+						   altmap);
>+				spin_lock(&init_mm.page_table_lock);
>+				pmd_clear(pmd);
>+				spin_unlock(&init_mm.page_table_lock);
> 			}
> 			continue;
> 		}
> 
> 		pte_base = (pte_t *)pmd_page_vaddr(*pmd);
>-		remove_pte_table(pte_base, addr, next, direct);
>+		remove_pte_table(pte_base, addr, next, direct, altmap);
> 		free_pte_table(pte_base, pmd);
> 	}
> 
>
>---
>
>base-commit: a2ddbfd1af0f54ea84bf17f0400088815d012e8d
>
>change-id: 20260428-vmemmap-ab4b949aa727
>
>--
>
>Cheers,
>
>David
>
>