| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e10d0c8f-c86b-4e26-8006-f38734993796@arm.com>
Date: Tue, 10 Jun 2025 20:00:40 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Dev Jain <dev.jain@....com>, catalin.marinas@....com, will@...nel.org
Cc: anshuman.khandual@....com, quic_zhenhuah@...cinc.com,
kevin.brodsky@....com, yangyicong@...ilicon.com, joey.gouly@....com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
david@...hat.com
Subject: Re: [PATCH v2] arm64: Enable vmalloc-huge with ptdump
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@....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;
Powered by blists - more mailing lists