mmap(2) with MAP_ANON honouring offset although it shouldn't

[Alexander Best]

although the mmap(2) manual states in section MAP_ANON:

"The offset argument is ignored."

this doesn't seem to be true. running

printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1,
0x12345678));

and

printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1, 0));

produces different outputs. i've attached a patch to solve the problem. the
patch is similar to the one proposed in this PR, but should apply cleanly to
CURRENT: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/71258

cheers.
alex

[Alan Cox]

The standards for mmap(2) actually disallow values of "off" that are not a
multiple of the page size.

See http://www.opengroup.org/onlinepubs/000095399/functions/mmap.html for
the following:
[EINVAL]The *addr* argument (if MAP_FIXED was specified) or *off* is not a
multiple of the page size as returned by
*sysconf*()<http://www.opengroup.org/onlinepubs/000095399/functions/sysconf.html>,
or is considered invalid by the implementation.Both Solaris and Linux
enforce this restriction.

I'm not convinced that the ability to specify a value for "off" that is not
a multiple of the page size is a useful differentiating feature of FreeBSD
versus Solaris or Linux. Does anyone have a compelling argument (or use
case) to motivate us being different in this respect?

If you disallow values for "off" that are not a multiple of the page size,
then you are effectively ignoring "off" for MAP_ANON.

Regards,
Alan
_______________________________________________
freebsd...@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hacke...@freebsd.org"

[John Baldwin]

A simpler patch would be to simply set pos = 0 below the MAP_STACK line if
MAP_ANON is set.

--
John Baldwin

[Ben Crowhurst]

would it be such a bad idea as to round up the addr/off arguments to the
next page size? This was most likely the intention of the caller anyway.

Cheers,
Ben

[Alexander Best]

John Baldwin schrieb am 2009-10-21:
> On Wednesday 21 October 2009 11:51:04 am Alexander Best wrote:
> > although the mmap(2) manual states in section MAP_ANON:

> > "The offset argument is ignored."

> > this doesn't seem to be true. running

> > printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1,
> > 0x12345678));

> > and

> > printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1,
> > 0));

> > produces different outputs. i've attached a patch to solve the
> > problem. the
> > patch is similar to the one proposed in this PR, but should apply
> > cleanly to
> > CURRENT: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/71258

> A simpler patch would be to simply set pos = 0 below the MAP_STACK
> line if
> MAP_ANON is set.

how about the following patch. problem seems to be that pos = 0 needs to be
set before pageoff is being calculated.

i've tested mmap with MAP_STACK and the offset gets discarded just as
documented in mmap(2). with the patch the offset handling with MAP_ANON and
MAP_STACK (implies MAP_ANON) are the same.

another short question:

why does the second call when doing

printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_READ|PROT_WRITE,
MAP_STACK, -1, 0x0));
printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_READ|PROT_WRITE,
MAP_STACK, -1, 0x0));

fail? doesn't MAP_STACK allow mapping the same region twice?

printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_READ|PROT_WRITE,
MAP_STACK, -1, 0x0));
printf("%p\n", mmap((void*)0x2000, 0x1000, PROT_READ|PROT_WRITE,
MAP_STACK, -1, 0x0));

works just as expected.

cheers.
alex

[John Baldwin]

On Friday 30 October 2009 10:38:24 pm Alexander Best wrote:
> John Baldwin schrieb am 2009-10-21:
> > On Wednesday 21 October 2009 11:51:04 am Alexander Best wrote:
> > > although the mmap(2) manual states in section MAP_ANON:
>
> > > "The offset argument is ignored."
>
> > > this doesn't seem to be true. running
>
> > > printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1,
> > > 0x12345678));
>
> > > and
>
> > > printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_NONE, MAP_ANON, -1,
> > > 0));
>
> > > produces different outputs. i've attached a patch to solve the
> > > problem. the
> > > patch is similar to the one proposed in this PR, but should apply
> > > cleanly to
> > > CURRENT: http://www.freebsd.org/cgi/query-pr.cgi?pr=kern/71258
>
> > A simpler patch would be to simply set pos = 0 below the MAP_STACK
> > line if
> > MAP_ANON is set.
>
> how about the following patch. problem seems to be that pos = 0 needs to be
> set before pageoff is being calculated.

I think that that patch is fine, but will defer to alc@. I think he argued
that any non-zero offset passed to MAP_ANON should fail with EINVAL.

--
John Baldwin

[Alexander Best]

> > > > and

thanks. if that's what the POSIX standard requests that's ok. however in that
case we need to change the mmap(2) manual, because right now it says in
section MAP_ANON:

