Re: [PATCH hotfix 6.12 v2 4/8] mm: resolve faulty mmap_region() error path behaviour
From: Vlastimil Babka <hidden>
Date: 2024-10-29 07:50:13
Also in:
linux-mm, lkml
On 10/28/24 23:14, Lorenzo Stoakes wrote:
On Mon, Oct 28, 2024 at 10:28:47PM +0100, Vlastimil Babka wrote:quoted
On 10/28/24 22:19, Linus Torvalds wrote:quoted
On Mon, 28 Oct 2024 at 11:00, Vlastimil Babka [off-list ref] wrote:quoted
VM_MTE_ALLOWED is also set by arm64's arch_calc_vm_flag_bits(): if (system_supports_mte() && (flags & MAP_ANONYMOUS)) return VM_MTE_ALLOWED;Yeah, but that should just move into arch_validate_flags() too. There's no reason why that's done in a separate place. LinusRight, and VM_DATA_DEFAULT_FLAGS is only used in do_brk_flags() which is also an anonymous VMA, and it doesn't perform arch_validate_flags() anyway.Unfortunately we can't do this and have everything work entirely consistently.
Consistently with the current implementation, which seems inconsiscent wrt hugetlbfs.
This is because, while adding a file parameter to arch_validate_flags() lets us do this shmem check, we can't be sure MAP_ANON was set and we do not have access to this information at the point of the arch_validate_flags() check. We could check file == NULL, but this then excludes the MAP_ANON | MAP_HUGETLB case which is (probably accidentally) permitted by arch_calc_vm_flag_bits() and for the purposes of this fix we probably just want to keep behaviour identical. We could alternatively check the file is shmem _or_ hugetlb but this would amount to a change in behaviour and not sure we want to go there. Anyway I attach a patch that does what you suggest, I actually compiled this one (!) but don't have a system set up to test it (Mark?) If we bring this in it should probably be a separate commit... ----8<---- From 247003cd2a4b5f4fc2dac97f5ef7e473a47f4324 Mon Sep 17 00:00:00 2001 From: Lorenzo Stoakes <redacted> Date: Mon, 28 Oct 2024 22:05:44 +0000 Subject: [PATCH] mm: perform MTE check within arm64 hook entirely It doesn't make sense to have shmem explicitly check this one arch-specific case, it is arch-specific, so the arch should handle it. We know shmem is a case in which we want to permit MTE, so simply check for this directly. This also fixes the issue with checking arch_validate_flags() early, which would otherwise break mmap_region(). In order to implement this we must pass a file pointer, and additionally update the sparc code to accept this parameter too. We'd ideally like to have eliminated the arch_calc_vm_flag_bits() case, but we risk inadvertently changing behaviour as we do not have mmap() flags available at the point of the arch_validate_flags() check and a MAP_ANON | MAP_HUGETLB case would be accepted for MTE currently, but a MAP_SHARED | MAP_HUGETLB would not. This is likely an oversight but we want to try to keep behaviour identical to before in this patch.
If it's confirmed to be oversight and MAP_SHARED hugetlbfs should be allowed, we could add another is_file_hugepages() condition
So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if MAP_ANON. Signed-off-by: Lorenzo Stoakes <redacted>
Acked-by: Vlastimil Babka <redacted>
quoted hunk ↗ jump to hunk
--- arch/arm64/include/asm/mman.h | 18 ++++++++++++++---- arch/sparc/include/asm/mman.h | 5 +++-- include/linux/mman.h | 2 +- mm/mmap.c | 4 ++-- mm/mprotect.c | 2 +- mm/shmem.c | 3 --- 6 files changed, 21 insertions(+), 13 deletions(-)diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h index 9e39217b4afb..44b7c8a1dd67 100644 --- a/arch/arm64/include/asm/mman.h +++ b/arch/arm64/include/asm/mman.h@@ -6,7 +6,9 @@ #ifndef BUILD_VDSO #include <linux/compiler.h> +#include <linux/fs.h> #include <linux/types.h> +#include <linux/shmem_fs.h> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, unsigned long pkey)@@ -60,15 +62,23 @@ static inline bool arch_validate_prot(unsigned long prot, } #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) -static inline bool arch_validate_flags(unsigned long vm_flags) +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) { if (!system_supports_mte()) return true; - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); + if (!(vm_flags & VM_MTE)) + return true; + + if (vm_flags & VM_MTE_ALLOWED) + return true; + + if (shmem_file(file)) + return true; + + return false; } -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) #endif /* !BUILD_VDSO */diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h index af9c10c83dc5..d426e1f7c2c1 100644 --- a/arch/sparc/include/asm/mman.h +++ b/arch/sparc/include/asm/mman.h@@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long len); #ifdef CONFIG_SPARC64 #include <asm/adi_64.h> +#include <linux/fs.h> static inline void ipi_set_tstate_mcde(void *arg) {@@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr) return 1; } -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) /* arch_validate_flags() - Ensure combination of flags is valid for a * VMA. */ -static inline bool arch_validate_flags(unsigned long vm_flags) +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) { /* If ADI is being enabled on this VMA, check for ADI * capability on the platform and ensure VMA is suitablediff --git a/include/linux/mman.h b/include/linux/mman.h index 8ddca62d6460..82e6488026b7 100644 --- a/include/linux/mman.h +++ b/include/linux/mman.h@@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long prot, unsigned long addr) * * Returns true if the VM_* flags are valid. */ -static inline bool arch_validate_flags(unsigned long flags) +static inline bool arch_validate_flags(struct file *file, unsigned long flags) { return true; }diff --git a/mm/mmap.c b/mm/mmap.c index 8462de1ee583..e06171d243ef 100644 --- a/mm/mmap.c +++ b/mm/mmap.c@@ -1504,7 +1504,7 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr, #ifdef CONFIG_SPARC64 /* TODO: Fix SPARC ADI! */ - WARN_ON_ONCE(!arch_validate_flags(vm_flags)); + WARN_ON_ONCE(!arch_validate_flags(file, vm_flags)); #endif /* Lock the VMA since it is modified after insertion into VMA tree */@@ -1587,7 +1587,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, return -EACCES; /* Allow architectures to sanity-check the vm_flags. */ - if (!arch_validate_flags(vm_flags)) + if (!arch_validate_flags(file, vm_flags)) return -EINVAL; /* Map writable and ensure this isn't a sealed memfd. */diff --git a/mm/mprotect.c b/mm/mprotect.c index 6f450af3252e..c6db98b893fc 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c@@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, } /* Allow architectures to sanity-check the new flags */ - if (!arch_validate_flags(newflags)) { + if (!arch_validate_flags(vma->vm_file, newflags)) { error = -EINVAL; break; }diff --git a/mm/shmem.c b/mm/shmem.c index 4ba1d00fabda..e87f5d6799a7 100644 --- a/mm/shmem.c +++ b/mm/shmem.c@@ -2733,9 +2733,6 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma) if (ret) return ret; - /* arm64 - allow memory tagging on RAM-based files */ - vm_flags_set(vma, VM_MTE_ALLOWED); - file_accessed(file); /* This is anonymous shared memory if it is unlinked at the time of mmap */ if (inode->i_nlink) --2.47.0