Re: [PATCH v2] arm64: Enable vmalloc-huge with ptdump

[Ryan Roberts <ryan.roberts@arm.com>]

On 10/06/2025 17:00, Dev Jain wrote:
> arm64 disables vmalloc-huge when kernel page table dumping is enabled,
> because an intermediate table may be removed, potentially causing the
> ptdump code to dereference an invalid address. We want to be able to
> analyze block vs page mappings for kernel mappings with ptdump, so to
> enable vmalloc-huge with ptdump, synchronize between page table removal in
> pmd_free_pte_page()/pud_free_pmd_page() and ptdump pagetable walking. We
> use mmap_read_lock and not write lock because we don't need to synchronize
> between two different vm_structs; two vmalloc objects running this same
> code path will point to different page tables, hence there is no race.
> 
> For pud_free_pmd_page(), we isolate the PMD table to avoid taking the lock
> 512 times again via pmd_free_pte_page(). Note that there is no need to
> move __flush_tlb_kernel_pgtable() to immediately after pud_clear(); the
> only argument against this would be that we immediately require a
> dsb(ishst) (present in __flush_tlb_kernel_pgtable()) after pud_clear(),
> but that is not the case, since the transition is from
> valid -> invalid, not vice-versa.
> 
> No issues were observed with mm-selftests. No issues were observed while
> parallelly running test_vmalloc.sh and dumping the kernel pagetable through
> sysfs in a loop.
> 
> v1->v2:
>  - Take lock only when CONFIG_PTDUMP_DEBUGFS is on

I thought we agreed that we would use a static key and some rcu synchronize
magic? What was the reason for taking this approach?

I'm guessing CONFIG_PTDUMP_DEBUGFS is very much a debug feature that we wouldn't
expect to enable in production kernels; if that's the case, then perhaps this
approach is good enough. But given Will suggested a solution that would make it
zero overhead when ptdump is not active, why not just take that approach?

Thanks,
Ryan

>  - In case of pud_free_pmd_page(), isolate the PMD table to avoid taking
>    the lock 512 times again via pmd_free_pte_page()
> 
> Signed-off-by: Dev Jain <dev.jain@arm.com>
> ---
>  arch/arm64/include/asm/vmalloc.h |  6 ++---
>  arch/arm64/mm/mmu.c              | 43 +++++++++++++++++++++++++++++---
>  2 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/vmalloc.h b/arch/arm64/include/asm/vmalloc.h
> index 12f534e8f3ed..e835fd437ae0 100644
> --- a/arch/arm64/include/asm/vmalloc.h
> +++ b/arch/arm64/include/asm/vmalloc.h
> @@ -12,15 +12,13 @@ static inline bool arch_vmap_pud_supported(pgprot_t prot)
>  	/*
>  	 * SW table walks can't handle removal of intermediate entries.
>  	 */
> -	return pud_sect_supported() &&
> -	       !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
> +	return pud_sect_supported();
>  }
>  
>  #define arch_vmap_pmd_supported arch_vmap_pmd_supported
>  static inline bool arch_vmap_pmd_supported(pgprot_t prot)
>  {
> -	/* See arch_vmap_pud_supported() */
> -	return !IS_ENABLED(CONFIG_PTDUMP_DEBUGFS);
> +	return true;
>  }
>  
>  #define arch_vmap_pte_range_map_size arch_vmap_pte_range_map_size
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 8fcf59ba39db..fa98a62e4baf 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1267,7 +1267,25 @@ int pmd_clear_huge(pmd_t *pmdp)
>  	return 1;
>  }
>  
> -int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
> +#ifdef CONFIG_PTDUMP_DEBUGFS
> +static inline void ptdump_synchronize_lock(void)
> +{
> +	/* Synchronize against ptdump_walk_pgd() */
> +	mmap_read_lock(&init_mm);
> +}
> +
> +static inline void ptdump_synchronize_unlock(void)
> +{
> +	mmap_read_unlock(&init_mm);
> +}
> +#else	/* CONFIG_PTDUMP_DEBUGFS */
> +
> +static inline void ptdump_synchronize_lock(void)	{}
> +static inline void ptdump_synchronize_unlock(void)	{}
> +
> +#endif	/* CONFIG_PTDUMP_DEBUGFS */
> +
> +static int __pmd_free_pte_page(pmd_t *pmdp, unsigned long addr, bool lock)
>  {
>  	pte_t *table;
>  	pmd_t pmd;
> @@ -1280,12 +1298,23 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  	}
>  
>  	table = pte_offset_kernel(pmdp, addr);
> +
> +	if (lock)
> +		ptdump_synchronize_lock();
>  	pmd_clear(pmdp);
> +	if (lock)
> +		ptdump_synchronize_unlock();
> +
>  	__flush_tlb_kernel_pgtable(addr);
>  	pte_free_kernel(NULL, table);
>  	return 1;
>  }
>  
> +int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
> +{
> +	return __pmd_free_pte_page(pmdp, addr, true);
> +}
> +
>  int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  {
>  	pmd_t *table;
> @@ -1301,14 +1330,22 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  	}
>  
>  	table = pmd_offset(pudp, addr);
> +
> +	/*
> +	 * Isolate the PMD table; in case of race with ptdump, this helps
> +	 * us to avoid taking the lock in __pmd_free_pte_page()
> +	 */
> +	ptdump_synchronize_lock();
> +	pud_clear(pudp);
> +	ptdump_synchronize_unlock();
> +
>  	pmdp = table;
>  	next = addr;
>  	end = addr + PUD_SIZE;
>  	do {
> -		pmd_free_pte_page(pmdp, next);
> +		__pmd_free_pte_page(pmdp, next, false);
>  	} while (pmdp++, next += PMD_SIZE, next != end);
>  
> -	pud_clear(pudp);
>  	__flush_tlb_kernel_pgtable(addr);
>  	pmd_free(NULL, table);
>  	return 1;