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
