Re: [PATCH] arm64: mm: call pagetable dtor when freeing hot-removed page tables

[Vishal Moola <vishal.moola@gmail.com>]

On Fri, May 22, 2026 at 08:15:09AM +0100, Catalin Marinas wrote:
> On Thu, May 21, 2026 at 03:31:30PM -0700, Andrew Morton wrote:
> > On Thu, 21 May 2026 13:27:30 +1000 Alistair Popple <apopple@nvidia.com> wrote:
> > > Since 5e8eb9aeeda3 ("arm64: mm: always call PTE/PMD ctor in
> > > __create_pgd_mapping()") page-table allocation on ARM64 always
> > > calls pagetable_{pte,pmd,pud,p4d}_ctor(). This sets the page_type
> > > to PGTY_table, increments NR_PAGETABLE and possible allocates a PTL.
> > > However the matching pagetable_dtor() calls were never added.
> > > 
> > > With DEBUG_VM enabled on kernel versions prior to v6.17 without
> > > 2dfcd1608f3a9 ("mm/page_alloc: let page freeing clear any set page
> > > type") this leads to the following warning when freeing these pages due
> > > to page->page_type sharing page->_mapcount:
> > > 
> > >   BUG: Bad page state in process ... pfn:284fbb
> > >   page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x284fbb
> > >   flags: 0x17fffc000000000(node=0|zone=2|lastcpupid=0x1ffff)
> > >   page_type: f2(table)
> > >   page dumped because: nonzero mapcount
> > >   Call trace:
> > >    bad_page+0x13c/0x160
> > >    __free_frozen_pages+0x6cc/0x860
> > >    ___free_pages+0xf4/0x180
> > >    free_pages+0x54/0x80
> > >    free_hotplug_page_range.part.0+0x58/0x90
> > >    free_empty_tables+0x438/0x500
> > >    __remove_pgd_mapping.constprop.0+0x60/0xa8
> > >    arch_remove_memory+0x48/0x80
> > >    try_remove_memory+0x158/0x1d8
> > >    offline_and_remove_memory+0x138/0x180
> > > 
> > > It can also lead to leaking the ptl allocation if ALLOC_SPLIT_PTLOCKS
> > > is defined and incorrect NR_PAGETABLE stats. Fix this by calling
> > > pagetable_dtor() in free_hotplug_pgtable_page() prior to freeing the
> > > page to undo the effects of calling pagetable_*_ctor().
> > > 
> > > Fixes: 5e8eb9aeeda3 ("arm64: mm: always call PTE/PMD ctor in __create_pgd_mapping()")
> > 
> > 6.16+, so I assume we want cc:stable here.
> > 
> > >  arch/arm64/mm/mmu.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > > index 8e1d80a7033e..0c24fe650e95 100644
> > > --- a/arch/arm64/mm/mmu.c
> > > +++ b/arch/arm64/mm/mmu.c
> > > @@ -1422,6 +1422,7 @@ static void free_hotplug_page_range(struct page *page, size_t size,
> > >  
> > >  static void free_hotplug_pgtable_page(struct page *page)
> > >  {
> > > +	pagetable_dtor(page_ptdesc(page));
> > >  	free_hotplug_page_range(page, PAGE_SIZE, NULL);
> > >  }
> > 
> > I'd of course prefer that arm maintainers handle this.  But
> > 5e8eb9aeeda3 came via myself so convention kinda-dictates that I get to
> > fix it.
> 
> That's fine but Sashiko has some points:
> 
> https://sashiko.dev/#/patchset/20260521032730.2104017-1-apopple@nvidia.com
> 
> The __remove_pgd_mapping() path is fine but we also have the
> vmemmap_free() path where the constructor was never called.
> 
> We could pass around a bool dtor argument but I wonder whether we could
> just check it's a pgtable page:

Free_empty_tables() looks like the only way we'd ever get to
free_hotplug_pgtable_page(). I'm a little curious why we can't
consolidate unmap_hotplug_range() and free_empty_tables().
I.e. just fold unmap_hotplug_range() into the latter.

> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 4c8959153ac4..9d42cbddce27 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1441,6 +1441,9 @@ static void free_hotplug_page_range(struct page *page, size_t size,
>  
>  static void free_hotplug_pgtable_page(struct page *page)
>  {
> +	if (folio_test_pgtable(page_folio(page)))

This should work.

> +		pagetable_dtor(page_ptdesc(page));
> +
>  	free_hotplug_page_range(page, PAGE_SIZE, NULL);

In the case we presumably have a page table page (ptdesc) at this
point, we should really be freeing it with pagetable_free() as well.

Its not a big deal that we don't right now, but losing track of the
matching allocation/free sites will become a headache when separately
allocating from struct page.

>  }
>  
> 
> -- 
> Catalin