Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-13 Thread Michal Hocko 
On Wed 13-12-17 04:50:53, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 10:25:49AM +0100, Michal Hocko wrote:
> > +++ b/mm/mmap.c
> > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned 
> > long addr,
> > if (!(file && path_noexec(>f_path)))
> > prot |= PROT_EXEC;
> >  
> > +   /* force arch specific MAP_FIXED handling in get_unmapped_area */
> > +   if (flags & MAP_FIXED_SAFE)
> > +   flags |= MAP_FIXED;
> > +
> > if (!(flags & MAP_FIXED))
> > addr = round_hint_to_min(addr);
> >  
> 
> We're up to 22 MAP_ flags now.  We'll run out soon.  Let's preserve half
> of a flag by giving userspace the definition:
> 
> #define MAP_FIXED_SAFE(MAP_FIXED | _MAP_NOT_HINT)

I've already tried to explain why this cannot be a modifier for
MAP_FIXED. Read about the backward compatibility note...
Or do I misunderstand what you are saying here?
 
> then in here:
> 
>   if ((flags & _MAP_NOT_HINT) && !(flags & MAP_FIXED))
>   return -EINVAL;
> 
> Now we can use _MAP_NOT_HINT all by itself in the future to mean
> something else.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-13 Thread Michal Hocko 
On Wed 13-12-17 04:50:53, Matthew Wilcox wrote:
> On Wed, Dec 13, 2017 at 10:25:49AM +0100, Michal Hocko wrote:
> > +++ b/mm/mmap.c
> > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned 
> > long addr,
> > if (!(file && path_noexec(>f_path)))
> > prot |= PROT_EXEC;
> >  
> > +   /* force arch specific MAP_FIXED handling in get_unmapped_area */
> > +   if (flags & MAP_FIXED_SAFE)
> > +   flags |= MAP_FIXED;
> > +
> > if (!(flags & MAP_FIXED))
> > addr = round_hint_to_min(addr);
> >  
> 
> We're up to 22 MAP_ flags now.  We'll run out soon.  Let's preserve half
> of a flag by giving userspace the definition:
> 
> #define MAP_FIXED_SAFE(MAP_FIXED | _MAP_NOT_HINT)

I've already tried to explain why this cannot be a modifier for
MAP_FIXED. Read about the backward compatibility note...
Or do I misunderstand what you are saying here?
 
> then in here:
> 
>   if ((flags & _MAP_NOT_HINT) && !(flags & MAP_FIXED))
>   return -EINVAL;
> 
> Now we can use _MAP_NOT_HINT all by itself in the future to mean
> something else.

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-13 Thread Matthew Wilcox 
On Wed, Dec 13, 2017 at 10:25:49AM +0100, Michal Hocko wrote:
> +++ b/mm/mmap.c
> @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long 
> addr,
>   if (!(file && path_noexec(>f_path)))
>   prot |= PROT_EXEC;
>  
> + /* force arch specific MAP_FIXED handling in get_unmapped_area */
> + if (flags & MAP_FIXED_SAFE)
> + flags |= MAP_FIXED;
> +
>   if (!(flags & MAP_FIXED))
>   addr = round_hint_to_min(addr);
>  

We're up to 22 MAP_ flags now.  We'll run out soon.  Let's preserve half
of a flag by giving userspace the definition:

#define MAP_FIXED_SAFE  (MAP_FIXED | _MAP_NOT_HINT)

then in here:

if ((flags & _MAP_NOT_HINT) && !(flags & MAP_FIXED))
return -EINVAL;

Now we can use _MAP_NOT_HINT all by itself in the future to mean
something else.

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-13 Thread Matthew Wilcox 
On Wed, Dec 13, 2017 at 10:25:49AM +0100, Michal Hocko wrote:
> +++ b/mm/mmap.c
> @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long 
> addr,
>   if (!(file && path_noexec(>f_path)))
>   prot |= PROT_EXEC;
>  
> + /* force arch specific MAP_FIXED handling in get_unmapped_area */
> + if (flags & MAP_FIXED_SAFE)
> + flags |= MAP_FIXED;
> +
>   if (!(flags & MAP_FIXED))
>   addr = round_hint_to_min(addr);
>  

We're up to 22 MAP_ flags now.  We'll run out soon.  Let's preserve half
of a flag by giving userspace the definition:

#define MAP_FIXED_SAFE  (MAP_FIXED | _MAP_NOT_HINT)

