linux-next: Tree for Nov 7
diff mbox

Message ID 20171113151641.yfqrecpcxllpn5mq@dhcp22.suse.cz
State New
Headers show

Commit Message

Michal Hocko Nov. 13, 2017, 3:16 p.m. UTC
On Mon 13-11-17 13:00:57, Michal Hocko wrote:
[...]
> Yes, I have mentioned that in the previous email but the amount of code
> would be even larger. Basically every arch which reimplements
> arch_get_unmapped_area would have to special case new MAP_FIXED flag to
> do vma lookup.

It turned out that this might be much more easier than I thought after
all. It seems we can really handle that in the common code. This would
mean that we are exposing a new functionality to the userspace though.
Myabe this would be useful on its own though. Just a quick draft (not
even compile tested) whether this makes sense in general. I would be
worried about unexpected behavior when somebody set other bit without a
good reason and we might fail with ENOMEM for such a call now.

Elf loader would then use MAP_FIXED_SAFE rather than MAP_FIXED.
---

Comments

Russell King - ARM Linux admin Nov. 13, 2017, 3:48 p.m. UTC | #1
On Mon, Nov 13, 2017 at 04:16:41PM +0100, Michal Hocko wrote:
> On Mon 13-11-17 13:00:57, Michal Hocko wrote:
> [...]
> > Yes, I have mentioned that in the previous email but the amount of code
> > would be even larger. Basically every arch which reimplements
> > arch_get_unmapped_area would have to special case new MAP_FIXED flag to
> > do vma lookup.
> 
> It turned out that this might be much more easier than I thought after
> all. It seems we can really handle that in the common code. This would
> mean that we are exposing a new functionality to the userspace though.
> Myabe this would be useful on its own though. Just a quick draft (not
> even compile tested) whether this makes sense in general. I would be
> worried about unexpected behavior when somebody set other bit without a
> good reason and we might fail with ENOMEM for such a call now.
> 
> Elf loader would then use MAP_FIXED_SAFE rather than MAP_FIXED.
> ---
> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> index 3b26cc62dadb..d021c21f9b01 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -31,6 +31,9 @@
>  #define MAP_STACK	0x80000		/* give out an address that is best suited for process/thread stacks */
>  #define MAP_HUGETLB	0x100000	/* create a huge page mapping */
>  
> +#define MAP_KEEP_MAPPING 0x2000000
> +#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */

A few things...

1. Does this need to be exposed to userland?
2. Can it end up in include/uapi/asm-generic/mman*.h ?
3. The definition of MAP_FIXED_SAFE should really have parens around it.

