2017-12-01 15:27:55

by Cyril Hrubis

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

Hi!
> > MAP_FIXED_UNIQUE
> > MAP_FIXED_ONCE
> > MAP_FIXED_FRESH
>
> Well, I can open a poll for the best name, but none of those you are
> proposing sound much better to me. Yeah, naming sucks...

Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
would probably be a best fit.

--
Cyril Hrubis
[email protected]


2017-12-06 04:51:50

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

Cyril Hrubis <[email protected]> writes:

> Hi!
>> > MAP_FIXED_UNIQUE
>> > MAP_FIXED_ONCE
>> > MAP_FIXED_FRESH
>>
>> Well, I can open a poll for the best name, but none of those you are
>> proposing sound much better to me. Yeah, naming sucks...
>
> Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
> would probably be a best fit.

Yeah that could work.

I prefer "no clobber" as I just suggested, because the existing
MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
one - which you or another thread may be using - and clobbers it with
the new one.

cheers

2017-12-06 04:54:43

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Wed, Dec 06, 2017 at 03:51:44PM +1100, Michael Ellerman wrote:
> Cyril Hrubis <[email protected]> writes:
>
> > Hi!
> >> > MAP_FIXED_UNIQUE
> >> > MAP_FIXED_ONCE
> >> > MAP_FIXED_FRESH
> >>
> >> Well, I can open a poll for the best name, but none of those you are
> >> proposing sound much better to me. Yeah, naming sucks...
> >
> > Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
> > would probably be a best fit.
>
> Yeah that could work.
>
> I prefer "no clobber" as I just suggested, because the existing
> MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
> one - which you or another thread may be using - and clobbers it with
> the new one.

It's longer than MAP_FIXED_WEAK :-P

You'd have to be pretty darn strong to clobber an existing mapping.

2017-12-06 07:04:00

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Tue, Dec 05, 2017 at 08:54:35PM -0800, Matthew Wilcox wrote:
> On Wed, Dec 06, 2017 at 03:51:44PM +1100, Michael Ellerman wrote:
> > Cyril Hrubis <[email protected]> writes:
> >
> > > Hi!
> > >> > MAP_FIXED_UNIQUE
> > >> > MAP_FIXED_ONCE
> > >> > MAP_FIXED_FRESH
> > >>
> > >> Well, I can open a poll for the best name, but none of those you are
> > >> proposing sound much better to me. Yeah, naming sucks...
> > >
> > > Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
> > > would probably be a best fit.
> >
> > Yeah that could work.
> >
> > I prefer "no clobber" as I just suggested, because the existing
> > MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
> > one - which you or another thread may be using - and clobbers it with
> > the new one.
>
> It's longer than MAP_FIXED_WEAK :-P
>
> You'd have to be pretty darn strong to clobber an existing mapping.

I think we're thinking about this all wrong. We shouldn't document it as
"This is a variant of MAP_FIXED". We should document it as "Here's an
alternative to MAP_FIXED".

So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
we could add a new paragraph saying "at most one of MAP_FIXED or
MAP_REQUIRED" and "any of the following values".

Now, we should implement MAP_REQUIRED as having each architecture
define _MAP_NOT_A_HINT, and then #define MAP_REQUIRED (MAP_FIXED |
_MAP_NOT_A_HINT), but that's not information to confuse users with.

Also, that lets us add a third option at some point that is Yet Another
Way to interpret the 'addr' argument, by having MAP_FIXED clear and
_MAP_NOT_A_HINT set.

I'm not set on MAP_REQUIRED. I came up with some awful names
(MAP_TODDLER, MAP_TANTRUM, MAP_ULTIMATUM, MAP_BOSS, MAP_PROGRAM_MANAGER,
etc). But I think we should drop FIXED from the middle of the name.

