On Tue, Oct 29, 2024 at 03:04:41PM +0000, Catalin Marinas wrote: > Hi Lorenzo, > > Thanks for trying to fix this. > > On Mon, Oct 28, 2024 at 10:14:50PM +0000, Lorenzo Stoakes wrote: > > From 247003cd2a4b5f4fc2dac97f5ef7e473a47f4324 Mon Sep 17 00:00:00 2001 > > From: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> > > 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. > > MAP_HUGETLB support for MTE is only in -next currently, so there > wouldn't be any ABI change if we also allowed MAP_SHARED | MAP_HUGETLB. > In 6.12, MAP_HUGETLB is not allowed to have PROT_MTE. > > > So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if > > MAP_ANON. > [...] > > 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); > > This breaks arm64 KVM if the VMM uses shared mappings for the memory > slots (which is possible). We have kvm_vma_mte_allowed() that checks for > the VM_MTE_ALLOWED flag as the VMM may not use PROT_MTE/VM_MTE directly. Ugh yup missed that thanks. > > I need to read this thread properly but why not pass the file argument > to arch_calc_vm_flag_bits() and set VM_MTE_ALLOWED in there? Can't really do that as it is entangled in a bunch of other stuff, e.g. calc_vm_prot_bits() would have to pass file and that's used in a bunch of places including arch code and... etc. etc. And definitely no good for a hotfix that has to be backported. I suggest instead we instead don't drop the yucky shmem thing, which will set VM_MTE_ALLOWED for shmem, with arch_calc_vm_flag_bits() still setting it for MAP_ANON, but the other changes will mean the arch_validate_flags() will be fixed too. So this just means not dropping the mm/shmem.c bit basically and everything should 'just work'? > > -- > Catalin But we definitely need to find a better way post-hotfix to sort all this out I'm sure you agree :)