then in here:

if ((flags & _MAP_NOT_HINT) && !(flags & MAP_FIXED))
return -EINVAL;

Now we can use _MAP_NOT_HINT all by itself in the future to mean
something else.

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-07 Thread Pavel Machek 
Hi!

> MAP_FIXED is used quite often to enforce mapping at the particular
> range. The main problem of this flag is, however, that it is inherently
> dangerous because it unmaps existing mappings covered by the requested
> range. This can cause silent memory corruptions. Some of them even with
> serious security implications. While the current semantic might be
> really desiderable in many cases there are others which would want to
> enforce the given range but rather see a failure than a silent memory
> corruption on a clashing range. Please note that there is no guarantee
> that a given range is obeyed by the mmap even when it is free - e.g.
> arch specific code is allowed to apply an alignment.
> 
> Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior.
> It has the same semantic as MAP_FIXED wrt. the given address request

Could we get some better name? Functionality seems reasonable, but
_SAFE suffix does not really explain what is going on to the user.

MAP_ADD_FIXED ?

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-07 Thread Pavel Machek 
Hi!

> MAP_FIXED is used quite often to enforce mapping at the particular
> range. The main problem of this flag is, however, that it is inherently
> dangerous because it unmaps existing mappings covered by the requested
> range. This can cause silent memory corruptions. Some of them even with
> serious security implications. While the current semantic might be
> really desiderable in many cases there are others which would want to
> enforce the given range but rather see a failure than a silent memory
> corruption on a clashing range. Please note that there is no guarantee
> that a given range is obeyed by the mmap even when it is free - e.g.
> arch specific code is allowed to apply an alignment.
> 
> Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior.
> It has the same semantic as MAP_FIXED wrt. the given address request

Could we get some better name? Functionality seems reasonable, but
_SAFE suffix does not really explain what is going on to the user.

MAP_ADD_FIXED ?

Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-06 Thread Michal Hocko 
On Wed 06-12-17 10:27:24, Michal Hocko wrote:
> On Wed 06-12-17 16:15:24, Michael Ellerman wrote:
[...]
> > So I think I proved above that all the arches that are using 0x8 are
> > also using mman-common.h, and vice-versa.
> > 
> > So you can put this in mman-common.h can't your?
> 
> Yes it seems I can. I would have to double check. It is true that
> defining the new flag closer to MAP_FIXED makes some sense

OK, so some recap
those which include generic mman-common.h directly
arch/sparc/include/uapi/asm/mman.h:#include 
arch/powerpc/include/uapi/asm/mman.h:#include 
arch/tile/include/uapi/asm/mman.h:#include 

then we have
arch/metag/include/asm/mman.h
which includes uapi/asm/mman.h and that is a generic one
arch/metag/include/uapi/asm/Kbuild:generic-y += mman.h

others include generic mman.h
arch/arm/include/uapi/asm/mman.h:#include 
arch/frv/include/uapi/asm/mman.h:#include 
arch/ia64/include/uapi/asm/mman.h:#include 
arch/m32r/include/uapi/asm/mman.h:#include 
arch/mn10300/include/uapi/asm/mman.h:#include 
arch/score/include/uapi/asm/mman.h:#include 
arch/x86/include/uapi/asm/mman.h:#include 

which includes mman-common.h as well

and then we are left with
arch/alpha/include/uapi/asm/mman.h
arch/mips/include/uapi/asm/mman.h
arch/parisc/include/uapi/asm/mman.h
arch/xtensa/include/uapi/asm/mman.h

which do not include anything. So the patch can be indeed simplified.
I will fold the following into the patch. I hope nothing got left behind
but my defconfig compile test on all arches which i have a cross
compiler for succeeded.
---
commit 52a9272f419f428cb079d340f64113325516ef9b
Author: Michal Hocko 
Date:   Wed Dec 6 10:46:16 2017 +0100

- define MAP_FIXED_SAFE in asm-generic/mman-common.h as per Michael
  Ellerman because all architecture which use this header can share
  the same value. This will leave us with only 4 arches which need
  special handling.

diff --git a/arch/alpha/include/uapi/asm/mman.h 
b/arch/alpha/include/uapi/asm/mman.h
index ef3770262925..7287dbf1e11b 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -31,7 +31,6 @@
 #define MAP_NONBLOCK   0x4 /* do not block on IO */
 #define MAP_STACK  0x8 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x10/* create a huge page mapping */
