| Message ID | ea10a05be83c70cf610d470fe3b4c05513e3859c.1730206735.git.lorenzo.stoakes@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix error handling in mmap_region() and refactor (hotfixes) | expand |
* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [241029 09:04]: > Currently MTE is permitted in two circumstances (desiring to use MTE having > been specified by the VM_MTE flag) - where MAP_ANONYMOUS is specified, as > checked by arch_calc_vm_flag_bits() and actualised by setting the > VM_MTE_ALLOWED flag, or if the file backing the mapping is shmem, in which > case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap hook is activated > in mmap_region(). > > The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also set > is the arm64 implementation of arch_validate_flags(). > > Unfortunately, we intend to refactor mmap_region() to perform this check > earlier, meaning that in the case of a shmem backing we will not have > invoked shmem_mmap() yet, causing the mapping to fail spuriously. > > It is inappropriate to set this architecture-specific flag in general mm > code anyway, so a sensible resolution of this issue is to instead move the > check to arch_validate_flags() itself. > > This requires a modification to the arch_validate_flags() signature to pass > in a pointer to the struct file associated with the mapping, however this > is not too egregious as this is only used by two architectures anyway - > arm64 and sparc. > > So this patch performs this adjustment and moves the check to > arch_validate_flags() which resolves the issue. > > 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. > > So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if > MAP_ANON. > > Reported-by: Jann Horn <jannh@google.com> > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails") > Cc: stable <stable@kernel.org> > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Tested-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com> > --- > arch/arm64/include/asm/mman.h | 29 +++++++++++++++++++++++++---- > arch/sparc/include/asm/mman.h | 5 +++-- > include/linux/mman.h | 2 +- > mm/mmap.c | 2 +- > mm/mprotect.c | 2 +- > mm/shmem.c | 3 --- > 6 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h > index 9e39217b4afb..94925a0482e3 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -6,7 +6,9 @@ > > #ifndef BUILD_VDSO > #include <linux/compiler.h> > +#include <linux/fs.h> > #include <linux/types.h> > +#include <linux/shmem_fs.h> > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, > unsigned long pkey) > @@ -60,15 +62,34 @@ static inline bool arch_validate_prot(unsigned long prot, > } > #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) > > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > if (!system_supports_mte()) > return true; > > - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ > - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); > + if (!(vm_flags & VM_MTE)) > + return true; > + > + /* > + * We explicitly permit MAP_ANONYMOUS and shmem backed mappings to use > + * MTE. We check for shmem explicitly here. > + * > + * The MAP_ANONYMOUS case is handled by arch_calc_vm_flag_bits() which > + * explicitly sets VM_MTE_ALLOWED which we check for here also. > + * > + * Ideally we'd perform both checks here but existing semantics support > + * hugetlb in MAP_ANONYMOUS mode but not a MAP_SHARED mapping, which is > + * likely unintended but we maintain identical behaviour for > + * consistency. > + */ > + if (vm_flags & VM_MTE_ALLOWED) > + return true; > + if (shmem_file(file)) > + return true; > + > + return false; > } > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > > #endif /* !BUILD_VDSO */ > > diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h > index af9c10c83dc5..d426e1f7c2c1 100644 > --- a/arch/sparc/include/asm/mman.h > +++ b/arch/sparc/include/asm/mman.h > @@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long len); > > #ifdef CONFIG_SPARC64 > #include <asm/adi_64.h> > +#include <linux/fs.h> > > static inline void ipi_set_tstate_mcde(void *arg) > { > @@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr) > return 1; > } > > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > /* arch_validate_flags() - Ensure combination of flags is valid for a > * VMA. > */ > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > /* If ADI is being enabled on this VMA, check for ADI > * capability on the platform and ensure VMA is suitable > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 8ddca62d6460..82e6488026b7 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long prot, unsigned long addr) > * > * Returns true if the VM_* flags are valid. > */ > -static inline bool arch_validate_flags(unsigned long flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long flags) > { > return true; > } > diff --git a/mm/mmap.c b/mm/mmap.c > index ab71d4c3464c..40b5858ae875 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1511,7 +1511,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > } > > /* Allow architectures to sanity-check the vm_flags */ > - if (!arch_validate_flags(vma->vm_flags)) { > + if (!arch_validate_flags(file, vma->vm_flags)) { > error = -EINVAL; > goto close_and_free_vma; > } > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 6f450af3252e..c6db98b893fc 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > } > > /* Allow architectures to sanity-check the new flags */ > - if (!arch_validate_flags(newflags)) { > + if (!arch_validate_flags(vma->vm_file, newflags)) { > error = -EINVAL; > break; > } > 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); > - > file_accessed(file); > /* This is anonymous shared memory if it is unlinked at the time of mmap */ > if (inode->i_nlink) > -- > 2.47.0
Andrew - could we modify this patch please? I'd like to not include the change to mm/shmem.c. Sorry to be a pain! On Tue, Oct 29, 2024 at 01:03:46PM +0000, Lorenzo Stoakes wrote: > Currently MTE is permitted in two circumstances (desiring to use MTE having > been specified by the VM_MTE flag) - where MAP_ANONYMOUS is specified, as > checked by arch_calc_vm_flag_bits() and actualised by setting the > VM_MTE_ALLOWED flag, or if the file backing the mapping is shmem, in which > case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap hook is activated > in mmap_region(). > > The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also set > is the arm64 implementation of arch_validate_flags(). > > Unfortunately, we intend to refactor mmap_region() to perform this check > earlier, meaning that in the case of a shmem backing we will not have > invoked shmem_mmap() yet, causing the mapping to fail spuriously. > > It is inappropriate to set this architecture-specific flag in general mm > code anyway, so a sensible resolution of this issue is to instead move the > check to arch_validate_flags() itself. > > This requires a modification to the arch_validate_flags() signature to pass > in a pointer to the struct file associated with the mapping, however this > is not too egregious as this is only used by two architectures anyway - > arm64 and sparc. > > So this patch performs this adjustment and moves the check to > arch_validate_flags() which resolves the issue. > > 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. > > So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if > MAP_ANON. > > Reported-by: Jann Horn <jannh@google.com> > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails") > Cc: stable <stable@kernel.org> > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Tested-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > arch/arm64/include/asm/mman.h | 29 +++++++++++++++++++++++++---- > arch/sparc/include/asm/mman.h | 5 +++-- > include/linux/mman.h | 2 +- > mm/mmap.c | 2 +- > mm/mprotect.c | 2 +- > mm/shmem.c | 3 --- > 6 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h > index 9e39217b4afb..94925a0482e3 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -6,7 +6,9 @@ > > #ifndef BUILD_VDSO > #include <linux/compiler.h> > +#include <linux/fs.h> > #include <linux/types.h> > +#include <linux/shmem_fs.h> > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, > unsigned long pkey) > @@ -60,15 +62,34 @@ static inline bool arch_validate_prot(unsigned long prot, > } > #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) > > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > if (!system_supports_mte()) > return true; > > - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ > - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); > + if (!(vm_flags & VM_MTE)) > + return true; > + > + /* > + * We explicitly permit MAP_ANONYMOUS and shmem backed mappings to use > + * MTE. We check for shmem explicitly here. > + * > + * The MAP_ANONYMOUS case is handled by arch_calc_vm_flag_bits() which > + * explicitly sets VM_MTE_ALLOWED which we check for here also. > + * > + * Ideally we'd perform both checks here but existing semantics support > + * hugetlb in MAP_ANONYMOUS mode but not a MAP_SHARED mapping, which is > + * likely unintended but we maintain identical behaviour for > + * consistency. > + */ > + if (vm_flags & VM_MTE_ALLOWED) > + return true; > + if (shmem_file(file)) > + return true; > + > + return false; > } > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > > #endif /* !BUILD_VDSO */ > > diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h > index af9c10c83dc5..d426e1f7c2c1 100644 > --- a/arch/sparc/include/asm/mman.h > +++ b/arch/sparc/include/asm/mman.h > @@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long len); > > #ifdef CONFIG_SPARC64 > #include <asm/adi_64.h> > +#include <linux/fs.h> > > static inline void ipi_set_tstate_mcde(void *arg) > { > @@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr) > return 1; > } > > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > /* arch_validate_flags() - Ensure combination of flags is valid for a > * VMA. > */ > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > /* If ADI is being enabled on this VMA, check for ADI > * capability on the platform and ensure VMA is suitable > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 8ddca62d6460..82e6488026b7 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long prot, unsigned long addr) > * > * Returns true if the VM_* flags are valid. > */ > -static inline bool arch_validate_flags(unsigned long flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long flags) > { > return true; > } > diff --git a/mm/mmap.c b/mm/mmap.c > index ab71d4c3464c..40b5858ae875 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1511,7 +1511,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > } > > /* Allow architectures to sanity-check the vm_flags */ > - if (!arch_validate_flags(vma->vm_flags)) { > + if (!arch_validate_flags(file, vma->vm_flags)) { > error = -EINVAL; > goto close_and_free_vma; > } > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 6f450af3252e..c6db98b893fc 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > } > > /* Allow architectures to sanity-check the new flags */ > - if (!arch_validate_flags(newflags)) { > + if (!arch_validate_flags(vma->vm_file, newflags)) { > error = -EINVAL; > break; > } > 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); > - Andrew - We don't want to include this in change to mm/shmem.c in this series as it turns out we always need to set this flag, hence the need to not include this part of the patch. Thanks! > file_accessed(file); > /* This is anonymous shared memory if it is unlinked at the time of mmap */ > if (inode->i_nlink) > -- > 2.47.0
On Tue, Oct 29, 2024 at 6:04 AM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > Currently MTE is permitted in two circumstances (desiring to use MTE having > been specified by the VM_MTE flag) - where MAP_ANONYMOUS is specified, as > checked by arch_calc_vm_flag_bits() and actualised by setting the > VM_MTE_ALLOWED flag, or if the file backing the mapping is shmem, in which > case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap hook is activated > in mmap_region(). > > The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also set > is the arm64 implementation of arch_validate_flags(). > > Unfortunately, we intend to refactor mmap_region() to perform this check > earlier, meaning that in the case of a shmem backing we will not have > invoked shmem_mmap() yet, causing the mapping to fail spuriously. > > It is inappropriate to set this architecture-specific flag in general mm > code anyway, so a sensible resolution of this issue is to instead move the > check to arch_validate_flags() itself. > > This requires a modification to the arch_validate_flags() signature to pass > in a pointer to the struct file associated with the mapping, however this > is not too egregious as this is only used by two architectures anyway - > arm64 and sparc. > > So this patch performs this adjustment and moves the check to > arch_validate_flags() which resolves the issue. > > 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. > > So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if > MAP_ANON. > > Reported-by: Jann Horn <jannh@google.com> > Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails") > Cc: stable <stable@kernel.org> > Acked-by: Vlastimil Babka <vbabka@suse.cz> > Tested-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > arch/arm64/include/asm/mman.h | 29 +++++++++++++++++++++++++---- > arch/sparc/include/asm/mman.h | 5 +++-- > include/linux/mman.h | 2 +- > mm/mmap.c | 2 +- > mm/mprotect.c | 2 +- > mm/shmem.c | 3 --- > 6 files changed, 31 insertions(+), 12 deletions(-) > > diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h > index 9e39217b4afb..94925a0482e3 100644 > --- a/arch/arm64/include/asm/mman.h > +++ b/arch/arm64/include/asm/mman.h > @@ -6,7 +6,9 @@ > > #ifndef BUILD_VDSO > #include <linux/compiler.h> > +#include <linux/fs.h> > #include <linux/types.h> > +#include <linux/shmem_fs.h> > > static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, > unsigned long pkey) > @@ -60,15 +62,34 @@ static inline bool arch_validate_prot(unsigned long prot, > } > #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) > > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > if (!system_supports_mte()) > return true; > > - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ > - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); > + if (!(vm_flags & VM_MTE)) > + return true; > + > + /* > + * We explicitly permit MAP_ANONYMOUS and shmem backed mappings to use > + * MTE. We check for shmem explicitly here. > + * > + * The MAP_ANONYMOUS case is handled by arch_calc_vm_flag_bits() which > + * explicitly sets VM_MTE_ALLOWED which we check for here also. > + * > + * Ideally we'd perform both checks here but existing semantics support > + * hugetlb in MAP_ANONYMOUS mode but not a MAP_SHARED mapping, which is > + * likely unintended but we maintain identical behaviour for > + * consistency. > + */ > + if (vm_flags & VM_MTE_ALLOWED) > + return true; > + if (shmem_file(file)) > + return true; Sorry for chiming in late. This looks problematic IIUC. The patch 5/5 moved arch_validate_flags() before call_mmap() as the commit log said. But shmem_file() does this check: mapping->a_ops == &shmem_aops But mapping->a_ops is not initialized until shmem_mmap(), which is called by call_mmap(). So shmem_file() should always return false here. Did I miss something? > + > + return false; > } > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > > #endif /* !BUILD_VDSO */ > > diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h > index af9c10c83dc5..d426e1f7c2c1 100644 > --- a/arch/sparc/include/asm/mman.h > +++ b/arch/sparc/include/asm/mman.h > @@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long len); > > #ifdef CONFIG_SPARC64 > #include <asm/adi_64.h> > +#include <linux/fs.h> > > static inline void ipi_set_tstate_mcde(void *arg) > { > @@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr) > return 1; > } > > -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) > +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) > /* arch_validate_flags() - Ensure combination of flags is valid for a > * VMA. > */ > -static inline bool arch_validate_flags(unsigned long vm_flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) > { > /* If ADI is being enabled on this VMA, check for ADI > * capability on the platform and ensure VMA is suitable > diff --git a/include/linux/mman.h b/include/linux/mman.h > index 8ddca62d6460..82e6488026b7 100644 > --- a/include/linux/mman.h > +++ b/include/linux/mman.h > @@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long prot, unsigned long addr) > * > * Returns true if the VM_* flags are valid. > */ > -static inline bool arch_validate_flags(unsigned long flags) > +static inline bool arch_validate_flags(struct file *file, unsigned long flags) > { > return true; > } > diff --git a/mm/mmap.c b/mm/mmap.c > index ab71d4c3464c..40b5858ae875 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1511,7 +1511,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > } > > /* Allow architectures to sanity-check the vm_flags */ > - if (!arch_validate_flags(vma->vm_flags)) { > + if (!arch_validate_flags(file, vma->vm_flags)) { > error = -EINVAL; > goto close_and_free_vma; > } > diff --git a/mm/mprotect.c b/mm/mprotect.c > index 6f450af3252e..c6db98b893fc 100644 > --- a/mm/mprotect.c > +++ b/mm/mprotect.c > @@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, > } > > /* Allow architectures to sanity-check the new flags */ > - if (!arch_validate_flags(newflags)) { > + if (!arch_validate_flags(vma->vm_file, newflags)) { > error = -EINVAL; > break; > } > 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); > - > file_accessed(file); > /* This is anonymous shared memory if it is unlinked at the time of mmap */ > if (inode->i_nlink) > -- > 2.47.0 >
On Tue, Oct 29, 2024 at 08:43:04AM -0700, Yang Shi wrote: > > Sorry for chiming in late. This looks problematic IIUC. The patch 5/5 > moved arch_validate_flags() before call_mmap() as the commit log said. > But shmem_file() does this check: > mapping->a_ops == &shmem_aops > > But mapping->a_ops is not initialized until shmem_mmap(), which is > called by call_mmap(). > > So shmem_file() should always return false here. Did I miss something? Well firstly this is tested and works so you did miss something :) Bu shmem_mmap() is not setting mapping->a_ops, it's setting vma->vm_ops. __shmem_get_inode() and shmem_symlink() set mapping->a_ops.
On Tue, Oct 29, 2024 at 8:52 AM Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > On Tue, Oct 29, 2024 at 08:43:04AM -0700, Yang Shi wrote: > > > > > Sorry for chiming in late. This looks problematic IIUC. The patch 5/5 > > moved arch_validate_flags() before call_mmap() as the commit log said. > > But shmem_file() does this check: > > mapping->a_ops == &shmem_aops > > > > But mapping->a_ops is not initialized until shmem_mmap(), which is > > called by call_mmap(). > > > > So shmem_file() should always return false here. Did I miss something? > > Well firstly this is tested and works so you did miss something :) > > Bu shmem_mmap() is not setting mapping->a_ops, it's setting vma->vm_ops. > > __shmem_get_inode() and shmem_symlink() set mapping->a_ops. My bad, stared at "_ops", but missed "vm" prefix. And shmem_mmap() can't touch a_ops. Sorry for the disruption.
On Tue, Oct 29, 2024 at 09:08:22AM -0700, Yang Shi wrote: > On Tue, Oct 29, 2024 at 8:52 AM Lorenzo Stoakes > <lorenzo.stoakes@oracle.com> wrote: > > > > On Tue, Oct 29, 2024 at 08:43:04AM -0700, Yang Shi wrote: > > > > > > > > Sorry for chiming in late. This looks problematic IIUC. The patch 5/5 > > > moved arch_validate_flags() before call_mmap() as the commit log said. > > > But shmem_file() does this check: > > > mapping->a_ops == &shmem_aops > > > > > > But mapping->a_ops is not initialized until shmem_mmap(), which is > > > called by call_mmap(). > > > > > > So shmem_file() should always return false here. Did I miss something? > > > > Well firstly this is tested and works so you did miss something :) > > > > Bu shmem_mmap() is not setting mapping->a_ops, it's setting vma->vm_ops. > > > > __shmem_get_inode() and shmem_symlink() set mapping->a_ops. > > My bad, stared at "_ops", but missed "vm" prefix. And shmem_mmap() > can't touch a_ops. Sorry for the disruption. No worries, the various ops are quite confusing and precisely where what happens, I had to double check this myself... :)
diff --git a/arch/arm64/include/asm/mman.h b/arch/arm64/include/asm/mman.h index 9e39217b4afb..94925a0482e3 100644 --- a/arch/arm64/include/asm/mman.h +++ b/arch/arm64/include/asm/mman.h @@ -6,7 +6,9 @@ #ifndef BUILD_VDSO #include <linux/compiler.h> +#include <linux/fs.h> #include <linux/types.h> +#include <linux/shmem_fs.h> static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot, unsigned long pkey) @@ -60,15 +62,34 @@ static inline bool arch_validate_prot(unsigned long prot, } #define arch_validate_prot(prot, addr) arch_validate_prot(prot, addr) -static inline bool arch_validate_flags(unsigned long vm_flags) +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) { if (!system_supports_mte()) return true; - /* only allow VM_MTE if VM_MTE_ALLOWED has been set previously */ - return !(vm_flags & VM_MTE) || (vm_flags & VM_MTE_ALLOWED); + if (!(vm_flags & VM_MTE)) + return true; + + /* + * We explicitly permit MAP_ANONYMOUS and shmem backed mappings to use + * MTE. We check for shmem explicitly here. + * + * The MAP_ANONYMOUS case is handled by arch_calc_vm_flag_bits() which + * explicitly sets VM_MTE_ALLOWED which we check for here also. + * + * Ideally we'd perform both checks here but existing semantics support + * hugetlb in MAP_ANONYMOUS mode but not a MAP_SHARED mapping, which is + * likely unintended but we maintain identical behaviour for + * consistency. + */ + if (vm_flags & VM_MTE_ALLOWED) + return true; + if (shmem_file(file)) + return true; + + return false; } -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) #endif /* !BUILD_VDSO */ diff --git a/arch/sparc/include/asm/mman.h b/arch/sparc/include/asm/mman.h index af9c10c83dc5..d426e1f7c2c1 100644 --- a/arch/sparc/include/asm/mman.h +++ b/arch/sparc/include/asm/mman.h @@ -10,6 +10,7 @@ int sparc_mmap_check(unsigned long addr, unsigned long len); #ifdef CONFIG_SPARC64 #include <asm/adi_64.h> +#include <linux/fs.h> static inline void ipi_set_tstate_mcde(void *arg) { @@ -54,11 +55,11 @@ static inline int sparc_validate_prot(unsigned long prot, unsigned long addr) return 1; } -#define arch_validate_flags(vm_flags) arch_validate_flags(vm_flags) +#define arch_validate_flags(file, vm_flags) arch_validate_flags(file, vm_flags) /* arch_validate_flags() - Ensure combination of flags is valid for a * VMA. */ -static inline bool arch_validate_flags(unsigned long vm_flags) +static inline bool arch_validate_flags(struct file *file, unsigned long vm_flags) { /* If ADI is being enabled on this VMA, check for ADI * capability on the platform and ensure VMA is suitable diff --git a/include/linux/mman.h b/include/linux/mman.h index 8ddca62d6460..82e6488026b7 100644 --- a/include/linux/mman.h +++ b/include/linux/mman.h @@ -117,7 +117,7 @@ static inline bool arch_validate_prot(unsigned long prot, unsigned long addr) * * Returns true if the VM_* flags are valid. */ -static inline bool arch_validate_flags(unsigned long flags) +static inline bool arch_validate_flags(struct file *file, unsigned long flags) { return true; } diff --git a/mm/mmap.c b/mm/mmap.c index ab71d4c3464c..40b5858ae875 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1511,7 +1511,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr, } /* Allow architectures to sanity-check the vm_flags */ - if (!arch_validate_flags(vma->vm_flags)) { + if (!arch_validate_flags(file, vma->vm_flags)) { error = -EINVAL; goto close_and_free_vma; } diff --git a/mm/mprotect.c b/mm/mprotect.c index 6f450af3252e..c6db98b893fc 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -816,7 +816,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len, } /* Allow architectures to sanity-check the new flags */ - if (!arch_validate_flags(newflags)) { + if (!arch_validate_flags(vma->vm_file, newflags)) { error = -EINVAL; break; } 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); - file_accessed(file); /* This is anonymous shared memory if it is unlinked at the time of mmap */ if (inode->i_nlink)
Currently MTE is permitted in two circumstances (desiring to use MTE having been specified by the VM_MTE flag) - where MAP_ANONYMOUS is specified, as checked by arch_calc_vm_flag_bits() and actualised by setting the VM_MTE_ALLOWED flag, or if the file backing the mapping is shmem, in which case we set VM_MTE_ALLOWED in shmem_mmap() when the mmap hook is activated in mmap_region(). The function that checks that, if VM_MTE is set, VM_MTE_ALLOWED is also set is the arm64 implementation of arch_validate_flags(). Unfortunately, we intend to refactor mmap_region() to perform this check earlier, meaning that in the case of a shmem backing we will not have invoked shmem_mmap() yet, causing the mapping to fail spuriously. It is inappropriate to set this architecture-specific flag in general mm code anyway, so a sensible resolution of this issue is to instead move the check to arch_validate_flags() itself. This requires a modification to the arch_validate_flags() signature to pass in a pointer to the struct file associated with the mapping, however this is not too egregious as this is only used by two architectures anyway - arm64 and sparc. So this patch performs this adjustment and moves the check to arch_validate_flags() which resolves the issue. 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. So continue to check VM_MTE_ALLOWED which arch_calc_vm_flag_bits() sets if MAP_ANON. Reported-by: Jann Horn <jannh@google.com> Fixes: deb0f6562884 ("mm/mmap: undo ->mmap() when arch_validate_flags() fails") Cc: stable <stable@kernel.org> Acked-by: Vlastimil Babka <vbabka@suse.cz> Tested-by: Mark Brown <broonie@kernel.org> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- arch/arm64/include/asm/mman.h | 29 +++++++++++++++++++++++++---- arch/sparc/include/asm/mman.h | 5 +++-- include/linux/mman.h | 2 +- mm/mmap.c | 2 +- mm/mprotect.c | 2 +- mm/shmem.c | 3 --- 6 files changed, 31 insertions(+), 12 deletions(-) -- 2.47.0