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 <j...@suse.cz>
> Cc: Arnd Bergmann <a...@arndb.de>
> Cc: Andy Lutomirski <l...@kernel.org>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Suggested-by: Christoph Hellwig <h...@lst.de>
> Suggested-by: Linus Torvalds <torva...@linux-foundation.org>
> Signed-off-by: Dan Williams <dan.j.willi...@intel.com>
> ---
> 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).
> 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. 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 <j...@suse.cz>
Honza
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
