On Thu, Nov 14, 2024 at 06:07:41PM +0000, Lorenzo Stoakes wrote: > Currently MTE is permitted in two circumstances (desiring to use MTE > having been specified by the VM_MTE flag) - where MAP_ANONYMOUS is > specified, as checked by arch_calc_vm_flag_bits() and actualised by > setting the VM_MTE_ALLOWED flag, or if the file backing the mapping is > shmem, in which case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap > hook is activated in mmap_region(). > > The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also > set is the arm64 implementation of arch_validate_flags(). > > Unfortunately, we intend to refactor mmap_region() to perform this check > earlier, meaning that in the case of a shmem backing we will not have > invoked shmem_mmap() yet, causing the mapping to fail spuriously. > > It is inappropriate to set this architecture-specific flag in general mm > code anyway, so a sensible resolution of this issue is to instead move the > check somewhere else. > > We resolve this by setting VM_MTE_ALLOWED much earlier in do_mmap(), via > the arch_calc_vm_flag_bits() call. > > This is an appropriate place to do this as we already check for the > MAP_ANONYMOUS case here, and the shmem file case is simply a variant of > the same idea - we permit RAM-backed memory. > > This requires a modification to the arch_calc_vm_flag_bits() signature to > pass in a pointer to the struct file associated with the mapping, however > this is not too egregious as this is only used by two architectures anyway > - arm64 and parisc. > > So this patch performs this adjustment and removes the unnecessary > assignment of VM_MTE_ALLOWED in shmem_mmap(). For avoidance of doubt, NACK this and rest of 6.1.y series, I'll resend. > > [akpm@xxxxxxxxxxxxxxxxxxxx: fix whitespace, per Catalin] > Link: https://lkml.kernel.org/r/ec251b20ba1964fb64cf1607d2ad80c47f3873df.1730224667.git.lorenzo.stoakes@xxxxxxxxxx > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails") > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx> > Reported-by: Jann Horn <jannh@xxxxxxxxxx> > Reviewed-by: Catalin Marinas <catalin.marinas@xxxxxxx> > Reviewed-by: Vlastimil Babka <vbabka@xxxxxxx> > Cc: Andreas Larsson <andreas@xxxxxxxxxxx> > Cc: David S. Miller <davem@xxxxxxxxxxxxx> > Cc: Helge Deller <deller@xxxxxx> > Cc: James E.J. Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > Cc: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> > Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > Cc: Mark Brown <broonie@xxxxxxxxxx> > Cc: Peter Xu <peterx@xxxxxxxxxx> > Cc: Will Deacon <will@xxxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > (cherry picked from commit 5baf8b037debf4ec60108ccfeccb8636d1dbad81) > --- > arch/arm64/include/asm/mman.h | 10 +++++++--- > include/linux/mman.h | 7 ++++--- > mm/mmap.c | 2 +- > mm/nommu.c | 2 +- > mm/shmem.c | 3 --- > 5 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h > index 5966ee4a6154..ef35c52aabd6 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -3,6 +3,8 @@ > #define __ASM_MMAN_H__ > > #include <linux/compiler.h> > +#include <linux/fs.h> > +#include <linux/shmem_fs.h> > #include <linux/types.h> > #include <uapi/asm/mman.h> > > @@ -21,19 +23,21 @@ static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, > } > #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot, pkey) > > -static inline unsigned long arch_calc_vm_flag_bits(unsigned long flags) > +static inline unsigned long arch_calc_vm_flag_bits(struct file *file, > + unsigned long flags) > { > /* > * Only allow MTE on anonymous mappings as these are guaranteed to be > * backed by tags-capable memory. The vm_flags may be overridden by a > * filesystem supporting MTE (RAM-based). > */ > - if (system_supports_mte() && (flags & MAP_ANONYMOUS)) > + if (system_supports_mte() && > + ((flags & MAP_ANONYMOUS) || shmem_file(file))) > return VM_MTE_ALLOWED; > > return 0; > } > -#define arch_calc_vm_flag_bits(flags) arch_calc_vm_flag_bits(flags) > +#define arch_calc_vm_flag_bits(file, flags) arch_calc_vm_flag_bits(file, flags) > > static inline bool arch_validate_prot(unsigned long prot, > unsigned long addr __always_unused) > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 58b3abd457a3..21ea08b919d9 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -2,6 +2,7 @@ > #ifndef _LINUX_MMAN_H > #define _LINUX_MMAN_H > > +#include <linux/fs.h> > #include <linux/mm.h> > #include <linux/percpu_counter.h> > > @@ -90,7 +91,7 @@ static inline void vm_unacct_memory(long pages) > #endif > > #ifndef arch_calc_vm_flag_bits > -#define arch_calc_vm_flag_bits(flags) 0 > +#define arch_calc_vm_flag_bits(file, flags) 0 > #endif > > #ifndef arch_validate_prot > @@ -147,12 +148,12 @@ calc_vm_prot_bits(unsigned long prot, unsigned long pkey) > * Combine the mmap "flags" argument into "vm_flags" used internally. > */ > static inline unsigned long > -calc_vm_flag_bits(unsigned long flags) > +calc_vm_flag_bits(struct file *file, unsigned long flags) > { > return _calc_vm_trans(flags, MAP_GROWSDOWN, VM_GROWSDOWN ) | > _calc_vm_trans(flags, MAP_LOCKED, VM_LOCKED ) | > _calc_vm_trans(flags, MAP_SYNC, VM_SYNC ) | > - arch_calc_vm_flag_bits(flags); > + arch_calc_vm_flag_bits(file, flags); > } > > unsigned long vm_commit_limit(void); > diff --git a/mm/mmap.c b/mm/mmap.c > index 4bfec4df51c2..322677f61d30 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1316,7 +1316,7 @@ unsigned long do_mmap(struct file *file, unsigned long addr, > * to. we assume access permissions have been handled by the open > * of the memory object, so we don't do any here. > */ > - vm_flags = calc_vm_prot_bits(prot, pkey) | calc_vm_flag_bits(flags) | > + vm_flags = calc_vm_prot_bits(prot, pkey) | calc_vm_flag_bits(file, flags) | > mm->def_flags | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC; > > if (flags & MAP_LOCKED) > diff --git a/mm/nommu.c b/mm/nommu.c > index e0428fa57526..859ba6bdeb9c 100644 > --- a/mm/nommu.c > +++ b/mm/nommu.c > @@ -903,7 +903,7 @@ static unsigned long determine_vm_flags(struct file *file, > { > unsigned long vm_flags; > > - vm_flags = calc_vm_prot_bits(prot, 0) | calc_vm_flag_bits(flags); > + vm_flags = calc_vm_prot_bits(prot, 0) | calc_vm_flag_bits(file, flags); > /* vm_flags |= mm->def_flags; */ > > if (!(capabilities & NOMMU_MAP_DIRECT)) { > diff --git a/mm/shmem.c b/mm/shmem.c > index 0e1fbc53717d..d1a33f66cc7f 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2308,9 +2308,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 */ > - vma->vm_flags |= VM_MTE_ALLOWED; > - > file_accessed(file); > vma->vm_ops = &shmem_vm_ops; > return 0; > -- > 2.47.0 >
