* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [241028 16:43]:
> On Mon, Oct 28, 2024 at 10:22:32AM -1000, Linus Torvalds wrote:
> > On Mon, 28 Oct 2024 at 10:18, Lorenzo Stoakes
> > <lorenzo.stoakes@xxxxxxxxxx> wrote:
> > >
> > > I'm genuinely not opposed to a horrible, awful:
> > >
> > > #ifdef CONFIG_ARM64
> > > if (file && file->f_ops == shmem_file_operations)
> > > vm_flags |= VM_MTE_ALLOWED;
> > > #endif
> > >
> > > Early in the operation prior to the arch_validate_flags() check.
> >
> > I would just put it inside the arm64 code itself.
> >
> > IOW, get rid of the VM_MTE_ALLOWED flag entirely, and just make the
> > arm64 arch_validate_flags() code do something like
> >
> > if (flags & VM_MTE) {
> > if (file->f_ops != shmem_file_operations)
> > return false;
> > }
> >
> > and be done with it.
> >
> > Considering that we only have that horrendous arch_validate_flags()
> > for two architectures, and that they both just have magical special
> > cases for MTE-like behavior, I do think that just making it be a hack
> > inside those functions is the way to go.
> >
> > Linus
>
> Ah yeah makes sense.
>
> FWIW I just made a fix -for now- which implements it in the hideous way,
> shown below.
>
> We can maybe take that as a fix-patch for now and I can look at replacing
> this tomorrow with something as you suggest properly.
>
> My only concern is that arm people might not be happy and we get some hold
> up here...
>
> Thanks, Lorenzo
>
>
> ----8<----
> From fb6c15c74ba0db57f18b08fc6d1e901676f25bf6 Mon Sep 17 00:00:00 2001
> From: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> Date: Mon, 28 Oct 2024 20:36:49 +0000
> Subject: [PATCH] mm: account for MTE in arm64 on mmap_region() operation
>
> Correctly account for MTE on mmap_region(). We need to check this ahead of
> the operation, the shmem mmap hook was doing it, but this is at a point
> where a failure would mean we'd have to tear down a partially installed
> VMA.
>
> Avoid all this by adding a function to specifically handle this case.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> mm/mmap.c | 20 ++++++++++++++++++++
> mm/shmem.c | 3 ---
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 8462de1ee583..83afa1ebfd75 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1575,6 +1575,24 @@ static unsigned long __mmap_region(struct file *file, unsigned long addr,
> return error;
> }
>
> +/*
> + * We check VMA flag validity early in the mmap() process, however this can
> + * cause issues for arm64 when using MTE, which requires that it be used with
> + * shmem and in this instance and only then is VM_MTE_ALLOWED set permitting
> + * this operation.
> + *
> + * To avoid having to tear down a partially complete mapping we do this ahead of
> + * time.
> + */
> +static vm_flags_t arch_adjust_flags(struct file *file, vm_flags_t vm_flags)
Is it worth adding an inline?
> +{
> + if (!IS_ENABLED(CONFIG_ARM64))
CONFIG_ARM64_MTE .. otherwise VM_MTE_ALLOWED is 0 so, really doesn't
matter I guess.
> + return vm_flags;
> +
> + if (shmem_file(file))
> + return vm_flags | VM_MTE_ALLOWED;
Would if (VM_MTE_ALLOWED && shmem_file(file)) allow for the pre-compiler
to remove some of this? Also probably doesn't matter much.
> +}
> +
> unsigned long mmap_region(struct file *file, unsigned long addr,
> unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
> struct list_head *uf)
> @@ -1586,6 +1604,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> if (map_deny_write_exec(vm_flags, vm_flags))
> return -EACCES;
>
> + vm_flags = arch_adjust_flags(file, vm_flags);
> +
> /* Allow architectures to sanity-check the vm_flags. */
> if (!arch_validate_flags(vm_flags))
> return -EINVAL;
> 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)
> --
