lkml.org 
[lkml]   [2017]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/2] mm: introduce MAP_FIXED_SAFE
On Fri 17-11-17 00:37:18, John Hubbard wrote:
> On 11/16/2017 02:18 AM, Michal Hocko wrote:
[...]
> > diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
> > index 03c06ba7464f..d97342ca25b1 100644
> > --- a/arch/powerpc/include/uapi/asm/mman.h
> > +++ b/arch/powerpc/include/uapi/asm/mman.h
> > @@ -28,5 +28,6 @@
> > #define MAP_NONBLOCK 0x10000 /* do not block on IO */
> > #define MAP_STACK 0x20000 /* give out an address that is best suited for process/thread stacks */
> > #define MAP_HUGETLB 0x40000 /* create a huge page mapping */
> > +#define MAP_FIXED_SAFE 0x800000 /* MAP_FIXED which doesn't unmap underlying mapping */
>
>
> Hi Michal,
>
> 1. The powerpc change, above, has one too many zeroes. It should be 0x80000,
> not 0x800000.

OK, I will fix it. It shouldn't matter much, because we only care about
non-clashing address but I agree that we should consume them from bottom
bits.

> 2. For the one-line comments, if you phrase them like this:
>
> /* Like MAP_FIXED, except that it doesn't unmap pre-existing mappings */
>
> ...I think that would be a little clearer.

I do not have any strong preference here.
[...]
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 680506faceae..89af0b5839a5 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1342,6 +1342,10 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
> > if (!(file && path_noexec(&file->f_path)))
> > prot |= PROT_EXEC;
> >
> > + /* force arch specific MAP_FIXED handling in get_unmapped_area */
> > + if (flags & MAP_FIXED_SAFE)
> > + flags |= MAP_FIXED;
>
> Hooking in at this point is a nice way to solve the problem. :)
>
> For the naming and implementation, I see a couple of things that might improve
> it slightly:
>
> a) Change MAP_FIXED_SAFE to MAP_NO_CLOBBER (as per Kees' idea), but keep the
> new flag independent, by omitting the above two lines. Instead of forcing
> MAP_FIXED as a result of the new flag, you could simply fail to take any action
> on MAP_NO_CLOBBER *unless* MAP_FIXED is set.
>
> This is a bit easier to explain and reason about, as compared to a flag that
> auto-sets another flag. I like this approach best.

As I've exaplained in other email I do not think we can make this a
modifier.

> or
>
> b) Change MAP_FIXED_SAFE to MAP_FIXED_NO_CLOBBER (also a variation on Kees' name
> idea, but a little longer, a bit uglier, and clearer), and leave the implementation
> the same.

I do not have a _strong_ preference on the name itself. But I think that
_SAFE reflects the behavior slightly better because _NO_CLOBBER is not
very specific _when_ and _what_ we do not clobber while _SAFE is clear
on that it doesn't perform any unsafe operations.

But if the majority think that _NO_CLOBBER is better i will change it.
--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2017-11-20 10:03    [W:0.139 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site