-
 #define MAP_FIXED_SAFE 0x20/* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 #define MS_ASYNC   1   /* sync memory asynchronously */
diff --git a/arch/powerpc/include/uapi/asm/mman.h 
b/arch/powerpc/include/uapi/asm/mman.h
index 3ffd284e7160..e63bc37e33af 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -29,6 +29,5 @@
 #define MAP_NONBLOCK   0x1 /* do not block on IO */
 #define MAP_STACK  0x2 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x4 /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x80/* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/sparc/include/uapi/asm/mman.h 
b/arch/sparc/include/uapi/asm/mman.h
index 0c282c09fae8..d21bffd5d3dc 100644
--- a/arch/sparc/include/uapi/asm/mman.h
+++ b/arch/sparc/include/uapi/asm/mman.h
@@ -24,7 +24,5 @@
 #define MAP_NONBLOCK   0x1 /* do not block on IO */
 #define MAP_STACK  0x2 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x4 /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't unmap 
underlying mapping */
-
 
 #endif /* _UAPI__SPARC_MMAN_H__ */
diff --git a/arch/tile/include/uapi/asm/mman.h 
b/arch/tile/include/uapi/asm/mman.h
index b212f5fd5345..9b7add95926b 100644
--- a/arch/tile/include/uapi/asm/mman.h
+++ b/arch/tile/include/uapi/asm/mman.h
@@ -30,7 +30,6 @@
 #define MAP_DENYWRITE  0x0800  /* ETXTBSY */
 #define MAP_EXECUTABLE 0x1000  /* mark it as an executable */
 #define MAP_HUGETLB0x4000  /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x8000  /* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 
 /*
diff --git a/include/uapi/asm-generic/mman-common.h 
b/include/uapi/asm-generic/mman-common.h
index 6d319c46fd90..1eca2cb10d44 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -25,6 +25,7 @@
 #else
 # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */
 #endif
+#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 /*
  * Flags for mlock
diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h
index 56cde132a80a..2dffcbf705b3 100644
--- a/include/uapi/asm-generic/mman.h
+++ b/include/uapi/asm-generic/mman.h
@@ -13,7 +13,6 @@

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-06 Thread Michal Hocko 
On Wed 06-12-17 10:27:24, Michal Hocko wrote:
> On Wed 06-12-17 16:15:24, Michael Ellerman wrote:
[...]
> > So I think I proved above that all the arches that are using 0x8 are
> > also using mman-common.h, and vice-versa.
> > 
> > So you can put this in mman-common.h can't your?
> 
> Yes it seems I can. I would have to double check. It is true that
> defining the new flag closer to MAP_FIXED makes some sense

OK, so some recap
those which include generic mman-common.h directly
arch/sparc/include/uapi/asm/mman.h:#include 
arch/powerpc/include/uapi/asm/mman.h:#include 
arch/tile/include/uapi/asm/mman.h:#include 

then we have
arch/metag/include/asm/mman.h
which includes uapi/asm/mman.h and that is a generic one
arch/metag/include/uapi/asm/Kbuild:generic-y += mman.h

others include generic mman.h
arch/arm/include/uapi/asm/mman.h:#include 
arch/frv/include/uapi/asm/mman.h:#include 
arch/ia64/include/uapi/asm/mman.h:#include 
arch/m32r/include/uapi/asm/mman.h:#include 
arch/mn10300/include/uapi/asm/mman.h:#include 
arch/score/include/uapi/asm/mman.h:#include 
arch/x86/include/uapi/asm/mman.h:#include 

which includes mman-common.h as well

and then we are left with
arch/alpha/include/uapi/asm/mman.h
arch/mips/include/uapi/asm/mman.h
arch/parisc/include/uapi/asm/mman.h
arch/xtensa/include/uapi/asm/mman.h

which do not include anything. So the patch can be indeed simplified.
I will fold the following into the patch. I hope nothing got left behind
but my defconfig compile test on all arches which i have a cross
compiler for succeeded.
---
commit 52a9272f419f428cb079d340f64113325516ef9b
Author: Michal Hocko 
Date:   Wed Dec 6 10:46:16 2017 +0100

- define MAP_FIXED_SAFE in asm-generic/mman-common.h as per Michael
  Ellerman because all architecture which use this header can share
  the same value. This will leave us with only 4 arches which need
  special handling.

