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