"The offset argument is ignored."

which should be changed to something like:

"The offset argument must be zero."

also if the behaviour of MAP_ANON changes this also changes the semantics of
MAP_STACK since it implies MAP_ANON. so we need to decide if MAP_STACK should
silently reset any offset value to zero or like MAP_ANON should fail if offset
isn't zero in which case the MAP_STACK section of the mmap(2) manual needs to
be changed to someting like:

"MAP_STACK implies MAP_ANON, and requires offset to be zero."

cheers.
alex

[John Baldwin]

Right now MAP_STACK sets pos to 0 in the current code, and I don't expect we
would remove that if we decide to reject non-zero offsets for MAP_ANON. I'd
probably rather err on the side of leniency and just ignore the offset rather
than rejecting non-zero, but I'm a bit burned from the last round of mmap()
API changes. :)

--
John Baldwin

[Alexander Best]

> > > > > > and

hmmm...i think this will require quite a few changes. if i remember correctly
MAP_STACK at some point does:

flags =| MAP_ANON;

so if we decide MAP_ANON and MAP_STACK should behave differently this will
require some checks to distinguish between both flags further down in the
code.

let's see what alc@ thinks about this one then. API changes are a nasty nasty
business. ;)

[John Baldwin]

Umm, if you revert your change and just add a simple clause that does:

if (flags & MAP_ANON && pos != 0)
return (EINVAL);

after the MAP_STACK section then I think that would work fine. It would
not require any further magic apart from that.

--
John Baldwin

[Alexander Best]

> > > > > > > > and

> > flags =| MAP_ANON;

oh. you're right. didn't think of that one. indeed this would let mmap fail
with MAP_ANON and pos != 0, but would keep the current MAP_STACK behaviour
(which is ignoring pos).

sounds like a really clean and useful mmap API change. if alc@ agrees i could
put your change in the form of a patch and together with a mmap(2) manual
change, submit it as followup to kern/71258. it shouldn't be a big deal
mfc'ing the changes to 8-stable (maybe even 8.0-release), 7-stable and
6-stable. well...better make that 8.1-release. ;) who knows what weird mmap
calls are in the ports. ;)

i'll try to build universe over the night to see if the changes break
anything.

alex

[Alexander Best]

> > > > > > > > > and

> > > flags =| MAP_ANON;

just realised that building universe or only world is pretty useless since the
API changes only affect apps during runtime and at compilation time. :)

i've run a few tests. the following app:

#include <sys/types.h>
#include <sys/mman.h>
#include <stdio.h>

main() {
printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_READ|PROT_WRITE,
MAP_STACK, -1, 1));
printf("%p\n", mmap((void*)0x1000, 0x1000, PROT_READ|PROT_WRITE, MAP_ANON,
-1, 1));
}

outputs:

0x1000
0xffffffff

as expected.

#include <sys/types.h>
#include <sys/mman.h>
#include <stdio.h>

main() {
printf("%p\n", mmap((void*)0, 0x1000, PROT_READ|PROT_WRITE, MAP_STACK, -1,
0));
printf("%p\n", mmap((void*)0, 0x1000, PROT_READ|PROT_WRITE, MAP_ANON, -1,
0));
}

however produces this output:

0xffffffff
0x28195000

which seems a bit odd. the mmap(2) manual doesn't say anything about MAP_STACK
not working when addr is zero.

i'll see if this is caused by the changes jhb@ suggested or not.

[Ed Schouten]

Hi Alan,

* Alan Cox <alan....@gmail.com> wrote:
> The standards for mmap(2) actually disallow values of "off" that are not a
> multiple of the page size.
>
> See http://www.opengroup.org/onlinepubs/000095399/functions/mmap.html for
> the following:

> <snip>

Just by accident I saw they changed that behaviour in a newer version of
the spec:

http://www.opengroup.org/onlinepubs/9699919799/functions/mmap.html

--
Ed Schouten <e...@80386.nl>
WWW: http://80386.nl/

[Alexander Best]

> > > > > > > > > > and

> > > > flags =| MAP_ANON;

> outputs:

> 0x1000
> 0xffffffff

> as expected.

> however produces this output:

> 0xffffffff
> 0x28195000

ok. checked it. not being caused by your changes. maybe i missed something and
in fact MAP_STACK requires addr to be non zero. couldn't find it in the
mmap(2) manual though.

[John Baldwin]