diff --git a/arch/alpha/include/uapi/asm/mman.h 
b/arch/alpha/include/uapi/asm/mman.h
index ef3770262925..7287dbf1e11b 100644
--- a/arch/alpha/include/uapi/asm/mman.h
+++ b/arch/alpha/include/uapi/asm/mman.h
@@ -31,7 +31,6 @@
 #define MAP_NONBLOCK   0x4 /* do not block on IO */
 #define MAP_STACK  0x8 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x10/* create a huge page mapping */
-
 #define MAP_FIXED_SAFE 0x20/* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 #define MS_ASYNC   1   /* sync memory asynchronously */
diff --git a/arch/powerpc/include/uapi/asm/mman.h 
b/arch/powerpc/include/uapi/asm/mman.h
index 3ffd284e7160..e63bc37e33af 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -29,6 +29,5 @@
 #define MAP_NONBLOCK   0x1 /* do not block on IO */
 #define MAP_STACK  0x2 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x4 /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x80/* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/sparc/include/uapi/asm/mman.h 
b/arch/sparc/include/uapi/asm/mman.h
index 0c282c09fae8..d21bffd5d3dc 100644
--- a/arch/sparc/include/uapi/asm/mman.h
+++ b/arch/sparc/include/uapi/asm/mman.h
@@ -24,7 +24,5 @@
 #define MAP_NONBLOCK   0x1 /* do not block on IO */
 #define MAP_STACK  0x2 /* give out an address that is best 
suited for process/thread stacks */
 #define MAP_HUGETLB0x4 /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't unmap 
underlying mapping */
-
 
 #endif /* _UAPI__SPARC_MMAN_H__ */
diff --git a/arch/tile/include/uapi/asm/mman.h 
b/arch/tile/include/uapi/asm/mman.h
index b212f5fd5345..9b7add95926b 100644
--- a/arch/tile/include/uapi/asm/mman.h
+++ b/arch/tile/include/uapi/asm/mman.h
@@ -30,7 +30,6 @@
 #define MAP_DENYWRITE  0x0800  /* ETXTBSY */
 #define MAP_EXECUTABLE 0x1000  /* mark it as an executable */
 #define MAP_HUGETLB0x4000  /* create a huge page mapping */
-#define MAP_FIXED_SAFE 0x8000  /* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 
 /*
diff --git a/include/uapi/asm-generic/mman-common.h 
b/include/uapi/asm-generic/mman-common.h
index 6d319c46fd90..1eca2cb10d44 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -25,6 +25,7 @@
 #else
 # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */
 #endif
+#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't unmap 
underlying mapping */
 
 /*
  * Flags for mlock
diff --git a/include/uapi/asm-generic/mman.h b/include/uapi/asm-generic/mman.h
index 56cde132a80a..2dffcbf705b3 100644
--- a/include/uapi/asm-generic/mman.h
+++ b/include/uapi/asm-generic/mman.h
@@ -13,7 +13,6 @@
 #define

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-06 Thread Michal Hocko 
On Wed 06-12-17 16:15:24, Michael Ellerman wrote:
> Hi Michal,
> 
> Some comments below.
> 
> Michal Hocko  writes:
> 
> > From: Michal Hocko 
> >
> > MAP_FIXED is used quite often to enforce mapping at the particular
> > range. The main problem of this flag is, however, that it is inherently
> > dangerous because it unmaps existing mappings covered by the requested
> > range. This can cause silent memory corruptions. Some of them even with
> > serious security implications. While the current semantic might be
> > really desiderable in many cases there are others which would want to
> > enforce the given range but rather see a failure than a silent memory
> > corruption on a clashing range. Please note that there is no guarantee
> > that a given range is obeyed by the mmap even when it is free - e.g.
> > arch specific code is allowed to apply an alignment.
> 
> I don't think this last sentence is correct. Or maybe I don't understand
> what you're referring to.
> 
> If you specifiy MAP_FIXED on a page boundary then the mapping must be
> made at that address, I don't think arch code is allowed to add any
> extra alignment.

The last sentence doesn't talk about MAP_FIXED. It talks about a hint
based mmap without MAP_FIXED ("there are others which would want to
enforce the given range but rather see a failure").
 
[...]
> > diff --git a/arch/alpha/include/uapi/asm/mman.h 
> > b/arch/alpha/include/uapi/asm/mman.h
> > index 6bf730063e3f..ef3770262925 100644
> > --- a/arch/alpha/include/uapi/asm/mman.h
> > +++ b/arch/alpha/include/uapi/asm/mman.h
> > @@ -32,6 +32,8 @@
> >  #define MAP_STACK  0x8 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x10/* create a huge page mapping */
> >  
> > +#define MAP_FIXED_SAFE 0x20/* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> > +
> 
> Why the new line before MAP_FIXED_SAFE? It should sit with the others.