> @@ -1365,6 +1365,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>  	if (offset_in_page(addr))
>  		return addr;
>  
> +	if ((flags & MAP_FIXED_SAFE) == MAP_FIXED_SAFE) {

I'm surprised this doesn't warn - since this effectively expands to:

	flags & MAP_FIXED | MAP_KEEP_MAPPING

hence why MAP_FIXED_SAFE needs parens.
Michal Hocko Nov. 13, 2017, 3:49 p.m. UTC | #2
On Mon 13-11-17 16:16:41, Michal Hocko wrote:
> On Mon 13-11-17 13:00:57, Michal Hocko wrote:
> [...]
> > Yes, I have mentioned that in the previous email but the amount of code
> > would be even larger. Basically every arch which reimplements
> > arch_get_unmapped_area would have to special case new MAP_FIXED flag to
> > do vma lookup.
> 
> It turned out that this might be much more easier than I thought after
> all. It seems we can really handle that in the common code. This would
> mean that we are exposing a new functionality to the userspace though.
> Myabe this would be useful on its own though. Just a quick draft (not
> even compile tested) whether this makes sense in general. I would be
> worried about unexpected behavior when somebody set other bit without a
> good reason and we might fail with ENOMEM for such a call now.

Hmm, the bigger problem would be the backward compatibility actually. We
would get silent corruptions which is exactly what the flag is trying
fix. mmap flags handling really sucks. So I guess we would have to make
the flag internal only :/
Michal Hocko Nov. 13, 2017, 3:59 p.m. UTC | #3
On Mon 13-11-17 15:48:13, Russell King - ARM Linux wrote:
> On Mon, Nov 13, 2017 at 04:16:41PM +0100, Michal Hocko wrote:
> > On Mon 13-11-17 13:00:57, Michal Hocko wrote:
> > [...]
> > > Yes, I have mentioned that in the previous email but the amount of code
> > > would be even larger. Basically every arch which reimplements
> > > arch_get_unmapped_area would have to special case new MAP_FIXED flag to
> > > do vma lookup.
> > 
> > It turned out that this might be much more easier than I thought after
> > all. It seems we can really handle that in the common code. This would
> > mean that we are exposing a new functionality to the userspace though.
> > Myabe this would be useful on its own though. Just a quick draft (not
> > even compile tested) whether this makes sense in general. I would be
> > worried about unexpected behavior when somebody set other bit without a
> > good reason and we might fail with ENOMEM for such a call now.
> > 
> > Elf loader would then use MAP_FIXED_SAFE rather than MAP_FIXED.
> > ---
> > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> > index 3b26cc62dadb..d021c21f9b01 100644
> > --- a/arch/alpha/include/uapi/asm/mman.h
> > +++ b/arch/alpha/include/uapi/asm/mman.h
> > @@ -31,6 +31,9 @@
> >  #define MAP_STACK	0x80000		/* give out an address that is best suited for process/thread stacks */
> >  #define MAP_HUGETLB	0x100000	/* create a huge page mapping */
> >  
> > +#define MAP_KEEP_MAPPING 0x2000000
> > +#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
> 
> A few things...
> 
> 1. Does this need to be exposed to userland?

As I've written in another email, exposing the flag this way would be
really dangerous wrt. backward compatibility. So we would either need some
translation or make it a flag on its own and touch the arch specific
code which I really wanted to prevent from.

Whether this is something useful for the userspace is a separate
question which I should bring up to linux-api for a wider audience to
discuss.

So I guess this goes down to whether we want/need something like
MAP_FIXED_SAFE or opt out the specific hardening code for arches that
cannot make unaligned mappings for the requested address.

> 2. Can it end up in include/uapi/asm-generic/mman*.h ?
> 3. The definition of MAP_FIXED_SAFE should really have parens around it.

Of course. I thought I did...

> > @@ -1365,6 +1365,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> >  	if (offset_in_page(addr))
> >  		return addr;
> >  
> > +	if ((flags & MAP_FIXED_SAFE) == MAP_FIXED_SAFE) {
> 
> I'm surprised this doesn't warn - since this effectively expands to:
> 
> 	flags & MAP_FIXED | MAP_KEEP_MAPPING
> 
> hence why MAP_FIXED_SAFE needs parens.

It sure does.

Thanks!
Michael Ellerman Nov. 14, 2017, 9:02 a.m. UTC | #4
Michal Hocko <mhocko@kernel.org> writes:

> On Mon 13-11-17 13:00:57, Michal Hocko wrote:
> [...]
>> Yes, I have mentioned that in the previous email but the amount of code
>> would be even larger. Basically every arch which reimplements
>> arch_get_unmapped_area would have to special case new MAP_FIXED flag to
>> do vma lookup.
>
> It turned out that this might be much more easier than I thought after
> all. It seems we can really handle that in the common code.

Ah nice. I should have read this before replying to your previous mail.

> This would mean that we are exposing a new functionality to the userspace though.
> Myabe this would be useful on its own though.

Yes I think it would. At least jemalloc seems like it could use it:

  https://github.com/jemalloc/jemalloc/blob/9f455e2786685b443201c33119765c8093461174/src/pages.c#L65

And I have memories of some JIT code I read once which did a loop of
mmap()s or something to try and get allocations below 4GB or some other
limit - but I can't remember now what it was.

> Just a quick draft (not
> even compile tested) whether this makes sense in general. I would be
> worried about unexpected behavior when somebody set other bit without a
> good reason and we might fail with ENOMEM for such a call now.
>
> Elf loader would then use MAP_FIXED_SAFE rather than MAP_FIXED.
> ---
> diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> index 3b26cc62dadb..d021c21f9b01 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -31,6 +31,9 @@
>  #define MAP_STACK	0x80000		/* give out an address that is best suited for process/thread stacks */
>  #define MAP_HUGETLB	0x100000	/* create a huge page mapping */
>  
> +#define MAP_KEEP_MAPPING 0x2000000
> +#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */


So bike-shedding a bit, but I think "SAFE" is too vague a name.

Perhaps MAP_NO_CLOBBER - which has the single semantic of "do not
clobber any existing mappings".

It would be a flag on its own, so you could pass it with or without
MAP_FIXED, but it would only change the behaviour when MAP_FIXED is
specified also.

cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
index 3b26cc62dadb..d021c21f9b01 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -31,6 +31,9 @@ 
 #define MAP_STACK	0x80000		/* give out an address that is best suited for process/thread stacks */
 #define MAP_HUGETLB	0x100000	/* create a huge page mapping */
 
+#define MAP_KEEP_MAPPING 0x2000000
+#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
+
 #define MS_ASYNC	1		/* sync memory asynchronously */
 #define MS_SYNC		2		/* synchronous memory sync */
 #define MS_INVALIDATE	4		/* invalidate the caches */
diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
index da3216007fe0..51e3885fbfc1 100644
--- a/arch/mips/include/uapi/asm/mman.h
+++ b/arch/mips/include/uapi/asm/mman.h
@@ -49,6 +49,9 @@ 
 #define MAP_STACK	0x40000		/* give out an address that is best suited for process/thread stacks */
 #define MAP_HUGETLB	0x80000		/* create a huge page mapping */
 
+#define MAP_KEEP_MAPPING 0x2000000
+#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
+
 /*
  * Flags for msync
  */
diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
index cc9ba1d34779..5a4381484fc5 100644
--- a/arch/parisc/include/uapi/asm/mman.h
+++ b/arch/parisc/include/uapi/asm/mman.h
@@ -25,6 +25,9 @@ 
 #define MAP_STACK	0x40000		/* give out an address that is best suited for process/thread stacks */
 #define MAP_HUGETLB	0x80000		/* create a huge page mapping */
 
+#define MAP_KEEP_MAPPING 0x2000000
+#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
+
 #define MS_SYNC		1		/* synchronous memory sync */
 #define MS_ASYNC	2		/* sync memory asynchronously */
 #define MS_INVALIDATE	4		/* invalidate the caches */
diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
index b15b278aa314..5df8a81524da 100644
--- a/arch/xtensa/include/uapi/asm/mman.h
+++ b/arch/xtensa/include/uapi/asm/mman.h
@@ -62,6 +62,9 @@ 
 # define MAP_UNINITIALIZED 0x0		/* Don't support this flag */
 #endif
 
+#define MAP_KEEP_MAPPING 0x2000000
+#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
+
 /*
  * Flags for msync
  */
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index 203268f9231e..22442846f5c8 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -25,6 +25,9 @@ 
 # define MAP_UNINITIALIZED 0x0		/* Don't support this flag */
 #endif
 
+#define MAP_KEEP_MAPPING 0x2000000
+#define MAP_FIXED_SAFE	MAP_FIXED|MAP_KEEP_MAPPING /* enforce MAP_FIXED without clobbering an existing mapping */
+
 /*
  * Flags for mlock
  */
diff --git a/mm/mmap.c b/mm/mmap.c
index 680506faceae..e53b6b15a8d9 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1365,6 +1365,13 @@  unsigned long do_mmap(struct file *file, unsigned long addr,
 	if (offset_in_page(addr))
 		return addr;
 
+	if ((flags & MAP_FIXED_SAFE) == MAP_FIXED_SAFE) {
+		struct vm_area_struct *vma = find_vma(mm, addr);
+
+		if (vma && vma->vm_start <= addr)
+			return -ENOMEM;
+	}
+
 	if (prot == PROT_EXEC) {
 		pkey = execute_only_pkey(mm);
 		if (pkey < 0)