2017-12-06 07:34:02

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On 12/05/2017 11:03 PM, Matthew Wilcox wrote:
> On Tue, Dec 05, 2017 at 08:54:35PM -0800, Matthew Wilcox wrote:
>> On Wed, Dec 06, 2017 at 03:51:44PM +1100, Michael Ellerman wrote:
>>> Cyril Hrubis <[email protected]> writes:
>>>
>>>> Hi!
>>>>>> MAP_FIXED_UNIQUE
>>>>>> MAP_FIXED_ONCE
>>>>>> MAP_FIXED_FRESH
>>>>>
>>>>> Well, I can open a poll for the best name, but none of those you are
>>>>> proposing sound much better to me. Yeah, naming sucks...
>>>>
>>>> Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
>>>> would probably be a best fit.
>>>
>>> Yeah that could work.
>>>
>>> I prefer "no clobber" as I just suggested, because the existing
>>> MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
>>> one - which you or another thread may be using - and clobbers it with
>>> the new one.
>>
>> It's longer than MAP_FIXED_WEAK :-P
>>
>> You'd have to be pretty darn strong to clobber an existing mapping.
>
> I think we're thinking about this all wrong. We shouldn't document it as
> "This is a variant of MAP_FIXED". We should document it as "Here's an
> alternative to MAP_FIXED".
>
> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
> we could add a new paragraph saying "at most one of MAP_FIXED or
> MAP_REQUIRED" and "any of the following values".
>
> Now, we should implement MAP_REQUIRED as having each architecture
> define _MAP_NOT_A_HINT, and then #define MAP_REQUIRED (MAP_FIXED |
> _MAP_NOT_A_HINT), but that's not information to confuse users with.
>
> Also, that lets us add a third option at some point that is Yet Another
> Way to interpret the 'addr' argument, by having MAP_FIXED clear and
> _MAP_NOT_A_HINT set.
>
> I'm not set on MAP_REQUIRED. I came up with some awful names
> (MAP_TODDLER, MAP_TANTRUM, MAP_ULTIMATUM, MAP_BOSS, MAP_PROGRAM_MANAGER,
> etc). But I think we should drop FIXED from the middle of the name.
>

In that case, maybe:

MAP_EXACT

? ...because that's the characteristic behavior. It doesn't clobber, but
you don't need to say that in the name, now that we're not including
_FIXED_ in the middle.

thanks,
John Hubbard
NVIDIA

2017-12-06 07:35:53

by Florian Weimer

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On 12/06/2017 08:33 AM, John Hubbard wrote:
> In that case, maybe:
>
> MAP_EXACT
>
> ? ...because that's the characteristic behavior.

Is that true? mmap still silently rounding up the length to the page
size, I assume, so even that name is misleading.

Thanks,
Florian

2017-12-06 08:06:58

by John Hubbard

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On 12/05/2017 11:35 PM, Florian Weimer wrote:
> On 12/06/2017 08:33 AM, John Hubbard wrote:
>> In that case, maybe:
>>
>>      MAP_EXACT
>>
>> ? ...because that's the characteristic behavior.
>
> Is that true?  mmap still silently rounding up the length to the page size, I assume, so even that name is misleading.

Hi Florian,

Not as far as I can tell, it's not doing that.

For both MAP_FIXED, and this new flag, the documented (and actual)
behavior is *not* to do any such rounding. Instead, the requested
input address is required to be page-aligned itself, and mmap()
should be honoring the exact addr.

>From the mmap(2) man page:

MAP_FIXED
Don't interpret addr as a hint: place the mapping at
exactly that address. addr must be a multiple of the
page size.


And from what I can see, the do_mmap() implementation leaves addr
unchanged, in the MAP_FIXED case:

