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

[Catalin Marinas <catalin.marinas@arm.com>]

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.
> 
> > 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, thanks for the hint.

I guess the conflict resolution in -next will be something like:

----------------8<----------------------------------
diff --cc arch/arm64/include/asm/mman.h
index 798d965760d4,65bc2b07f666..8b9b819196e5
--- a/arch/arm64/include/asm/mman.h
+++ b/arch/arm64/include/asm/mman.h
@@@ -42,7 -39,7 +42,7 @@@ static inline unsigned long arch_calc_v
  	 * filesystem supporting MTE (RAM-based).
  	 */
  	if (system_supports_mte() &&
- 	    ((flags & MAP_ANONYMOUS) || shmem_file(file)))
 -	    (flags & (MAP_ANONYMOUS | MAP_HUGETLB)))
++	    ((flags & (MAP_ANONYMOUS | MAP_HUGETLB)) || shmem_file(file)))
  		return VM_MTE_ALLOWED;

  	return 0;
----------------8<----------------------------------

The fix-up for hugetlbfs is something like:

----------------8<----------------------------------
diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h
index 8b9b819196e5..988eff8269a6 100644
--- a/arch/arm64/include/asm/mman.h
+++ b/arch/arm64/include/asm/mman.h
@@ -6,6 +6,7 @@

 #ifndef BUILD_VDSO
 #include <linux/compiler.h>
+#include <linux/hugetlb.h>
 #include <linux/fs.h>
 #include <linux/shmem_fs.h>
 #include <linux/types.h>
@@ -37,12 +38,12 @@ 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).
+	 * Only allow MTE on anonymous, shmem and hugetlb mappings as these
+	 * are guaranteed to be backed by tags-capable memory.
 	 */
 	if (system_supports_mte() &&
-	    ((flags & (MAP_ANONYMOUS | MAP_HUGETLB)) || shmem_file(file)))
+	    ((flags & (MAP_ANONYMOUS | MAP_HUGETLB)) || shmem_file(file) ||
+	     (file && is_file_hugepages(file))))
 		return VM_MTE_ALLOWED;

 	return 0;
diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index f26b3b53d7de..5cf327337e22 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -110,7 +110,7 @@ static int hugetlbfs_file_mmap(struct file *file, struct vm_area_struct *vma)
 	 * way when do_mmap unwinds (may be important on powerpc
 	 * and ia64).
 	 */
-	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND | VM_MTE_ALLOWED);
+	vm_flags_set(vma, VM_HUGETLB | VM_DONTEXPAND);
 	vma->vm_ops = &hugetlb_vm_ops;

 	ret = seal_check_write(info->seals, vma);
----------------8<----------------------------------

We still have VM_DATA_DEFAULT_FLAGS but I think this is fine, the flag
is set by the arch code. This is only to allow mprotect(PROT_MTE) on brk
ranges if any user app wants to do that.

I did not specifically require that only the arch code sets
VM_MTE_ALLOWED but I'd expect it to be the case unless we get some
obscure arm-specific driver that wants to allow MTE on mmap for on-chip
memory (very unlikely though).

That 'if' block needs to be split into multiple ones, it becomes harder
to read. shmem_file() does check for !file but is_file_hugepages()
doesn't, might as well put them under the same 'if' block. And thinking
about it, current arm64 code seems broken as it allows
mmap(MAP_ANONYMOUS | MAP_HUGETLB) but it doesn't actually work properly
prior to Yang's patch in -next.

-- 
Catalin