From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A4FE5CA101F for ; Wed, 10 Sep 2025 14:33:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=i1wHhJfpnhs7p5z4M93u+XI9aTOl4p9mbiFPI1n+AKo=; b=kRagbR6WpaCHIP7zetfUl2a2M2 kbvwNiepOBb3PO93zVp2T1kPwebVUYwkobQaoS37p3rF+MNKne5nWPNXbhjySTVPCDKx3tHFptMw5 expAdC0wH31whMHoR9HRsdndAkdwyEsGr9EcbKc0850prD37DJlvWTdj0/rShcg+O6HHZQtnCt1Dx N6SvyQpJdk7qQo6f0qsGQ1BuaEsmDthHADZPEybrdI0hAZxB8jVyuQfP+GobySOW5mrf7PS/gAecd SaPexb29ZIBP/VuDZUSpwoIjs9sV65R1HQklJrNacmavIIt85BKC+H/STTu43ttRNyb2hV6ZSV3aL if8CHSAA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uwLt6-0000000Ek0e-3zuD; Wed, 10 Sep 2025 14:33:32 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uwLt4-0000000Ejyw-0gRp for linux-arm-kernel@lists.infradead.org; Wed, 10 Sep 2025 14:33:31 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 15D3F16F2; Wed, 10 Sep 2025 07:33:21 -0700 (PDT) Received: from [10.57.90.208] (unknown [10.57.90.208]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 6B6F53F694; Wed, 10 Sep 2025 07:33:27 -0700 (PDT) Message-ID: <6fd9df06-7120-4eef-9f02-70ba49266c75@arm.com> Date: Wed, 10 Sep 2025 15:33:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] arm64: mm: Move KPTI helpers to mmu.c Content-Language: en-GB To: Kevin Brodsky , linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org, Anshuman Khandual , Ard Biesheuvel , Catalin Marinas , Kees Cook , Mark Rutland , Suzuki K Poulose , Will Deacon , Yeoreum Yun References: <20250910104454.317067-1-kevin.brodsky@arm.com> From: Ryan Roberts In-Reply-To: <20250910104454.317067-1-kevin.brodsky@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250910_073330_286690_6691DE9E X-CRM114-Status: GOOD ( 38.85 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 10/09/2025 11:44, Kevin Brodsky wrote: > create_kpti_ng_temp_pgd() is currently defined (as an alias) in > mmu.c without matching declaration in a header; instead cpufeature.c > makes its own declaration. This is clearly not pretty, and as commit > ceca927c86e6 ("arm64: mm: Fix CFI failure due to kpti_ng_pgd_alloc > function signature") showed, it also makes it very easy for the > prototypes to go out of sync. > > All this would be much simpler if kpti_install_ng_mappings() and > associated functions lived in mmu.c, where they logically belong. > This is what this patch does: > - Move kpti_install_ng_mappings() and associated functions from > cpufeature.c to mmu.c, add a declaration to > - Make create_kpti_ng_temp_pgd() a static function that simply calls > __create_pgd_mapping_locked() instead of aliasing it > - Mark all these functions __init > - Move __initdata after kpti_ng_temp_alloc (as suggested by > checkpatch) This is a great clean up IMHO; that alias has caught me out a few times in the past when hacking in this area. And this code clearly belongs in mmu.c. > > Signed-off-by: Kevin Brodsky > --- > Note: as things stand, create_kpti_ng_temp_pgd() could be removed, > but a separate patch [1] will make use of it to add an > assertion. I'd vote for removing it and just calling __create_pgd_mapping_locked() direct. The next version of the other patch can just rebase on top of yours and add the assert in __kpti_install_ng_mappings(). Either way: Reviewed-by: Ryan Roberts > > [1] https://lore.kernel.org/all/20250813145607.1612234-3-chaitanyas.prakash@arm.com/ > --- > Cc: Anshuman Khandual > Cc: Ard Biesheuvel > Cc: Catalin Marinas > Cc: Kees Cook , > Cc: Mark Rutland > Cc: Ryan Roberts > Cc: Suzuki K Poulose > Cc: Will Deacon > Cc: Yeoreum Yun > --- > arch/arm64/include/asm/mmu.h | 6 ++ > arch/arm64/kernel/cpufeature.c | 97 ------------------------------ > arch/arm64/mm/mmu.c | 106 ++++++++++++++++++++++++++++++--- > 3 files changed, 103 insertions(+), 106 deletions(-) > > diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h > index 49f1a810df16..624edd6c4964 100644 > --- a/arch/arm64/include/asm/mmu.h > +++ b/arch/arm64/include/asm/mmu.h > @@ -104,5 +104,11 @@ static inline bool kaslr_requires_kpti(void) > return true; > } > > +#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > +void kpti_install_ng_mappings(void); > +#else > +static inline void kpti_install_ng_mappings(void) {} > +#endif > + > #endif /* !__ASSEMBLY__ */ > #endif > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index ef269a5a37e1..b99eaad48c14 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -1940,103 +1940,6 @@ static bool has_pmuv3(const struct arm64_cpu_capabilities *entry, int scope) > } > #endif > > -#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > -#define KPTI_NG_TEMP_VA (-(1UL << PMD_SHIFT)) > - > -extern > -void create_kpti_ng_temp_pgd(pgd_t *pgdir, phys_addr_t phys, unsigned long virt, > - phys_addr_t size, pgprot_t prot, > - phys_addr_t (*pgtable_alloc)(enum pgtable_type), int flags); > - > -static phys_addr_t __initdata kpti_ng_temp_alloc; > - > -static phys_addr_t __init kpti_ng_pgd_alloc(enum pgtable_type type) > -{ > - kpti_ng_temp_alloc -= PAGE_SIZE; > - return kpti_ng_temp_alloc; > -} > - > -static int __init __kpti_install_ng_mappings(void *__unused) > -{ > - typedef void (kpti_remap_fn)(int, int, phys_addr_t, unsigned long); > - extern kpti_remap_fn idmap_kpti_install_ng_mappings; > - kpti_remap_fn *remap_fn; > - > - int cpu = smp_processor_id(); > - int levels = CONFIG_PGTABLE_LEVELS; > - int order = order_base_2(levels); > - u64 kpti_ng_temp_pgd_pa = 0; > - pgd_t *kpti_ng_temp_pgd; > - u64 alloc = 0; > - > - if (levels == 5 && !pgtable_l5_enabled()) > - levels = 4; > - else if (levels == 4 && !pgtable_l4_enabled()) > - levels = 3; > - > - remap_fn = (void *)__pa_symbol(idmap_kpti_install_ng_mappings); > - > - if (!cpu) { > - alloc = __get_free_pages(GFP_ATOMIC | __GFP_ZERO, order); > - kpti_ng_temp_pgd = (pgd_t *)(alloc + (levels - 1) * PAGE_SIZE); > - kpti_ng_temp_alloc = kpti_ng_temp_pgd_pa = __pa(kpti_ng_temp_pgd); > - > - // > - // Create a minimal page table hierarchy that permits us to map > - // the swapper page tables temporarily as we traverse them. > - // > - // The physical pages are laid out as follows: > - // > - // +--------+-/-------+-/------ +-/------ +-\\\--------+ > - // : PTE[] : | PMD[] : | PUD[] : | P4D[] : ||| PGD[] : > - // +--------+-\-------+-\------ +-\------ +-///--------+ > - // ^ > - // The first page is mapped into this hierarchy at a PMD_SHIFT > - // aligned virtual address, so that we can manipulate the PTE > - // level entries while the mapping is active. The first entry > - // covers the PTE[] page itself, the remaining entries are free > - // to be used as a ad-hoc fixmap. > - // > - create_kpti_ng_temp_pgd(kpti_ng_temp_pgd, __pa(alloc), > - KPTI_NG_TEMP_VA, PAGE_SIZE, PAGE_KERNEL, > - kpti_ng_pgd_alloc, 0); > - } > - > - cpu_install_idmap(); > - remap_fn(cpu, num_online_cpus(), kpti_ng_temp_pgd_pa, KPTI_NG_TEMP_VA); > - cpu_uninstall_idmap(); > - > - if (!cpu) { > - free_pages(alloc, order); > - arm64_use_ng_mappings = true; > - } > - > - return 0; > -} > - > -static void __init kpti_install_ng_mappings(void) > -{ > - /* Check whether KPTI is going to be used */ > - if (!arm64_kernel_unmapped_at_el0()) > - return; > - > - /* > - * We don't need to rewrite the page-tables if either we've done > - * it already or we have KASLR enabled and therefore have not > - * created any global mappings at all. > - */ > - if (arm64_use_ng_mappings) > - return; > - > - stop_machine(__kpti_install_ng_mappings, NULL, cpu_online_mask); > -} > - > -#else > -static inline void kpti_install_ng_mappings(void) > -{ > -} > -#endif /* CONFIG_UNMAP_KERNEL_AT_EL0 */ > - > static void cpu_enable_kpti(struct arm64_cpu_capabilities const *cap) > { > if (__this_cpu_read(this_cpu_vector) == vectors) { > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c > index 183801520740..eff3295393ee 100644 > --- a/arch/arm64/mm/mmu.c > +++ b/arch/arm64/mm/mmu.c > @@ -27,6 +27,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -466,14 +467,6 @@ static void __create_pgd_mapping(pgd_t *pgdir, phys_addr_t phys, > mutex_unlock(&fixmap_lock); > } > > -#ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > -extern __alias(__create_pgd_mapping_locked) > -void create_kpti_ng_temp_pgd(pgd_t *pgdir, phys_addr_t phys, unsigned long virt, > - phys_addr_t size, pgprot_t prot, > - phys_addr_t (*pgtable_alloc)(enum pgtable_type), > - int flags); > -#endif > - > static phys_addr_t __pgd_pgtable_alloc(struct mm_struct *mm, > enum pgtable_type pgtable_type) > { > @@ -735,7 +728,102 @@ static void __init declare_vma(struct vm_struct *vma, > } > > #ifdef CONFIG_UNMAP_KERNEL_AT_EL0 > -static pgprot_t kernel_exec_prot(void) > +#define KPTI_NG_TEMP_VA (-(1UL << PMD_SHIFT)) > + > +static phys_addr_t kpti_ng_temp_alloc __initdata; > + > +static phys_addr_t __init kpti_ng_pgd_alloc(enum pgtable_type type) > +{ > + kpti_ng_temp_alloc -= PAGE_SIZE; > + return kpti_ng_temp_alloc; > +} > + > +static void __init create_kpti_ng_temp_pgd(pgd_t *pgdir, phys_addr_t phys, > + unsigned long virt, phys_addr_t size, > + pgprot_t prot, > + phys_addr_t (*pgtable_alloc)(enum pgtable_type), > + int flags) > +{ > + __create_pgd_mapping_locked(pgdir, phys, virt, size, prot, > + pgtable_alloc, flags); > +} > + > +static int __init __kpti_install_ng_mappings(void *__unused) > +{ > + typedef void (kpti_remap_fn)(int, int, phys_addr_t, unsigned long); > + extern kpti_remap_fn idmap_kpti_install_ng_mappings; > + kpti_remap_fn *remap_fn; > + > + int cpu = smp_processor_id(); > + int levels = CONFIG_PGTABLE_LEVELS; > + int order = order_base_2(levels); > + u64 kpti_ng_temp_pgd_pa = 0; > + pgd_t *kpti_ng_temp_pgd; > + u64 alloc = 0; > + > + if (levels == 5 && !pgtable_l5_enabled()) > + levels = 4; > + else if (levels == 4 && !pgtable_l4_enabled()) > + levels = 3; > + > + remap_fn = (void *)__pa_symbol(idmap_kpti_install_ng_mappings); > + > + if (!cpu) { > + alloc = __get_free_pages(GFP_ATOMIC | __GFP_ZERO, order); > + kpti_ng_temp_pgd = (pgd_t *)(alloc + (levels - 1) * PAGE_SIZE); > + kpti_ng_temp_alloc = kpti_ng_temp_pgd_pa = __pa(kpti_ng_temp_pgd); > + > + // > + // Create a minimal page table hierarchy that permits us to map > + // the swapper page tables temporarily as we traverse them. > + // > + // The physical pages are laid out as follows: > + // > + // +--------+-/-------+-/------ +-/------ +-\\\--------+ > + // : PTE[] : | PMD[] : | PUD[] : | P4D[] : ||| PGD[] : > + // +--------+-\-------+-\------ +-\------ +-///--------+ > + // ^ > + // The first page is mapped into this hierarchy at a PMD_SHIFT > + // aligned virtual address, so that we can manipulate the PTE > + // level entries while the mapping is active. The first entry > + // covers the PTE[] page itself, the remaining entries are free > + // to be used as a ad-hoc fixmap. > + // > + create_kpti_ng_temp_pgd(kpti_ng_temp_pgd, __pa(alloc), > + KPTI_NG_TEMP_VA, PAGE_SIZE, PAGE_KERNEL, > + kpti_ng_pgd_alloc, 0); > + } > + > + cpu_install_idmap(); > + remap_fn(cpu, num_online_cpus(), kpti_ng_temp_pgd_pa, KPTI_NG_TEMP_VA); > + cpu_uninstall_idmap(); > + > + if (!cpu) { > + free_pages(alloc, order); > + arm64_use_ng_mappings = true; > + } > + > + return 0; > +} > + > +void __init kpti_install_ng_mappings(void) > +{ > + /* Check whether KPTI is going to be used */ > + if (!arm64_kernel_unmapped_at_el0()) > + return; > + > + /* > + * We don't need to rewrite the page-tables if either we've done > + * it already or we have KASLR enabled and therefore have not > + * created any global mappings at all. > + */ > + if (arm64_use_ng_mappings) > + return; > + > + stop_machine(__kpti_install_ng_mappings, NULL, cpu_online_mask); > +} > + > +static pgprot_t __init kernel_exec_prot(void) nit: this change (to add __init) is unrelated; does it deserve it's own patch? > { > return rodata_enabled ? PAGE_KERNEL_ROX : PAGE_KERNEL_EXEC; > } > > base-commit: 76eeb9b8de9880ca38696b2fb56ac45ac0a25c6c