will remove the empty line

> You're using a different value to other arches here, but that's OK, and
> alpha doesn't use asm-generic/mman.h or mman-common.h
> 
> > diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> > b/arch/powerpc/include/uapi/asm/mman.h
> > index e63bc37e33af..3ffd284e7160 100644
> > --- a/arch/powerpc/include/uapi/asm/mman.h
> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> > @@ -29,5 +29,6 @@
> >  #define MAP_NONBLOCK   0x1 /* do not block on IO */
> >  #define MAP_STACK  0x2 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x4 /* create a huge page mapping */
> > +#define MAP_FIXED_SAFE 0x80/* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> 
> Why did you pick 0x80?
> 
> I don't see any reason you can't use 0x8000 on powerpc.

Copy I guess, will update it.

[...]

> >  #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED
> >  # define MAP_UNINITIALIZED 0x400   /* For anonymous mmap, memory 
> > could be
> >  * uninitialized */
> > @@ -63,6 +64,7 @@
> >  # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */
> >  #endif
> >  
> > +
> 
> Stray new line.

will remove

> >  /*
> >   * Flags for msync
> >   */
> > diff --git a/include/uapi/asm-generic/mman.h 
> > b/include/uapi/asm-generic/mman.h
> > index 2dffcbf705b3..56cde132a80a 100644
> > --- a/include/uapi/asm-generic/mman.h
> > +++ b/include/uapi/asm-generic/mman.h
> > @@ -13,6 +13,7 @@
> >  #define MAP_NONBLOCK   0x1 /* do not block on IO */
> >  #define MAP_STACK  0x2 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x4 /* create a huge page mapping */
> > +#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> 
> So I think I proved above that all the arches that are using 0x8 are
> also using mman-common.h, and vice-versa.
> 
> So you can put this in mman-common.h can't you?

Yes it seems I can. I would have to double check. It is true that
defining the new flag closer to MAP_FIXED makes some sense

[...]
> So it would be more accurate to say something like:
> 
>   /*
>* Internal to the kernel MAP_FIXED_SAFE is a superset of
>* MAP_FIXED, so set MAP_FIXED in flags if MAP_FIXED_SAFE was
>* set by the caller. This avoids all the arch code having to
>* check for MAP_FIXED and MAP_FIXED_SAFE.
>*/
>   if (flags & MAP_FIXED_SAFE)
>   flags |= MAP_FIXED;

OK, I will use this wording.

Thanks for your review! Finally something that doesn't try to beat the
name to death ;)
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-06 Thread Michal Hocko 
On Wed 06-12-17 16:15:24, Michael Ellerman wrote:
> Hi Michal,
> 
> Some comments below.
> 
> Michal Hocko  writes:
> 
> > From: Michal Hocko 
> >
> > MAP_FIXED is used quite often to enforce mapping at the particular
> > range. The main problem of this flag is, however, that it is inherently
> > dangerous because it unmaps existing mappings covered by the requested
> > range. This can cause silent memory corruptions. Some of them even with
> > serious security implications. While the current semantic might be
> > really desiderable in many cases there are others which would want to
> > enforce the given range but rather see a failure than a silent memory
> > corruption on a clashing range. Please note that there is no guarantee
> > that a given range is obeyed by the mmap even when it is free - e.g.
> > arch specific code is allowed to apply an alignment.
> 
> I don't think this last sentence is correct. Or maybe I don't understand
> what you're referring to.
> 
> If you specifiy MAP_FIXED on a page boundary then the mapping must be
> made at that address, I don't think arch code is allowed to add any
> extra alignment.

