mm: update mips to use do_mmap(), make mmap_region() internal
Subsystems (4)
cover mm: update mips to use do_mmap(), make mmap_region() internal
Currently the only user of mmap_region() outside of the memory management code is the MIPS VDSO implementation.
This uses mmap_region() to map a 'delay slot emulation page' at the top of the stack which is read-only and executable.
This mapping requires that an already-acquired mmap write lock is utilised and that uffd and populate logic is ignored. This rules out vm_mmap(), however do_mmap() fits the bill.
Adapt this code to use do_mmap() and then once done, make mmap_region() internal and userland testable, and avoid any other uses of mmap_region(), which is absolutely and strictly an internal mm function which bypasses a great number of checks and logic.
REVIEWERS NOTES:
Thomas - I lack the hardware or set up to test this beyond a simple cross-compilation test, so I need some input from you MIPS guys as to whether this is workable.
I've therefore sent this as an RFC so we can be sure this is suitable! Please could you check to make sure this change is OK and I haven't missed anything?
Thanks!
Lorenzo Stoakes (2):
mips: vdso: prefer do_mmap() to mmap_region()
mm: make mmap_region() internal
arch/mips/kernel/vdso.c | 10 +++-- include/linux/mm.h | 3 -- mm/mmap.c | 34 ----------------- mm/vma.c | 36 +++++++++++++++++- mm/vma.h | 6 +-- tools/testing/vma/vma_internal.h | 65 ++++++++++++++++++++++++++++++++ 6 files changed, 109 insertions(+), 45 deletions(-)
--
2.47.1
RFC PATCH 1/2 mips: vdso: prefer do_mmap() to mmap_region()
mmap_region() is an internal memory management implementation detail that is not intended to be used outside of the memory management subsystem.
Map the delay slot emulation page using do_mmap() which makes use of the already-held mmap write lock and bypasses unneeded populate and userfaultfd logic.
This should have the precise same behaviour as the existing logic.
--- arch/mips/kernel/vdso.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
arch/mips/kernel/vdso.c +6 -4
@@ -11,6 +11,7 @@#include <linux/ioport.h> #include <linux/kernel.h> #include <linux/mm.h> +#include <linux/mman.h> #include <linux/random.h> #include <linux/sched.h> #include <linux/slab.h>
@@ -97,11 +98,12 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)return -EINTR ;if (IS_ENABLED (CONFIG_MIPS_FP_SUPPORT )) { +unsigned long unused; +/* Map delay slot emulation page */ - base =mmap_region (NULL ,STACK_TOP ,PAGE_SIZE , -VM_READ |VM_EXEC | -VM_MAYREAD |VM_MAYWRITE |VM_MAYEXEC , -0 ,NULL ); + base =do_mmap (NULL ,STACK_TOP ,PAGE_SIZE ,PROT_READ |PROT_EXEC , +MAP_ANONYMOUS |MAP_PRIVATE |MAP_FIXED ,0 ,0 , &unused, +NULL );if (IS_ERR_VALUE (base)) { ret = base; goto out;
Signature · 1 line
2.47.1
RFC PATCH 2/2 mm: make mmap_region() internal
In reply to Lorenzo StoakesNow that we have removed the one user of mmap_region() outside of mm, make it internal and add it to vma.c so it can be userland tested.
This ensures that all external memory mappings are performed using the appropriate interfaces and allows us to modify memory mapping logic as we see fit.
Additionally expand test stubs to allow for the mmap_region() code to compile and be userland testable.
--- include/linux/mm.h | 3 -- mm/mmap.c | 34 ----------------- mm/vma.c | 36 +++++++++++++++++- mm/vma.h | 6 +-- tools/testing/vma/vma_internal.h | 65 ++++++++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 41 deletions(-)
include/linux/mm.h +0 -3
@@ -3436,9 +3436,6 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,return __get_unmapped_area (file, addr, len, pgoff, flags,0 ); } -extern unsigned long mmap_region (struct file *file,unsigned long addr, -unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, -struct list_head *uf); extern unsigned long do_mmap(struct file *file, unsigned long addr, unsigned long len, unsigned long prot, unsigned long flags, vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate,
mm/mmap.c +0 -34
@@ -1016,40 +1016,6 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len,return do_vmi_munmap (&vmi, mm, start, len, uf, false); } -unsigned long mmap_region (struct file *file,unsigned long addr, -unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, -struct list_head *uf) -{ -unsigned long ret; -bool writable_file_mapping = false; - -/* Check to see if MDWE is applicable. */ -if (map_deny_write_exec (vm_flags, vm_flags)) -return -EACCES ; - -/* Allow architectures to sanity-check the vm_flags. */ -if (!arch_validate_flags (vm_flags)) -return -EINVAL ; - -/* Map writable and ensure this isn't a sealed memfd. */ -if (file &&is_shared_maywrite (vm_flags)) { -int error =mapping_map_writable (file->f_mapping); - -if (error) -return error; - writable_file_mapping = true; - } - - ret =__mmap_region (file, addr, len, vm_flags, pgoff, uf); - -/* Clear our write mapping regardless of error. */ -if (writable_file_mapping) -mapping_unmap_writable (file->f_mapping); - -validate_mm (current->mm); -return ret; -} -int vm_munmap (unsigned long start,size_t len) {return __vm_munmap (start, len, false);
mm/vma.c +35 -1
@@ -2432,7 +2432,7 @@ static void __mmap_complete(struct mmap_state *map, struct vm_area_struct *vma)vma_set_page_prot (vma); } -unsigned long __mmap_region (struct file *file,unsigned long addr, +static unsigned long __mmap_region (struct file *file,unsigned long addr,unsigned long len,vm_flags_t vm_flags,unsigned long pgoff,struct list_head *uf) {
@@ -2484,6 +2484,40 @@ unsigned long __mmap_region(struct file *file, unsigned long addr,return error; } +unsigned long mmap_region (struct file *file,unsigned long addr, +unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, +struct list_head *uf) +{ +unsigned long ret; +bool writable_file_mapping = false; + +/* Check to see if MDWE is applicable. */ +if (map_deny_write_exec (vm_flags, vm_flags)) +return -EACCES ; + +/* Allow architectures to sanity-check the vm_flags. */ +if (!arch_validate_flags (vm_flags)) +return -EINVAL ; + +/* Map writable and ensure this isn't a sealed memfd. */ +if (file &&is_shared_maywrite (vm_flags)) { +int error =mapping_map_writable (file->f_mapping); + +if (error) +return error; + writable_file_mapping = true; + } + + ret =__mmap_region (file, addr, len, vm_flags, pgoff, uf); + +/* Clear our write mapping regardless of error. */ +if (writable_file_mapping) +mapping_unmap_writable (file->f_mapping); + +validate_mm (current->mm); +return ret; +} +/* * do_brk_flags() - Increase the brk vma if the flags match. * @vmi: The vma iterator
mm/vma.h +3 -3
@@ -242,9 +242,9 @@ bool vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);int mm_take_all_locks (struct mm_struct *mm);void mm_drop_all_locks (struct mm_struct *mm); -unsigned long __mmap_region (struct file *file,unsigned long addr, -unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, -struct list_head *uf); +unsigned long mmap_region (struct file *file,unsigned long addr, +unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, +struct list_head *uf);int do_brk_flags (struct vma_iterator *vmi,struct vm_area_struct *brkvma,unsigned long addr,unsigned long request,unsigned long flags);
tools/testing/vma/vma_internal.h +65 -0
81 lines not on this page · raw message
Signature · 1 line
2.47.1
Re: [RFC PATCH 2/2] mm: make mmap_region() internal
mm/vma.h bool vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot); · line 247
Quoted 11 lines · Lorenzo Stoakes
+++
b/mm/vma.hmm/vma.h +3
-3@@ -242,9 +242,9 @@ bool vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);int mm_take_all_locks (struct mm_struct *mm);void mm_drop_all_locks (struct mm_struct *mm); -unsigned long __mmap_region (struct file *file,unsigned long addr, -unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, -struct list_head *uf); +unsigned long mmap_region (struct file *file,unsigned long addr, +unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, +struct list_head *uf);
Please don't line up the arguments with the paren. Just leave it as two tabs.
Re: [RFC PATCH 2/2] mm: make mmap_region() internal
Quoted 6 lines · Matthew Wilcox
Quoted 5 lines · Lorenzo Stoakes
+++
b/mm/vma.hmm/vma.h +0
-0@@ -242,9 +242,9 @@ bool vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot);int mm_take_all_locks (struct mm_struct *mm);void mm_drop_all_locks (struct mm_struct *mm);
Quoted 10 lines
mm/vma.h bool vma_wants_writenotify(struct vm_area_struct *vma, pgprot_t vm_page_prot); · line 247
Quoted 6 lines
mm/vma.h +3
-3-unsigned long __mmap_region (struct file *file,unsigned long addr, -unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, -struct list_head *uf); +unsigned long mmap_region (struct file *file,unsigned long addr, +unsigned long len,vm_flags_t vm_flags,unsigned long pgoff, +struct list_head *uf);Please don't line up the arguments with the paren. Just leave it as two tabs.
Ack, emacs likes to do this, will tweak on respin assuming mips guys confirm that idea works!
Thomas Bogendoerfer
maintainerQuoted 17 lines · Lorenzo Stoakes
Currently the only user of
mmap_region()outside of the memory management code is the MIPS VDSO implementation.This uses
mmap_region()to map a 'delay slot emulation page' at the top of the stack which is read-only and executable.This mapping requires that an already-acquired mmap write lock is utilised and that uffd and populate logic is ignored. This rules out
vm_mmap(), howeverdo_mmap()fits the bill.Adapt this code to use
do_mmap()and then once done, makemmap_region()internal and userland testable, and avoid any other uses ofmmap_region(), which is absolutely and strictly an internal mm function which bypasses a great number of checks and logic.REVIEWERS NOTES:
Quoted 7 lines
Thomas - I lack the hardware or set up to test this beyond a simple cross-compilation test, so I need some input from you MIPS guys as to whether this is workable.
I've therefore sent this as an RFC so we can be sure this is suitable! Please could you check to make sure this change is OK and I haven't missed anything?
conversation looks correct, but this patch doesn't apply to upstream v6.13-rc1. Which tree are you using ?
Thomas.
Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a good idea. [ RFC1925, 2.3 ]
Quoted 22 lines · Thomas Bogendoerfer
Quoted 21 lines · Lorenzo Stoakes
Currently the only user of
mmap_region()outside of the memory management code is the MIPS VDSO implementation.This uses
mmap_region()to map a 'delay slot emulation page' at the top of the stack which is read-only and executable.This mapping requires that an already-acquired mmap write lock is utilised and that uffd and populate logic is ignored. This rules out
vm_mmap(), howeverdo_mmap()fits the bill.Adapt this code to use
do_mmap()and then once done, makemmap_region()internal and userland testable, and avoid any other uses ofmmap_region(), which is absolutely and strictly an internal mm function which bypasses a great number of checks and logic.REVIEWERS NOTES:
Thomas - I lack the hardware or set up to test this beyond a simple cross-compilation test, so I need some input from you MIPS guys as to whether this is workable.
Quoted 6 lines
Quoted 3 lines
I've therefore sent this as an RFC so we can be sure this is suitable! Please could you check to make sure this change is OK and I haven't missed anything?
conversation looks correct, but this patch doesn't apply to upstream v6.13-rc1. Which tree are you using ?
He's out right now, but generally he works on mm-unstable. It doesn't apply cleanly there either, so my guess is mm-unstable has been rebased.
We should do this, but there's no rush.
Thanks,
Liam
Quoted 22 lines · Thomas Bogendoerfer
Quoted 21 lines · Lorenzo Stoakes
Currently the only user of
mmap_region()outside of the memory management code is the MIPS VDSO implementation.This uses
mmap_region()to map a 'delay slot emulation page' at the top of the stack which is read-only and executable.This mapping requires that an already-acquired mmap write lock is utilised and that uffd and populate logic is ignored. This rules out
vm_mmap(), howeverdo_mmap()fits the bill.Adapt this code to use
do_mmap()and then once done, makemmap_region()internal and userland testable, and avoid any other uses ofmmap_region(), which is absolutely and strictly an internal mm function which bypasses a great number of checks and logic.REVIEWERS NOTES:
Thomas - I lack the hardware or set up to test this beyond a simple cross-compilation test, so I need some input from you MIPS guys as to whether this is workable.
Quoted 6 lines
Quoted 3 lines
I've therefore sent this as an RFC so we can be sure this is suitable! Please could you check to make sure this change is OK and I haven't missed anything?
conversation looks correct, but this patch doesn't apply to upstream v6.13-rc1. Which tree are you using ?
Thanks, yeah mm-unstable as Liam points out but clearly not the tip one :)
Let me rebase and resend. Thanks!
Quoted 7 lines
Thomas.
--
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea. [ RFC1925, 2.3 ]