do_mmap(...)
{
/* ... */
if (!(flags & MAP_FIXED))
addr = round_hint_to_min(addr);

...although it does look like device drivers have the opportunity
to break that:

mmap_region(...)
{
/* Can addr have changed??
*
* Answer: Yes, several device drivers can do it in their
* f_op->mmap method. -DaveM
* Bug: If addr is changed, prev, rb_link, rb_parent should
* be updated for vma_link()
*/
WARN_ON_ONCE(addr != vma->vm_start);

addr = vma->vm_start;


--
thanks,
John Hubbard
NVIDIA

>
> Thanks,
> Florian

2017-12-06 08:54:37

by Florian Weimer

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On 12/06/2017 09:06 AM, John Hubbard wrote:
> On 12/05/2017 11:35 PM, Florian Weimer wrote:
>> On 12/06/2017 08:33 AM, John Hubbard wrote:
>>> In that case, maybe:
>>>
>>>      MAP_EXACT
>>>
>>> ? ...because that's the characteristic behavior.
>>
>> Is that true?  mmap still silently rounding up the length to the page size, I assume, so even that name is misleading.
>
> Hi Florian,
>
> Not as far as I can tell, it's not doing that.
>
> For both MAP_FIXED, and this new flag, the documented (and actual)
> behavior is *not* to do any such rounding. Instead, the requested
> input address is required to be page-aligned itself, and mmap()
> should be honoring the exact addr.

I meant the length, not the address.

Florian

2017-12-07 05:46:17

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

Matthew Wilcox <[email protected]> writes:

> On Tue, Dec 05, 2017 at 08:54:35PM -0800, Matthew Wilcox wrote:
>> On Wed, Dec 06, 2017 at 03:51:44PM +1100, Michael Ellerman wrote:
>> > Cyril Hrubis <[email protected]> writes:
>> >
>> > > Hi!
>> > >> > MAP_FIXED_UNIQUE
>> > >> > MAP_FIXED_ONCE
>> > >> > MAP_FIXED_FRESH
>> > >>
>> > >> Well, I can open a poll for the best name, but none of those you are
>> > >> proposing sound much better to me. Yeah, naming sucks...
>> > >
>> > > Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
>> > > would probably be a best fit.
>> >
>> > Yeah that could work.
>> >
>> > I prefer "no clobber" as I just suggested, because the existing
>> > MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
>> > one - which you or another thread may be using - and clobbers it with
>> > the new one.
>>
>> It's longer than MAP_FIXED_WEAK :-P
>>
>> You'd have to be pretty darn strong to clobber an existing mapping.
>
> I think we're thinking about this all wrong. We shouldn't document it as
> "This is a variant of MAP_FIXED". We should document it as "Here's an
> alternative to MAP_FIXED".
>
> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
> we could add a new paragraph saying "at most one of MAP_FIXED or
> MAP_REQUIRED" and "any of the following values".
>
> Now, we should implement MAP_REQUIRED as having each architecture
> define _MAP_NOT_A_HINT, and then #define MAP_REQUIRED (MAP_FIXED |
> _MAP_NOT_A_HINT), but that's not information to confuse users with.
>
> Also, that lets us add a third option at some point that is Yet Another
> Way to interpret the 'addr' argument, by having MAP_FIXED clear and
> _MAP_NOT_A_HINT set.
>
> I'm not set on MAP_REQUIRED. I came up with some awful names
> (MAP_TODDLER, MAP_TANTRUM, MAP_ULTIMATUM, MAP_BOSS, MAP_PROGRAM_MANAGER,
> etc). But I think we should drop FIXED from the middle of the name.

MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
it either :)

What about MAP_AT_ADDR ?

It's short, and says what it does on the tin. The first argument to mmap
is actually called "addr" too.

cheers

