| Message ID | 1646045273-9343-6-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmap: Drop protection_map[] and platform's __SXXX/__PXXX requirements | expand |
Hi Anshuman, On Mon, Feb 28, 2022 at 04:17:28PM +0530, Anshuman Khandual wrote: > +static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) > +{ > + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { > + case VM_NONE: > + return PAGE_NONE; > + case VM_READ: > + case VM_WRITE: > + case VM_WRITE | VM_READ: > + return PAGE_READONLY; > + case VM_EXEC: > + return PAGE_EXECONLY; > + case VM_EXEC | VM_READ: > + case VM_EXEC | VM_WRITE: > + case VM_EXEC | VM_WRITE | VM_READ: > + return PAGE_READONLY_EXEC; > + case VM_SHARED: > + return PAGE_NONE; > + case VM_SHARED | VM_READ: > + return PAGE_READONLY; > + case VM_SHARED | VM_WRITE: > + case VM_SHARED | VM_WRITE | VM_READ: > + return PAGE_SHARED; > + case VM_SHARED | VM_EXEC: > + return PAGE_EXECONLY; > + case VM_SHARED | VM_EXEC | VM_READ: > + return PAGE_READONLY_EXEC; > + case VM_SHARED | VM_EXEC | VM_WRITE: > + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: > + return PAGE_SHARED_EXEC; > + default: > + BUILD_BUG(); > + } > +} I'd say ack for trying to get of the extra arch_vm_get_page_prot() and arch_filter_pgprot() but, TBH, I'm not so keen on the outcome. I haven't built the code to see what's generated but I suspect it's no significant improvement. As for the code readability, the arm64 parts don't look much better either. The only advantage with this patch is that all functions have been moved under arch/arm64. I'd keep most architectures that don't have own arch_vm_get_page_prot() or arch_filter_pgprot() unchanged and with a generic protection_map[] array. For architectures that need fancier stuff, add a CONFIG_ARCH_HAS_VM_GET_PAGE_PROT (as you do) and allow them to define vm_get_page_prot() while getting rid of arch_vm_get_page_prot() and arch_filter_pgprot(). I think you could also duplicate protection_map[] for architectures with own vm_get_page_prot() (make it static) and #ifdef it out in mm/mmap.c. If later you have more complex needs or a switch statement generates better code, go for it, but for this series I'd keep things simple, only focus on getting rid of arch_vm_get_page_prot() and arch_filter_pgprot(). If I grep'ed correctly, there are only 4 architectures that have own arch_vm_get_page_prot() (arm64, powerpc, sparc, x86) and 2 that have own arch_filter_pgprot() (arm64, x86). Try to only change these for the time being, together with the other generic mm cleanups you have in this series. I think there are a couple more that touch protection_map[] (arm, m68k). You can leave the generic protection_map[] global if the arch does not select ARCH_HAS_VM_GET_PAGE_PROT. > +static pgprot_t arm64_arch_filter_pgprot(pgprot_t prot) > +{ > + if (cpus_have_const_cap(ARM64_HAS_EPAN)) > + return prot; > + > + if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY)) > + return prot; > + > + return PAGE_READONLY_EXEC; > +} > + > +static pgprot_t arm64_arch_vm_get_page_prot(unsigned long vm_flags) > +{ > + pteval_t prot = 0; > + > + if (vm_flags & VM_ARM64_BTI) > + prot |= PTE_GP; > + > + /* > + * There are two conditions required for returning a Normal Tagged > + * memory type: (1) the user requested it via PROT_MTE passed to > + * mmap() or mprotect() and (2) the corresponding vma supports MTE. We > + * register (1) as VM_MTE in the vma->vm_flags and (2) as > + * VM_MTE_ALLOWED. Note that the latter can only be set during the > + * mmap() call since mprotect() does not accept MAP_* flags. > + * Checking for VM_MTE only is sufficient since arch_validate_flags() > + * does not permit (VM_MTE & !VM_MTE_ALLOWED). > + */ > + if (vm_flags & VM_MTE) > + prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); > + > + return __pgprot(prot); > +} > + > +pgprot_t vm_get_page_prot(unsigned long vm_flags) > +{ > + pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) | > + pgprot_val(arm64_arch_vm_get_page_prot(vm_flags))); > + > + return arm64_arch_filter_pgprot(ret); > +} If we kept the array, we can have everything in a single function (untested and with my own comments for future changes): pgprot_t vm_get_page_prot(unsigned long vm_flags) { pgprot_t prot = __pgprot(pgprot_val(protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)])); /* * We could get rid of this test if we updated protection_map[] * to turn exec-only into read-exec during boot. */ if (!cpus_have_const_cap(ARM64_HAS_EPAN) && pgprot_val(prot) == pgprot_val(PAGE_EXECONLY)) prot = PAGE_READONLY_EXEC; if (vm_flags & VM_ARM64_BTI) prot != PTE_GP; /* * We can get rid of the requirement for PROT_NORMAL to be 0 * since here we can mask out PTE_ATTRINDX_MASK. */ if (vm_flags & VM_MTE) { prot &= ~PTE_ATTRINDX_MASK; prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); } return prot; }
On 3/3/22 20:58, Catalin Marinas wrote: > Hi Anshuman, > > On Mon, Feb 28, 2022 at 04:17:28PM +0530, Anshuman Khandual wrote: >> +static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) >> +{ >> + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { >> + case VM_NONE: >> + return PAGE_NONE; >> + case VM_READ: >> + case VM_WRITE: >> + case VM_WRITE | VM_READ: >> + return PAGE_READONLY; >> + case VM_EXEC: >> + return PAGE_EXECONLY; >> + case VM_EXEC | VM_READ: >> + case VM_EXEC | VM_WRITE: >> + case VM_EXEC | VM_WRITE | VM_READ: >> + return PAGE_READONLY_EXEC; >> + case VM_SHARED: >> + return PAGE_NONE; >> + case VM_SHARED | VM_READ: >> + return PAGE_READONLY; >> + case VM_SHARED | VM_WRITE: >> + case VM_SHARED | VM_WRITE | VM_READ: >> + return PAGE_SHARED; >> + case VM_SHARED | VM_EXEC: >> + return PAGE_EXECONLY; >> + case VM_SHARED | VM_EXEC | VM_READ: >> + return PAGE_READONLY_EXEC; >> + case VM_SHARED | VM_EXEC | VM_WRITE: >> + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: >> + return PAGE_SHARED_EXEC; >> + default: >> + BUILD_BUG(); >> + } >> +} > > I'd say ack for trying to get of the extra arch_vm_get_page_prot() and > arch_filter_pgprot() but, TBH, I'm not so keen on the outcome. I haven't > built the code to see what's generated but I suspect it's no significant > improvement. As for the code readability, the arm64 parts don't look > much better either. The only advantage with this patch is that all > functions have been moved under arch/arm64. Got it. > > I'd keep most architectures that don't have own arch_vm_get_page_prot() > or arch_filter_pgprot() unchanged and with a generic protection_map[] > array. For architectures that need fancier stuff, add a > CONFIG_ARCH_HAS_VM_GET_PAGE_PROT (as you do) and allow them to define > vm_get_page_prot() while getting rid of arch_vm_get_page_prot() and > arch_filter_pgprot(). I think you could also duplicate protection_map[] > for architectures with own vm_get_page_prot() (make it static) and > #ifdef it out in mm/mmap.c. > > If later you have more complex needs or a switch statement generates > better code, go for it, but for this series I'd keep things simple, only > focus on getting rid of arch_vm_get_page_prot() and > arch_filter_pgprot(). Got it. > > If I grep'ed correctly, there are only 4 architectures that have own > arch_vm_get_page_prot() (arm64, powerpc, sparc, x86) and 2 that have own > arch_filter_pgprot() (arm64, x86). Try to only change these for the time > being, together with the other generic mm cleanups you have in this > series. I think there are a couple more that touch protection_map[] > (arm, m68k). You can leave the generic protection_map[] global if the > arch does not select ARCH_HAS_VM_GET_PAGE_PROT. Okay, I will probably split the series into two parts. - Drop arch_vm_get_page_prot() and arch_filter_pgprot() on relevant platforms i.e arm64, powerpc, sparc and x86 via this new config ARCH_HAS_VM_GET_PAGE_PROT, keeping the generic protection_map[] since platform __SXXX/__PXX macros would be still around. - Drop __SXXX/__PXXX across all platforms via just initializing protection_map[] early during boot in the platform OR moving both vm_get_page_prot() via ARCH_HAS_VM_GET_PAGE_PROT and the generic protection_map[] inside the platform. There were some objections with respect to switch case code in comparison to the array based table look up. > >> +static pgprot_t arm64_arch_filter_pgprot(pgprot_t prot) >> +{ >> + if (cpus_have_const_cap(ARM64_HAS_EPAN)) >> + return prot; >> + >> + if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY)) >> + return prot; >> + >> + return PAGE_READONLY_EXEC; >> +} >> + >> +static pgprot_t arm64_arch_vm_get_page_prot(unsigned long vm_flags) >> +{ >> + pteval_t prot = 0; >> + >> + if (vm_flags & VM_ARM64_BTI) >> + prot |= PTE_GP; >> + >> + /* >> + * There are two conditions required for returning a Normal Tagged >> + * memory type: (1) the user requested it via PROT_MTE passed to >> + * mmap() or mprotect() and (2) the corresponding vma supports MTE. We >> + * register (1) as VM_MTE in the vma->vm_flags and (2) as >> + * VM_MTE_ALLOWED. Note that the latter can only be set during the >> + * mmap() call since mprotect() does not accept MAP_* flags. >> + * Checking for VM_MTE only is sufficient since arch_validate_flags() >> + * does not permit (VM_MTE & !VM_MTE_ALLOWED). >> + */ >> + if (vm_flags & VM_MTE) >> + prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); >> + >> + return __pgprot(prot); >> +} >> + >> +pgprot_t vm_get_page_prot(unsigned long vm_flags) >> +{ >> + pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) | >> + pgprot_val(arm64_arch_vm_get_page_prot(vm_flags))); >> + >> + return arm64_arch_filter_pgprot(ret); >> +} > > If we kept the array, we can have everything in a single function > (untested and with my own comments for future changes): Got it. > > pgprot_t vm_get_page_prot(unsigned long vm_flags) > { > pgprot_t prot = __pgprot(pgprot_val(protection_map[vm_flags & > (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)])); > > /* > * We could get rid of this test if we updated protection_map[] > * to turn exec-only into read-exec during boot. > */ > if (!cpus_have_const_cap(ARM64_HAS_EPAN) && > pgprot_val(prot) == pgprot_val(PAGE_EXECONLY)) > prot = PAGE_READONLY_EXEC; > > if (vm_flags & VM_ARM64_BTI) > prot != PTE_GP; > > /* > * We can get rid of the requirement for PROT_NORMAL to be 0 > * since here we can mask out PTE_ATTRINDX_MASK. > */ > if (vm_flags & VM_MTE) { > prot &= ~PTE_ATTRINDX_MASK; > prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); > } > > return prot; > } >
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 2e5d2eac6fc6..7153d5fff603 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -23,7 +23,6 @@ config ARM64 select ARCH_HAS_DMA_PREP_COHERENT select ARCH_HAS_ACPI_TABLE_UPGRADE if ACPI select ARCH_HAS_FAST_MULTIPLIER - select ARCH_HAS_FILTER_PGPROT select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_GIGANTIC_PAGE @@ -44,6 +43,7 @@ config ARM64 select ARCH_HAS_SYSCALL_WRAPPER select ARCH_HAS_TEARDOWN_DMA_OPS if IOMMU_SUPPORT select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST + select ARCH_HAS_VM_GET_PAGE_PROT select ARCH_HAS_ZONE_DMA_SET if EXPERT select ARCH_HAVE_ELF_PROT select ARCH_HAVE_NMI_SAFE_CMPXCHG diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index 0af70d9abede..64a613a0349b 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -126,8 +126,7 @@ * Memory types available. * * IMPORTANT: MT_NORMAL must be index 0 since vm_get_page_prot() may 'or' in - * the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. Note - * that protection_map[] only contains MT_NORMAL attributes. + * the MT_NORMAL_TAGGED memory type for PROT_MTE mappings. */ #define MT_NORMAL 0 #define MT_NORMAL_TAGGED 1 diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h index e3e28f7daf62..5966ee4a6154 100644 --- a/arch/arm64/include/asm/mman.h +++ b/arch/arm64/include/asm/mman.h @@ -35,30 +35,6 @@ static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags) } #define arch_calc_vm_flag_bits(flags) arch_calc_vm_flag_bits(flags) -static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags) -{ - pteval_t prot = 0; - - if (vm_flags & VM_ARM64_BTI) - prot |= PTE_GP; - - /* - * There are two conditions required for returning a Normal Tagged - * memory type: (1) the user requested it via PROT_MTE passed to - * mmap() or mprotect() and (2) the corresponding vma supports MTE. We - * register (1) as VM_MTE in the vma->vm_flags and (2) as - * VM_MTE_ALLOWED. Note that the latter can only be set during the - * mmap() call since mprotect() does not accept MAP_* flags. - * Checking for VM_MTE only is sufficient since arch_validate_flags() - * does not permit (VM_MTE & !VM_MTE_ALLOWED). - */ - if (vm_flags & VM_MTE) - prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); - - return __pgprot(prot); -} -#define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags) - static inline bool arch_validate_prot(unsigned long prot, unsigned long addr __always_unused) { diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h index 7032f04c8ac6..d8ee0aa7886d 100644 --- a/arch/arm64/include/asm/pgtable-prot.h +++ b/arch/arm64/include/asm/pgtable-prot.h @@ -88,24 +88,6 @@ extern bool arm64_use_ng_mappings; #define PAGE_READONLY_EXEC __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN) #define PAGE_EXECONLY __pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN) -#define __P000 PAGE_NONE -#define __P001 PAGE_READONLY -#define __P010 PAGE_READONLY -#define __P011 PAGE_READONLY -#define __P100 PAGE_EXECONLY -#define __P101 PAGE_READONLY_EXEC -#define __P110 PAGE_READONLY_EXEC -#define __P111 PAGE_READONLY_EXEC - -#define __S000 PAGE_NONE -#define __S001 PAGE_READONLY -#define __S010 PAGE_SHARED -#define __S011 PAGE_SHARED -#define __S100 PAGE_EXECONLY -#define __S101 PAGE_READONLY_EXEC -#define __S110 PAGE_SHARED_EXEC -#define __S111 PAGE_SHARED_EXEC - #endif /* __ASSEMBLY__ */ #endif /* __ASM_PGTABLE_PROT_H */ diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h index c4ba047a82d2..94e147e5456c 100644 --- a/arch/arm64/include/asm/pgtable.h +++ b/arch/arm64/include/asm/pgtable.h @@ -1017,17 +1017,6 @@ static inline bool arch_wants_old_prefaulted_pte(void) } #define arch_wants_old_prefaulted_pte arch_wants_old_prefaulted_pte -static inline pgprot_t arch_filter_pgprot(pgprot_t prot) -{ - if (cpus_have_const_cap(ARM64_HAS_EPAN)) - return prot; - - if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY)) - return prot; - - return PAGE_READONLY_EXEC; -} - static inline bool pud_sect_supported(void) { return PAGE_SIZE == SZ_4K; diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c index a38f54cd638c..bd0233d376a2 100644 --- a/arch/arm64/mm/mmap.c +++ b/arch/arm64/mm/mmap.c @@ -10,6 +10,7 @@ #include <linux/types.h> #include <asm/page.h> +#include <asm/mman.h> /* * You really shouldn't be using read() or write() on /dev/mem. This might go @@ -38,3 +39,80 @@ int valid_mmap_phys_addr_range(unsigned long pfn, size_t size) { return !(((pfn << PAGE_SHIFT) + size) & ~PHYS_MASK); } + +static inline pgprot_t __vm_get_page_prot(unsigned long vm_flags) +{ + switch (vm_flags & (VM_READ | VM_WRITE | VM_EXEC | VM_SHARED)) { + case VM_NONE: + return PAGE_NONE; + case VM_READ: + case VM_WRITE: + case VM_WRITE | VM_READ: + return PAGE_READONLY; + case VM_EXEC: + return PAGE_EXECONLY; + case VM_EXEC | VM_READ: + case VM_EXEC | VM_WRITE: + case VM_EXEC | VM_WRITE | VM_READ: + return PAGE_READONLY_EXEC; + case VM_SHARED: + return PAGE_NONE; + case VM_SHARED | VM_READ: + return PAGE_READONLY; + case VM_SHARED | VM_WRITE: + case VM_SHARED | VM_WRITE | VM_READ: + return PAGE_SHARED; + case VM_SHARED | VM_EXEC: + return PAGE_EXECONLY; + case VM_SHARED | VM_EXEC | VM_READ: + return PAGE_READONLY_EXEC; + case VM_SHARED | VM_EXEC | VM_WRITE: + case VM_SHARED | VM_EXEC | VM_WRITE | VM_READ: + return PAGE_SHARED_EXEC; + default: + BUILD_BUG(); + } +} + +static pgprot_t arm64_arch_filter_pgprot(pgprot_t prot) +{ + if (cpus_have_const_cap(ARM64_HAS_EPAN)) + return prot; + + if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY)) + return prot; + + return PAGE_READONLY_EXEC; +} + +static pgprot_t arm64_arch_vm_get_page_prot(unsigned long vm_flags) +{ + pteval_t prot = 0; + + if (vm_flags & VM_ARM64_BTI) + prot |= PTE_GP; + + /* + * There are two conditions required for returning a Normal Tagged + * memory type: (1) the user requested it via PROT_MTE passed to + * mmap() or mprotect() and (2) the corresponding vma supports MTE. We + * register (1) as VM_MTE in the vma->vm_flags and (2) as + * VM_MTE_ALLOWED. Note that the latter can only be set during the + * mmap() call since mprotect() does not accept MAP_* flags. + * Checking for VM_MTE only is sufficient since arch_validate_flags() + * does not permit (VM_MTE & !VM_MTE_ALLOWED). + */ + if (vm_flags & VM_MTE) + prot |= PTE_ATTRINDX(MT_NORMAL_TAGGED); + + return __pgprot(prot); +} + +pgprot_t vm_get_page_prot(unsigned long vm_flags) +{ + pgprot_t ret = __pgprot(pgprot_val(__vm_get_page_prot(vm_flags)) | + pgprot_val(arm64_arch_vm_get_page_prot(vm_flags))); + + return arm64_arch_filter_pgprot(ret); +} +EXPORT_SYMBOL(vm_get_page_prot);
This defines and exports a platform specific custom vm_get_page_prot() via subscribing ARCH_HAS_VM_GET_PAGE_PROT. Subsequently all __SXXX and __PXXX macros can be dropped which are no longer needed. This also localizes both arch_filter_pgprot and arch_vm_get_page_prot() helpers, unsubscribing from ARCH_HAS_FILTER_PGPROT as well. Moved both these localized functions near vm_get_page_prot(). Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- arch/arm64/Kconfig | 2 +- arch/arm64/include/asm/memory.h | 3 +- arch/arm64/include/asm/mman.h | 24 --------- arch/arm64/include/asm/pgtable-prot.h | 18 ------- arch/arm64/include/asm/pgtable.h | 11 ---- arch/arm64/mm/mmap.c | 78 +++++++++++++++++++++++++++ 6 files changed, 80 insertions(+), 56 deletions(-)