On Tue, Oct 29, 2024 at 05:02:23PM +0000, Catalin Marinas wrote: > On Tue, Oct 29, 2024 at 04:36:32PM +0000, Lorenzo Stoakes wrote: > > On Tue, Oct 29, 2024 at 04:22:42PM +0000, Catalin Marinas wrote: > > > On Tue, Oct 29, 2024 at 03:16:00PM +0000, Lorenzo Stoakes wrote: > > > > On Tue, Oct 29, 2024 at 03:04:41PM +0000, Catalin Marinas wrote: > > > > > On Mon, Oct 28, 2024 at 10:14:50PM +0000, Lorenzo Stoakes wrote: > > > > > > 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. > > > > > > Not calc_vm_prot_bits() but calc_vm_flag_bits(). > > > arch_calc_vm_flag_bits() is only implemented by two architectures - > > > arm64 and parisc and calc_vm_flag_bits() is only called from do_mmap(). > > > > > > Basically we want to set VM_MTE_ALLOWED early during the mmap() call > > > and, at the time, my thinking was to do it in calc_vm_flag_bits(). The > > > calc_vm_prot_bits() OTOH is also called on the mprotect() path and is > > > responsible for translating PROT_MTE into a VM_MTE flag without any > > > checks. arch_validate_flags() would check if VM_MTE comes together with > > > VM_MTE_ALLOWED. But, as in the KVM case, that's not the only function > > > checking VM_MTE_ALLOWED. > > > > > > Since calc_vm_flag_bits() did not take a file argument, the lazy > > > approach was to add the flag explicitly for shmem (and hugetlbfs in > > > -next). But I think it would be easier to just add the file argument to > > > calc_vm_flag_bits() and do the check in the arch code to return > > > VM_MTE_ALLOWED. AFAICT, this is called before mmap_region() and > > > arch_validate_flags() (unless I missed something in the recent > > > reworking). > > > > I mean I totally get why you're suggesting it > > Not sure ;) > > > - it's the right _place_ but... > > It would require changes to a ton of code which is no good for a backport > > and we don't _need_ to do it. > > > > I'd rather do the smallest delta at this point, as I am not a huge fan of > > sticking it in here (I mean your point is wholly valid - it's at a better > > place to do so and we can change flags here, it's just - it's not where you > > expect to do this obviously). > > > > I mean for instance in arch/x86/kernel/cpu/sgx/encl.c (a file I'd _really_ > > like us not to touch here by the way) we'd have to what pass NULL? > > That's calc_vm_prot_bits(). I suggested calc_vm_flag_bits() (I know, > confusing names and total lack of inspiration when we added MTE > support). The latter is only called in one place - do_mmap(). > > That's what I meant (untested, on top of -next as it has a MAP_HUGETLB > check in there). I don't think it's much worse than your proposal, > assuming that it works: Right sorry misread. Yeah this is better, let me do a quick -v4 then! Cheers, sorry pretty tired at this stage, was looking at this all last night...