2017-12-07 19:14:32

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Wed, Dec 6, 2017 at 9:46 PM, Michael Ellerman <[email protected]> wrote:
> Matthew Wilcox <[email protected]> writes:
>
>> On Tue, Dec 05, 2017 at 08:54:35PM -0800, Matthew Wilcox wrote:
>>> On Wed, Dec 06, 2017 at 03:51:44PM +1100, Michael Ellerman wrote:
>>> > Cyril Hrubis <[email protected]> writes:
>>> >
>>> > > Hi!
>>> > >> > MAP_FIXED_UNIQUE
>>> > >> > MAP_FIXED_ONCE
>>> > >> > MAP_FIXED_FRESH
>>> > >>
>>> > >> Well, I can open a poll for the best name, but none of those you are
>>> > >> proposing sound much better to me. Yeah, naming sucks...
>>> > >
>>> > > Given that MAP_FIXED replaces the previous mapping MAP_FIXED_NOREPLACE
>>> > > would probably be a best fit.
>>> >
>>> > Yeah that could work.
>>> >
>>> > I prefer "no clobber" as I just suggested, because the existing
>>> > MAP_FIXED doesn't politely "replace" a mapping, it destroys the current
>>> > one - which you or another thread may be using - and clobbers it with
>>> > the new one.
>>>
>>> It's longer than MAP_FIXED_WEAK :-P
>>>
>>> You'd have to be pretty darn strong to clobber an existing mapping.
>>
>> I think we're thinking about this all wrong. We shouldn't document it as
>> "This is a variant of MAP_FIXED". We should document it as "Here's an
>> alternative to MAP_FIXED".
>>
>> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
>> we could add a new paragraph saying "at most one of MAP_FIXED or
>> MAP_REQUIRED" and "any of the following values".
>>
>> Now, we should implement MAP_REQUIRED as having each architecture
>> define _MAP_NOT_A_HINT, and then #define MAP_REQUIRED (MAP_FIXED |
>> _MAP_NOT_A_HINT), but that's not information to confuse users with.
>>
>> Also, that lets us add a third option at some point that is Yet Another
>> Way to interpret the 'addr' argument, by having MAP_FIXED clear and
>> _MAP_NOT_A_HINT set.
>>
>> I'm not set on MAP_REQUIRED. I came up with some awful names
>> (MAP_TODDLER, MAP_TANTRUM, MAP_ULTIMATUM, MAP_BOSS, MAP_PROGRAM_MANAGER,
>> etc). But I think we should drop FIXED from the middle of the name.
>
> MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
> it either :)
>
> What about MAP_AT_ADDR ?
>
> It's short, and says what it does on the tin. The first argument to mmap
> is actually called "addr" too.

"FIXED" is supposed to do this too.

Pavel suggested:

MAP_ADD_FIXED

