Re: [PATCH hotfix 6.12 v4 4/5] mm: refactor arch_calc_vm_flag_bits() and arm64 MTE handling

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Wed, Oct 30, 2024 at 07:58:33AM -0700, Yang Shi wrote:
>
>
> On 10/30/24 4:53 AM, Lorenzo Stoakes wrote:
> > On Wed, Oct 30, 2024 at 12:09:43PM +0100, Vlastimil Babka wrote:
> > > On 10/30/24 11:58, Catalin Marinas wrote:
> > > > On Wed, Oct 30, 2024 at 10:18:27AM +0100, Vlastimil Babka wrote:
> > > > > On 10/29/24 19:11, Lorenzo Stoakes wrote:
> > > > > > --- a/arch/arm64/include/asm/mman.h
> > > > > > +++ b/arch/arm64/include/asm/mman.h
> > > > > > @@ -6,6 +6,8 @@
> > > > > >
> > > > > >   #ifndef BUILD_VDSO
> > > > > >   #include <linux/compiler.h>
> > > > > > +#include <linux/fs.h>
> > > > > > +#include <linux/shmem_fs.h>
> > > > > >   #include <linux/types.h>
> > > > > >
> > > > > >   static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
> > > > > > @@ -31,19 +33,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).
> > > > > We should also eventually remove the last sentence or even replace it with
> > > > > its negation, or somebody might try reintroducing the pattern that won't
> > > > > work anymore (wasn't there such a hugetlbfs thing in -next?).
> > > > I agree, we should update this comment as well though as a fix this
> > > > patch is fine for now.
> > > >
> > > > There is indeed a hugetlbfs change in -next adding VM_MTE_ALLOWED. It
> > > > should still work after the above change but we'd need to move it over
> > > I guess it will work after the above change, but not after 5/5?
> > >
> > > > here (and fix the comment at the same time). We'll probably do it around
> > > > -rc1 or maybe earlier once this fix hits mainline.
> > > I assume this will hopefully go to rc7.
> > To be clear - this is a CRITICAL fix that MUST land for 6.12. I'd be inclined to
> > try to get it to an earlier rc-.
> >
> > > > I don't think we have
> > > > an equivalent of shmem_file() for hugetlbfs, we'll need to figure
> > > > something out.
> > > I've found is_file_hugepages(), could work? And while adding the hugetlbfs
> > > change here, the comment could be adjusted too, right?
> > Right but the MAP_HUGETLB should work to? Can we save such changes that
> > alter any kind of existing behaviour to later series?
>
> We should need both because mmap hugetlbfs file may not use MAP_HUGETLB.

Right yeah, we could create a memfd with MFD_HUGETLB for instance and mount
that...

Perhaps somebody could propose the 6.13 change (as this series is just
focused on the hotfix)?

Note that we absolutely plan to try to merge this in 6.12 (it is a critical
fix for a few separate issues).

I guess since we already have something in the arm64 tree adding
MAP_HUGETLB we could rebase that and add a is_file_hugepages() there to
cover off that case too?

(Though I note that shm_file_operations_huge also sets FOP_HUGE_PAGES which
this predicate picks up, not sure if we're ok wtih that? But discussion
better had I think in whichever thread this hugetlb change came from
perhaps?)

Catalin, perhaps?

>
> >
> > As this is going to be backported (by me...!) and I don't want to risk
> > inadvertant changes.
> >
> > > > > >   	 */
> > > > > > -	if (system_supports_mte() && (flags & MAP_ANONYMOUS))
> > > > > > +	if (system_supports_mte() &&
> > > > > > +	    ((flags & MAP_ANONYMOUS) || shmem_file(file)))
> > > > > >   		return VM_MTE_ALLOWED;
> > > > > >
> > > > > >   	return 0;
> > > > > >   }
> > > > This will conflict with the arm64 for-next/core tree as it's adding
> > > > a MAP_HUGETLB check. Trivial resolution though, Stephen will handle it.
> > Thanks!
> >
>

Thanks all!