On Wed, Oct 11, 2017 at 12:43 AM, Jan Kara <jack@xxxxxxx> wrote: > On Tue 10-10-17 07:49:01, Dan Williams wrote: >> The mmap(2) syscall suffers from the ABI anti-pattern of not validating >> unknown flags. However, proposals like MAP_SYNC and MAP_DIRECT need a >> mechanism to define new behavior that is known to fail on older kernels >> without the support. Define a new MAP_SHARED_VALIDATE flag pattern that >> is guaranteed to fail on all legacy mmap implementations. >> >> It is worth noting that the original proposal was for a standalone >> MAP_VALIDATE flag. However, when that could not be supported by all >> archs Linus observed: >> >> I see why you *think* you want a bitmap. You think you want >> a bitmap because you want to make MAP_VALIDATE be part of MAP_SYNC >> etc, so that people can do >> >> ret = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED >> | MAP_SYNC, fd, 0); >> >> and "know" that MAP_SYNC actually takes. >> >> And I'm saying that whole wish is bogus. You're fundamentally >> depending on special semantics, just make it explicit. It's already >> not portable, so don't try to make it so. >> >> Rename that MAP_VALIDATE as MAP_SHARED_VALIDATE, make it have a value >> of 0x3, and make people do >> >> ret = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED_VALIDATE >> | MAP_SYNC, fd, 0); >> >> and then the kernel side is easier too (none of that random garbage >> playing games with looking at the "MAP_VALIDATE bit", but just another >> case statement in that map type thing. >> >> Boom. Done. >> >> Similar to ->fallocate() we also want the ability to validate the >> support for new flags on a per ->mmap() 'struct file_operations' >> instance basis. Towards that end arrange for flags to be generically >> validated against a mmap_supported_mask exported by 'struct >> file_operations'. By default all existing flags are implicitly >> supported, but new flags require MAP_SHARED_VALIDATE and >> per-instance-opt-in. >> >> Cc: Jan Kara <jack@xxxxxxx> >> Cc: Arnd Bergmann <arnd@xxxxxxxx> >> Cc: Andy Lutomirski <luto@xxxxxxxxxx> >> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> >> Suggested-by: Christoph Hellwig <hch@xxxxxx> >> Suggested-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx> >> --- >> arch/alpha/include/uapi/asm/mman.h | 1 + >> arch/mips/include/uapi/asm/mman.h | 1 + >> arch/mips/kernel/vdso.c | 2 + >> arch/parisc/include/uapi/asm/mman.h | 1 + >> arch/tile/mm/elf.c | 3 +- >> arch/xtensa/include/uapi/asm/mman.h | 1 + >> include/linux/fs.h | 2 + >> include/linux/mm.h | 2 + >> include/linux/mman.h | 39 ++++++++++++++++++++++++++ >> include/uapi/asm-generic/mman-common.h | 1 + >> mm/mmap.c | 21 ++++++++++++-- >> tools/include/uapi/asm-generic/mman-common.h | 1 + >> 12 files changed, 69 insertions(+), 6 deletions(-) >> >> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h >> index 3b26cc62dadb..92823f24890b 100644 >> --- a/arch/alpha/include/uapi/asm/mman.h >> +++ b/arch/alpha/include/uapi/asm/mman.h >> @@ -14,6 +14,7 @@ >> #define MAP_TYPE 0x0f /* Mask for type of mapping (OSF/1 is _wrong_) */ >> #define MAP_FIXED 0x100 /* Interpret addr exactly */ >> #define MAP_ANONYMOUS 0x10 /* don't use a file */ >> +#define MAP_SHARED_VALIDATE 0x3 /* share + validate extension flags */ > > Just a nit but I'd put definition of MAP_SHARED_VALIDATE close to the > definition of MAP_SHARED and MAP_PRIVATE where it logically belongs (for > all archs). Will do. > >> diff --git a/include/linux/mm.h b/include/linux/mm.h >> index f8c10d336e42..5c4c98e4adc9 100644 >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> @@ -2133,7 +2133,7 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo >> >> 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); >> + struct list_head *uf, unsigned long map_flags); >> 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, > > I have to say I'm not very keen on passing down both vm_flags and map_flags > - vm_flags are almost a subset of map_flags but not quite and the ambiguity > which needs to be used for a particular check seems to open a space for > errors. Granted you currently only care about MAP_DIRECT in ->mmap_validate > and just pass map_flags through mmap_region() so there's no space for > confusion but future checks could do something different. I was hoping the fact that one can't trigger a call to ->mmap_validate() unless they specify a flag outside of LEGACY_MAP_MASK makes it clearer that validation is only for new flags. Old flags get the existing "may be silently ignored" behavior. > But OTOH I don't > see a cleaner way of avoiding the need to allocate vma flag for something > you need to check down in ->mmap_validate so I guess I'll live with that > and if problems really happen, we may have cleaner idea what needs to be > done. > > So overall feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> Thanks Jan.