The last sentence doesn't talk about MAP_FIXED. It talks about a hint
based mmap without MAP_FIXED ("there are others which would want to
enforce the given range but rather see a failure").
 
[...]
> > diff --git a/arch/alpha/include/uapi/asm/mman.h 
> > b/arch/alpha/include/uapi/asm/mman.h
> > index 6bf730063e3f..ef3770262925 100644
> > --- a/arch/alpha/include/uapi/asm/mman.h
> > +++ b/arch/alpha/include/uapi/asm/mman.h
> > @@ -32,6 +32,8 @@
> >  #define MAP_STACK  0x8 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x10/* create a huge page mapping */
> >  
> > +#define MAP_FIXED_SAFE 0x20/* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> > +
> 
> Why the new line before MAP_FIXED_SAFE? It should sit with the others.

will remove the empty line

> You're using a different value to other arches here, but that's OK, and
> alpha doesn't use asm-generic/mman.h or mman-common.h
> 
> > diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> > b/arch/powerpc/include/uapi/asm/mman.h
> > index e63bc37e33af..3ffd284e7160 100644
> > --- a/arch/powerpc/include/uapi/asm/mman.h
> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> > @@ -29,5 +29,6 @@
> >  #define MAP_NONBLOCK   0x1 /* do not block on IO */
> >  #define MAP_STACK  0x2 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x4 /* create a huge page mapping */
> > +#define MAP_FIXED_SAFE 0x80/* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> 
> Why did you pick 0x80?
> 
> I don't see any reason you can't use 0x8000 on powerpc.

Copy I guess, will update it.

[...]

> >  #ifdef CONFIG_MMAP_ALLOW_UNINITIALIZED
> >  # define MAP_UNINITIALIZED 0x400   /* For anonymous mmap, memory 
> > could be
> >  * uninitialized */
> > @@ -63,6 +64,7 @@
> >  # define MAP_UNINITIALIZED 0x0 /* Don't support this flag */
> >  #endif
> >  
> > +
> 
> Stray new line.

will remove

> >  /*
> >   * Flags for msync
> >   */
> > diff --git a/include/uapi/asm-generic/mman.h 
> > b/include/uapi/asm-generic/mman.h
> > index 2dffcbf705b3..56cde132a80a 100644
> > --- a/include/uapi/asm-generic/mman.h
> > +++ b/include/uapi/asm-generic/mman.h
> > @@ -13,6 +13,7 @@
> >  #define MAP_NONBLOCK   0x1 /* do not block on IO */
> >  #define MAP_STACK  0x2 /* give out an address that is best 
> > suited for process/thread stacks */
> >  #define MAP_HUGETLB0x4 /* create a huge page mapping */
> > +#define MAP_FIXED_SAFE 0x8 /* MAP_FIXED which doesn't 
> > unmap underlying mapping */
> 
> So I think I proved above that all the arches that are using 0x8 are
> also using mman-common.h, and vice-versa.
> 
> So you can put this in mman-common.h can't you?

Yes it seems I can. I would have to double check. It is true that
defining the new flag closer to MAP_FIXED makes some sense

[...]
> So it would be more accurate to say something like:
> 
>   /*
>* Internal to the kernel MAP_FIXED_SAFE is a superset of
>* MAP_FIXED, so set MAP_FIXED in flags if MAP_FIXED_SAFE was
>* set by the caller. This avoids all the arch code having to
>* check for MAP_FIXED and MAP_FIXED_SAFE.
>*/
>   if (flags & MAP_FIXED_SAFE)
>   flags |= MAP_FIXED;

OK, I will use this wording.

Thanks for your review! Finally something that doesn't try to beat the
name to death ;)
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-05 Thread Michael Ellerman 
Hi Michal,

Some comments below.

Michal Hocko  writes:

> From: Michal Hocko 
>
> MAP_FIXED is used quite often to enforce mapping at the particular
> range. The main problem of this flag is, however, that it is inherently
> dangerous because it unmaps existing mappings covered by the requested
> range. This can cause silent memory corruptions. Some of them even with
> serious security implications. While the current semantic might be
> really desiderable in many cases there are others which would want to
> enforce the given range but rather see a failure than a silent memory
> corruption on a clashing range. Please note that there is no guarantee
> that a given range is obeyed by the mmap even when it is free - e.g.
> arch specific code is allowed to apply an alignment.

I don't think this last sentence is correct. Or maybe I don't understand
what you're referring to.

If you specifiy MAP_FIXED on a page boundary then the mapping must be
made at that address, I don't think arch code is allowed to add any
extra alignment.

> Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior.
> It has the same semantic as MAP_FIXED wrt. the given address request
> with a single exception that it fails with EEXIST if the requested
> address is already covered by an existing mapping. We still do rely on
> get_unmaped_area to handle all the arch specific MAP_FIXED treatment and
> check for a conflicting vma after it returns.
>
> [fail on clashing range with EEXIST as per Florian Weimer]
> [set MAP_FIXED before round_hint_to_min as per Khalid Aziz]
> Reviewed-by: Khalid Aziz 
> Signed-off-by: Michal Hocko 
> ---
>  arch/alpha/include/uapi/asm/mman.h   |  2 ++
>  arch/mips/include/uapi/asm/mman.h|  2 ++
>  arch/parisc/include/uapi/asm/mman.h  |  2 ++
>  arch/powerpc/include/uapi/asm/mman.h |  1 +
>  arch/sparc/include/uapi/asm/mman.h   |  1 +
>  arch/tile/include/uapi/asm/mman.h|  1 +
>  arch/xtensa/include/uapi/asm/mman.h  |  2 ++
>  include/uapi/asm-generic/mman.h  |  1 +
>  mm/mmap.c| 11 +++
>  9 files changed, 23 insertions(+)
>
> diff --git a/arch/alpha/include/uapi/asm/mman.h 
> b/arch/alpha/include/uapi/asm/mman.h
> index 6bf730063e3f..ef3770262925 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -32,6 +32,8 @@
>  #define MAP_STACK0x8 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x10/* create a huge page mapping */
>  
> +#define MAP_FIXED_SAFE   0x20/* MAP_FIXED which doesn't 
> unmap underlying mapping */
> +

Why the new line before MAP_FIXED_SAFE? It should sit with the others.

You're using a different value to other arches here, but that's OK, and
alpha doesn't use asm-generic/mman.h or mman-common.h

> diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> b/arch/powerpc/include/uapi/asm/mman.h
> index e63bc37e33af..3ffd284e7160 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -29,5 +29,6 @@
>  #define MAP_NONBLOCK 0x1 /* do not block on IO */
>  #define MAP_STACK0x2 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x4 /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x80/* MAP_FIXED which doesn't 
> unmap underlying mapping */

Why did you pick 0x80?

I don't see any reason you can't use 0x8000 on powerpc.

 
> diff --git a/arch/sparc/include/uapi/asm/mman.h 
> b/arch/sparc/include/uapi/asm/mman.h
> index 715a2c927e79..0c282c09fae8 100644
> --- a/arch/sparc/include/uapi/asm/mman.h
> +++ b/arch/sparc/include/uapi/asm/mman.h
> @@ -24,6 +24,7 @@
>  #define MAP_NONBLOCK 0x1 /* do not block on IO */
>  #define MAP_STACK0x2 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x4 /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x8 /* MAP_FIXED which doesn't 
> unmap underlying mapping */

Using 0x8 on sparc, sparc uses mman-common.h.

> diff --git a/arch/tile/include/uapi/asm/mman.h 
> b/arch/tile/include/uapi/asm/mman.h
> index 9b7add95926b..b212f5fd5345 100644
> --- a/arch/tile/include/uapi/asm/mman.h
> +++ b/arch/tile/include/uapi/asm/mman.h
> @@ -30,6 +30,7 @@
>  #define MAP_DENYWRITE0x0800  /* ETXTBSY */
>  #define MAP_EXECUTABLE   0x1000  /* mark it as an executable */
>  #define MAP_HUGETLB  0x4000  /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x8000  /* MAP_FIXED which doesn't 
> unmap underlying mapping */
  
That is the next free flag, but you could also use 0x8 on tile.

tile uses mman-common.h.

> diff --git a/arch/xtensa/include/uapi/asm/mman.h 
>

Re: [PATCH 1/2] mm: introduce MAP_FIXED_SAFE

2017-12-05 Thread Michael Ellerman 
Hi Michal,

Some comments below.

Michal Hocko  writes:

> From: Michal Hocko 
>
> MAP_FIXED is used quite often to enforce mapping at the particular
> range. The main problem of this flag is, however, that it is inherently
> dangerous because it unmaps existing mappings covered by the requested
> range. This can cause silent memory corruptions. Some of them even with
> serious security implications. While the current semantic might be
> really desiderable in many cases there are others which would want to
> enforce the given range but rather see a failure than a silent memory
> corruption on a clashing range. Please note that there is no guarantee
> that a given range is obeyed by the mmap even when it is free - e.g.
> arch specific code is allowed to apply an alignment.

I don't think this last sentence is correct. Or maybe I don't understand
what you're referring to.

If you specifiy MAP_FIXED on a page boundary then the mapping must be
made at that address, I don't think arch code is allowed to add any
extra alignment.