On Tuesday 03 November 2009 12:24:52 pm Ed Schouten wrote:
> Hi Alan,
>
> * Alan Cox <alan....@gmail.com> wrote:
> > The standards for mmap(2) actually disallow values of "off" that are not a
> > multiple of the page size.
> >
> > See http://www.opengroup.org/onlinepubs/000095399/functions/mmap.html for
> > the following:
> > <snip>
>
> Just by accident I saw they changed that behaviour in a newer version of
> the spec:
>
> http://www.opengroup.org/onlinepubs/9699919799/functions/mmap.html

Note that the spec doesn't cover MAP_ANON at all FWIW.

--
John Baldwin

[Ed Schouten]

* John Baldwin <j...@freebsd.org> wrote:
> Note that the spec doesn't cover MAP_ANON at all FWIW.

Yes. I've noticed Linux also uses MAP_ANONYMOUS instead of MAP_ANON.
They do provide MAP_ANON for compatibility, if I remember correctly.

[Alan Cox]

Ed Schouten wrote:
> * John Baldwin <j...@freebsd.org> wrote:
>
>> Note that the spec doesn't cover MAP_ANON at all FWIW.
>>
>
> Yes. I've noticed Linux also uses MAP_ANONYMOUS instead of MAP_ANON.
> They do provide MAP_ANON for compatibility, if I remember correctly.
>
>

For what it's worth, I believe that Solaris does the exact opposite.
They provide MAP_ANONYMOUS for compatibility. It seems like a good idea
for us to do the same.

We also have an unimplemented option MAP_RENAME defined for
compatibility with "Sun" that is nowhere mentioned in modern Solaris
documentation.

Alan

[Ed Schouten]

* Alan Cox <a...@cs.rice.edu> wrote:
> For what it's worth, I believe that Solaris does the exact opposite.
> They provide MAP_ANONYMOUS for compatibility. It seems like a good
> idea for us to do the same.

Something like this?

Index: mman.h
===================================================================
--- mman.h (revision 198919)
+++ mman.h (working copy)
@@ -82,6 +82,9 @@
*/
#define MAP_FILE 0x0000 /* map from file (default) */
#define MAP_ANON 0x1000 /* allocated from memory, swap space */
+#ifndef _KERNEL
+#define MAP_ANONYMOUS MAP_ANON /* For compatibility. */
+#endif /* !_KERNEL */

/*
* Extended flags

[Alan Cox]

Yes. If no one objects in the next day or so, then please commit this
change.

[Alexander Best]

Alan Cox schrieb am 2009-11-04:
> Ed Schouten wrote:
> >* Alan Cox <a...@cs.rice.edu> wrote:

> For what it's worth, I believe that Solaris does the exact opposite.
> >>They provide MAP_ANONYMOUS for compatibility. It seems like a good
> >>idea for us to do the same.

> >Something like this?

> >Index: mman.h
> >===================================================================
> >--- mman.h (revision 198919)
> >+++ mman.h (working copy)
> >@@ -82,6 +82,9 @@
> > */
> >#define MAP_FILE 0x0000 /* map from file (default) */
> >#define MAP_ANON 0x1000 /* allocated from memory,
> >swap space */
> >+#ifndef _KERNEL
> >+#define MAP_ANONYMOUS MAP_ANON /* For compatibility. */
> >+#endif /* !_KERNEL */
> > /*
> > * Extended flags

> Yes. If no one objects in the next day or so, then please commit
> this change.

> Alan

should this compatibility addition be documented in the mmap(2) manual?

any thoughts on the previous change request so mmap fails with MAP_ANON and
pos=0?

alex

[Alan Cox]

I don't have a strong opinion one way or the other, but I would lean
toward "yes". That said, Solaris doesn't.

> any thoughts on the previous change request so mmap fails with MAP_ANON and
> pos=0?
>
>

Unfortunately, no. My FreeBSD time for the last few days has been spent
looking into some anomalies in wiring memory and writing breakpoints.

Alan

[Alexander Best]

no problem. i've sent the final patch as followup to kern/71258 and also
attached it to this message. to make it short. what's being changed by the
patch:

1) if MAP_ANON is defined and offset !=0 ====> return EINVAL
2) if MAP_STACK is defined and offset !=0 ====> offset = 0

would be great if you could have a look at the patch if you've got a spare
minute.

cheers.
alex

[Alexander Best]

John Baldwin schrieb am 2009-11-09:

> I didn't think 2) changed? I.e. both the old and new code do this,
> so only 1)
> is changing?

you're right sorry about that mistake. so the only aspect of mmap() the patch
changes is:

if MAP_ANON is defined and offset !=0 ====> return EINVAL

cheers.
alex