(which is different from "use fixed", and describes why it would fail:
can't add since it already exists.)

Perhaps "MAP_FIXED_NEW"?

There has been a request to drop "FIXED" from the name, so these:

MAP_FIXED_NOCLOBBER
MAP_FIXED_NOREPLACE
MAP_FIXED_ADD
MAP_FIXED_NEW

Could be:

MAP_NOCLOBBER
MAP_NOREPLACE
MAP_ADD
MAP_NEW

and we still have the unloved, but acceptable:

MAP_REQUIRED

My vote is still for "NOREPLACE" or "NOCLOBBER" since it's very
specific, though "NEW" is pretty clear too.

-Kees

--
Kees Cook
Pixel Security

2017-12-07 19:57:35

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Thu, Dec 07, 2017 at 11:14:27AM -0800, Kees Cook wrote:
> On Wed, Dec 6, 2017 at 9:46 PM, Michael Ellerman <[email protected]> wrote:
> > Matthew Wilcox <[email protected]> writes:
> >> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
> >> we could add a new paragraph saying "at most one of MAP_FIXED or
> >> MAP_REQUIRED" and "any of the following values".
> >
> > MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
> > it either :)
> >
> > What about MAP_AT_ADDR ?
> >
> > It's short, and says what it does on the tin. The first argument to mmap
> > is actually called "addr" too.
>
> "FIXED" is supposed to do this too.
>
> Pavel suggested:
>
> MAP_ADD_FIXED
>
> (which is different from "use fixed", and describes why it would fail:
> can't add since it already exists.)
>
> Perhaps "MAP_FIXED_NEW"?
>
> There has been a request to drop "FIXED" from the name, so these:
>
> MAP_FIXED_NOCLOBBER
> MAP_FIXED_NOREPLACE
> MAP_FIXED_ADD
> MAP_FIXED_NEW
>
> Could be:
>
> MAP_NOCLOBBER
> MAP_NOREPLACE
> MAP_ADD
> MAP_NEW
>
> and we still have the unloved, but acceptable:
>
> MAP_REQUIRED
>
> My vote is still for "NOREPLACE" or "NOCLOBBER" since it's very
> specific, though "NEW" is pretty clear too.

How about MAP_NOFORCE?

2017-12-08 08:33:21

by Michal Hocko

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Thu 07-12-17 11:57:27, Matthew Wilcox wrote:
> On Thu, Dec 07, 2017 at 11:14:27AM -0800, Kees Cook wrote:
> > On Wed, Dec 6, 2017 at 9:46 PM, Michael Ellerman <[email protected]> wrote:
> > > Matthew Wilcox <[email protected]> writes:
> > >> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
> > >> we could add a new paragraph saying "at most one of MAP_FIXED or
> > >> MAP_REQUIRED" and "any of the following values".
> > >
> > > MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
> > > it either :)
> > >
> > > What about MAP_AT_ADDR ?
> > >
> > > It's short, and says what it does on the tin. The first argument to mmap
> > > is actually called "addr" too.
> >
> > "FIXED" is supposed to do this too.
> >
> > Pavel suggested:
> >
> > MAP_ADD_FIXED
> >
> > (which is different from "use fixed", and describes why it would fail:
> > can't add since it already exists.)
> >
> > Perhaps "MAP_FIXED_NEW"?
> >
> > There has been a request to drop "FIXED" from the name, so these:
> >
> > MAP_FIXED_NOCLOBBER
> > MAP_FIXED_NOREPLACE
> > MAP_FIXED_ADD
> > MAP_FIXED_NEW
> >
> > Could be:
> >
> > MAP_NOCLOBBER
> > MAP_NOREPLACE
> > MAP_ADD
> > MAP_NEW
> >
> > and we still have the unloved, but acceptable:
> >
> > MAP_REQUIRED
> >
> > My vote is still for "NOREPLACE" or "NOCLOBBER" since it's very
> > specific, though "NEW" is pretty clear too.
>
> How about MAP_NOFORCE?

OK, this doesn't seem to lead to anywhere. The more this is discussed
the more names we are getting. So you know what? I will resubmit and
keep my original name. If somebody really hates it then feel free to
nack the patch and push alternative and gain concensus on it.

I will keep MAP_FIXED_SAFE because it is an alternative to MAP_FIXED so
having that in the name is _useful_ for everybody familiar with
MAP_FIXED already. And _SAFE suffix tells that the operation doesn't
cause any silent memory corruptions or other unexpected side effects.
--
Michal Hocko
SUSE Labs

2017-12-08 11:08:15

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

Matthew Wilcox <[email protected]> writes:

> On Thu, Dec 07, 2017 at 11:14:27AM -0800, Kees Cook wrote:
>> On Wed, Dec 6, 2017 at 9:46 PM, Michael Ellerman <[email protected]> wrote:
>> > Matthew Wilcox <[email protected]> writes:
>> >> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
>> >> we could add a new paragraph saying "at most one of MAP_FIXED or
>> >> MAP_REQUIRED" and "any of the following values".
>> >
>> > MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
>> > it either :)
>> >
>> > What about MAP_AT_ADDR ?
>> >
>> > It's short, and says what it does on the tin. The first argument to mmap
>> > is actually called "addr" too.
>>
>> "FIXED" is supposed to do this too.
>>
>> Pavel suggested:
>>
>> MAP_ADD_FIXED
>>
>> (which is different from "use fixed", and describes why it would fail:
>> can't add since it already exists.)
>>
>> Perhaps "MAP_FIXED_NEW"?
>>
>> There has been a request to drop "FIXED" from the name, so these:
>>
>> MAP_FIXED_NOCLOBBER
>> MAP_FIXED_NOREPLACE
>> MAP_FIXED_ADD
>> MAP_FIXED_NEW
>>
>> Could be:
>>
>> MAP_NOCLOBBER
>> MAP_NOREPLACE
>> MAP_ADD
>> MAP_NEW
>>
>> and we still have the unloved, but acceptable:
>>
>> MAP_REQUIRED
>>
>> My vote is still for "NOREPLACE" or "NOCLOBBER" since it's very
>> specific, though "NEW" is pretty clear too.
>
> How about MAP_NOFORCE?

It doesn't tell me that addr is not a hint. That's a crucial detail.

Without MAP_FIXED mmap never "forces/replaces/clobbers", so why would I
need MAP_NOFORCE if I don't have MAP_FIXED?

So it needs something in there to indicate that the addr is not a hint,
that's the only thing that flag actually *does*.


If we had a time machine, the right set of flags would be:

- MAP_FIXED: don't treat addr as a hint, fail if addr is not free
- MAP_REPLACE: replace an existing mapping (or force or clobber)

But the two were conflated for some reason in the current MAP_FIXED.

Given we can't go back and fix it, the closest we can get is to add a
variant of MAP_FIXED which subtracts the "REPLACE" semantic.

ie: MAP_FIXED_NOREPLACE

cheers

2017-12-08 14:27:22

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Fri 2017-12-08 22:08:07, Michael Ellerman wrote:
> Matthew Wilcox <[email protected]> writes:
>
> > On Thu, Dec 07, 2017 at 11:14:27AM -0800, Kees Cook wrote:
> >> On Wed, Dec 6, 2017 at 9:46 PM, Michael Ellerman <[email protected]> wrote:
> >> > Matthew Wilcox <[email protected]> writes:
> >> >> So, just like we currently say "exactly one of MAP_SHARED or MAP_PRIVATE",
> >> >> we could add a new paragraph saying "at most one of MAP_FIXED or
> >> >> MAP_REQUIRED" and "any of the following values".
> >> >
> >> > MAP_REQUIRED doesn't immediately grab me, but I don't actively dislike
> >> > it either :)
> >> >
> >> > What about MAP_AT_ADDR ?
> >> >
> >> > It's short, and says what it does on the tin. The first argument to mmap
> >> > is actually called "addr" too.
> >>
> >> "FIXED" is supposed to do this too.
> >>
> >> Pavel suggested:
> >>
> >> MAP_ADD_FIXED
> >>
> >> (which is different from "use fixed", and describes why it would fail:
> >> can't add since it already exists.)
> >>
> >> Perhaps "MAP_FIXED_NEW"?
> >>
> >> There has been a request to drop "FIXED" from the name, so these:
> >>
> >> MAP_FIXED_NOCLOBBER
> >> MAP_FIXED_NOREPLACE
> >> MAP_FIXED_ADD
> >> MAP_FIXED_NEW
> >>
> >> Could be:
> >>
> >> MAP_NOCLOBBER
> >> MAP_NOREPLACE
> >> MAP_ADD
> >> MAP_NEW
> >>
> >> and we still have the unloved, but acceptable:
> >>
> >> MAP_REQUIRED
> >>
> >> My vote is still for "NOREPLACE" or "NOCLOBBER" since it's very
> >> specific, though "NEW" is pretty clear too.
> >
> > How about MAP_NOFORCE?
>
> It doesn't tell me that addr is not a hint. That's a crucial detail.
>
> Without MAP_FIXED mmap never "forces/replaces/clobbers", so why would I
> need MAP_NOFORCE if I don't have MAP_FIXED?
>
> So it needs something in there to indicate that the addr is not a hint,
> that's the only thing that flag actually *does*.
>
>
> If we had a time machine, the right set of flags would be:
>
> - MAP_FIXED: don't treat addr as a hint, fail if addr is not free
> - MAP_REPLACE: replace an existing mapping (or force or clobber)

Actually, if we had a time machine... would we even provide
MAP_REPLACE functionality?

> But the two were conflated for some reason in the current MAP_FIXED.
>
> Given we can't go back and fix it, the closest we can get is to add a
> variant of MAP_FIXED which subtracts the "REPLACE" semantic.
>
> ie: MAP_FIXED_NOREPLACE

I like MAP_FIXED_NOREPLACE.

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


Attachments:
(No filename) (2.52 kB)
signature.asc (181.00 B)
Digital signature
Download all attachments

2017-12-08 14:33:26

by David Laight

[permalink] [raw]
Subject: RE: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

From: Michael Ellerman
> Sent: 08 December 2017 11:08
...
> If we had a time machine, the right set of flags would be:
>
> - MAP_FIXED: don't treat addr as a hint, fail if addr is not free
> - MAP_REPLACE: replace an existing mapping (or force or clobber)
>
> But the two were conflated for some reason in the current MAP_FIXED.

Possibly because the original use was loading overlays?

> Given we can't go back and fix it, the closest we can get is to add a
> variant of MAP_FIXED which subtracts the "REPLACE" semantic.
>
> ie: MAP_FIXED_NOREPLACE

Much better than _SAFE - which is always bad because it is usually
one 'safe' for one specific use case.

David

2017-12-08 20:13:40

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Fri, Dec 8, 2017 at 12:33 AM, Michal Hocko <[email protected]> wrote:
> OK, this doesn't seem to lead to anywhere. The more this is discussed
> the more names we are getting. So you know what? I will resubmit and
> keep my original name. If somebody really hates it then feel free to
> nack the patch and push alternative and gain concensus on it.
>
> I will keep MAP_FIXED_SAFE because it is an alternative to MAP_FIXED so
> having that in the name is _useful_ for everybody familiar with
> MAP_FIXED already. And _SAFE suffix tells that the operation doesn't
> cause any silent memory corruptions or other unexpected side effects.

Looks like consensus is MAP_FIXED_NOREPLACE.

-Kees

--
Kees Cook
Pixel Security

2017-12-08 20:32:57

by Cyril Hrubis

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

Hi!
> > If we had a time machine, the right set of flags would be:
> >
> > - MAP_FIXED: don't treat addr as a hint, fail if addr is not free
> > - MAP_REPLACE: replace an existing mapping (or force or clobber)
>
> Actually, if we had a time machine... would we even provide
> MAP_REPLACE functionality?

I did a bit of archeology just beacause we can and since there is a git
repository of the unix history [1].

The first version of mmap() seems to appear in BSD-4_2-Snapshot there was no
MAP_FIXED flag and the addr is expected to be used for the mapping. At least
that is what manual seems to say, the kernel code is not written at this point.
This seems to correspond to a time when Berkley students were busy rewriting
UNIX kernel to take advantage of the VAX's virtual memory.

The MAP_FIXED arrived to the manual shortly after, probably someone figured out
that passing an address to the call does not make much sense in most of the
cases.

The first actual implementation that supports MAP_FIXED appeared in the
BSD-4_3_Reno-Snapshot and already includes the replace behavior. The original
purpose seems to be replacing mappings in the implementation of the execve()
call.

So the answer would probably be yes but it would probably made sense to keep it
as kernel internal flag.

And BTW it looks like HPUX got it right before it was changed to follow POSIX.
There seems to be HPUX compatibility code in the early BSD codebase that
contains both HPUXMAP_FIXED and HPUXMAP_REPLACE.

[1] https://github.com/dspinellis/unix-history-repo

--
Cyril Hrubis
[email protected]

2017-12-08 20:47:30

by Florian Weimer

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On 12/08/2017 03:27 PM, Pavel Machek wrote:
> On Fri 2017-12-08 22:08:07, Michael Ellerman wrote:
>> If we had a time machine, the right set of flags would be:
>>
>> - MAP_FIXED: don't treat addr as a hint, fail if addr is not free
>> - MAP_REPLACE: replace an existing mapping (or force or clobber)

> Actually, if we had a time machine... would we even provide
> MAP_REPLACE functionality?

Probably yes. ELF loading needs to construct a complex set of mappings
from a single file. munmap (to create a hole) followed by mmap would be
racy because another thread could have reused the gap in the meantime.
The only alternative to overriding existing mappings would be mremap
with MREMAP_FIXED, and that doesn't look like an improvement API-wise.

(The glibc dynamic linker uses an mmap call with an increased length to
reserve address space and then loads additional segments with MAP_FIXED
at the offsets specified in the program header.)

Thanks,
Florian

2017-12-08 20:57:44

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH 0/2] mm: introduce MAP_FIXED_SAFE

On Fri, Dec 08, 2017 at 12:13:31PM -0800, Kees Cook wrote:
> On Fri, Dec 8, 2017 at 12:33 AM, Michal Hocko <[email protected]> wrote:
> > OK, this doesn't seem to lead to anywhere. The more this is discussed
> > the more names we are getting. So you know what? I will resubmit and
> > keep my original name. If somebody really hates it then feel free to
> > nack the patch and push alternative and gain concensus on it.
> >
> > I will keep MAP_FIXED_SAFE because it is an alternative to MAP_FIXED so
> > having that in the name is _useful_ for everybody familiar with
> > MAP_FIXED already. And _SAFE suffix tells that the operation doesn't
> > cause any silent memory corruptions or other unexpected side effects.
>
> Looks like consensus is MAP_FIXED_NOREPLACE.

I'd rather MAP_AT_ADDR or MAP_REQUIRED, but I prefer FIXED_NOREPLACE to
FIXED_SAFE.

I just had a thought though -- MAP_STATIC? ie don't move it.