> Introduce a new MAP_FIXED_SAFE flag for mmap to achieve this behavior.
> It has the same semantic as MAP_FIXED wrt. the given address request
> with a single exception that it fails with EEXIST if the requested
> address is already covered by an existing mapping. We still do rely on
> get_unmaped_area to handle all the arch specific MAP_FIXED treatment and
> check for a conflicting vma after it returns.
>
> [fail on clashing range with EEXIST as per Florian Weimer]
> [set MAP_FIXED before round_hint_to_min as per Khalid Aziz]
> Reviewed-by: Khalid Aziz 
> Signed-off-by: Michal Hocko 
> ---
>  arch/alpha/include/uapi/asm/mman.h   |  2 ++
>  arch/mips/include/uapi/asm/mman.h|  2 ++
>  arch/parisc/include/uapi/asm/mman.h  |  2 ++
>  arch/powerpc/include/uapi/asm/mman.h |  1 +
>  arch/sparc/include/uapi/asm/mman.h   |  1 +
>  arch/tile/include/uapi/asm/mman.h|  1 +
>  arch/xtensa/include/uapi/asm/mman.h  |  2 ++
>  include/uapi/asm-generic/mman.h  |  1 +
>  mm/mmap.c| 11 +++
>  9 files changed, 23 insertions(+)
>
> diff --git a/arch/alpha/include/uapi/asm/mman.h 
> b/arch/alpha/include/uapi/asm/mman.h
> index 6bf730063e3f..ef3770262925 100644
> --- a/arch/alpha/include/uapi/asm/mman.h
> +++ b/arch/alpha/include/uapi/asm/mman.h
> @@ -32,6 +32,8 @@
>  #define MAP_STACK0x8 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x10/* create a huge page mapping */
>  
> +#define MAP_FIXED_SAFE   0x20/* MAP_FIXED which doesn't 
> unmap underlying mapping */
> +

Why the new line before MAP_FIXED_SAFE? It should sit with the others.

You're using a different value to other arches here, but that's OK, and
alpha doesn't use asm-generic/mman.h or mman-common.h

> diff --git a/arch/powerpc/include/uapi/asm/mman.h 
> b/arch/powerpc/include/uapi/asm/mman.h
> index e63bc37e33af..3ffd284e7160 100644
> --- a/arch/powerpc/include/uapi/asm/mman.h
> +++ b/arch/powerpc/include/uapi/asm/mman.h
> @@ -29,5 +29,6 @@
>  #define MAP_NONBLOCK 0x1 /* do not block on IO */
>  #define MAP_STACK0x2 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x4 /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x80/* MAP_FIXED which doesn't 
> unmap underlying mapping */

Why did you pick 0x80?

I don't see any reason you can't use 0x8000 on powerpc.

 
> diff --git a/arch/sparc/include/uapi/asm/mman.h 
> b/arch/sparc/include/uapi/asm/mman.h
> index 715a2c927e79..0c282c09fae8 100644
> --- a/arch/sparc/include/uapi/asm/mman.h
> +++ b/arch/sparc/include/uapi/asm/mman.h
> @@ -24,6 +24,7 @@
>  #define MAP_NONBLOCK 0x1 /* do not block on IO */
>  #define MAP_STACK0x2 /* give out an address that is best 
> suited for process/thread stacks */
>  #define MAP_HUGETLB  0x4 /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x8 /* MAP_FIXED which doesn't 
> unmap underlying mapping */

Using 0x8 on sparc, sparc uses mman-common.h.

> diff --git a/arch/tile/include/uapi/asm/mman.h 
> b/arch/tile/include/uapi/asm/mman.h
> index 9b7add95926b..b212f5fd5345 100644
> --- a/arch/tile/include/uapi/asm/mman.h
> +++ b/arch/tile/include/uapi/asm/mman.h
> @@ -30,6 +30,7 @@
>  #define MAP_DENYWRITE0x0800  /* ETXTBSY */
>  #define MAP_EXECUTABLE   0x1000  /* mark it as an executable */
>  #define MAP_HUGETLB  0x4000  /* create a huge page mapping */
> +#define MAP_FIXED_SAFE   0x8000  /* MAP_FIXED which doesn't 
> unmap underlying mapping */
  
That is the next free flag, but you could also use 0x8 on tile.

tile uses mman-common.h.

> diff --git a/arch/xtensa/include/uapi/asm/mman.h 
> b/arch/xtensa/include/uapi/asm/mman.h
> index 2bfe590694fc..0daf199caa57 100644
> ---