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

--- src/sys/vm/vm_mmap.c 2009-10-21 04:13:24.000000000 +0200
+++ src/sys/vm/vm_mmap.c 2009-10-21 04:13:43.000000000 +0200
@@ -245,15 +245,18 @@
  }
 
  /*
- * Align the file position to a page boundary,
- * and save its page offset component.
+ * Unless the MAP_ANON flag is set, align the file position
+ * to a page boundary and save its page offset component.
  */
- pageoff = (pos & PAGE_MASK);
- pos -= pageoff;
-
- /* Adjust size for rounding (on both ends). */
- size += pageoff; /* low end... */
- size = (vm_size_t) round_page(size); /* hi end */
+ if (flags & MAP_ANON) {
+ pageoff = pos = 0;
+ } else {
+ pageoff = (pos & PAGE_MASK);
+ pos -= pageoff;
+ /* Adjust size for rounding (on both ends). */
+ size += pageoff; /* low end... */
+ size = (vm_size_t) round_page(size); /* hi end */
+ }
 
  /*
  * Check for illegal addresses.  Watch out for address wrap... Note
@@ -300,7 +303,6 @@
  handle = NULL;
  handle_type = OBJT_DEFAULT;
  maxprot = VM_PROT_ALL;
- pos = 0;
  } else {
  /*
  * Mapping file, get fp for validation and

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

[John Baldwin]

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.

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

[Alan Cox-9]

On Wed, Oct 21, 2009 at 10:51 AM, Alexander Best <
alexbestms@...> 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
>

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-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ben Crowhurst-3]

Alan Cox wrote:

> On Wed, Oct 21, 2009 at 10:51 AM, Alexander Best <
> alexbestms@...> 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
>>
>>    
>
> 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-hackers@... mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."
>  

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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[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

--- /usr/src/sys/vm/vm_mmap.c 2009-10-28 21:37:53.000000000 +0100
+++ ./vm_mmap.c 2009-10-31 03:22:44.000000000 +0100
@@ -241,9 +241,11 @@
     ((prot & (PROT_READ | PROT_WRITE)) != (PROT_READ | PROT_WRITE)))
  return (EINVAL);
  flags |= MAP_ANON;
- pos = 0;
  }
 
+ if (flags & MAP_ANON)
+ pos = 0;
+
  /*
  * Align the file position to a page boundary,
  * and save its page offset component.
@@ -300,7 +302,6 @@
  handle = NULL;
  handle_type = OBJT_DEFAULT;
  maxprot = VM_PROT_ALL;
- pos = 0;
  } else {
  /*
  * Mapping file, get fp for validation and

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

[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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Alexander Best]

John Baldwin schrieb am 2009-11-02:
> 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.

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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[John Baldwin]

On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:

> John Baldwin schrieb am 2009-11-02:
> > 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.
>
> 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."

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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Alexander Best]

John Baldwin schrieb am 2009-11-02:
> On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:
> > John Baldwin schrieb am 2009-11-02:
> > > 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.

> > 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."

> 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. :)

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. ;)
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[John Baldwin]

On Monday 02 November 2009 5:14:27 pm Alexander Best wrote:

> John Baldwin schrieb am 2009-11-02:
> > On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:
> > > John Baldwin schrieb am 2009-11-02:
> > > > 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.
>
> > > 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."
>
> > 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. :)
>
> 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. ;)

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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Alexander Best]

John Baldwin schrieb am 2009-11-03:
> On Monday 02 November 2009 5:14:27 pm Alexander Best wrote:
> > John Baldwin schrieb am 2009-11-02:
> > > On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:
> > > > John Baldwin schrieb am 2009-11-02:
> > > > > 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.

> > > > 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."

> > > 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. :)

> > 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. ;)

> 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.

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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Alexander Best]

Alexander Best schrieb am 2009-11-03:

> John Baldwin schrieb am 2009-11-03:
> > On Monday 02 November 2009 5:14:27 pm Alexander Best wrote:
> > > John Baldwin schrieb am 2009-11-02:
> > > > On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:
> > > > > John Baldwin schrieb am 2009-11-02:
> > > > > > 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.

> > > > > 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."

> > > > 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. :)

> > > 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. ;)

> > 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.

> 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.

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.

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

[Ed Schouten-3]

Hi Alan,

* Alan Cox <alan.l.cox@...> 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 <ed@...>
 WWW: http://80386.nl/

[Alexander Best]

Alexander Best schrieb am 2009-11-03:

> Alexander Best schrieb am 2009-11-03:
> > John Baldwin schrieb am 2009-11-03:
> > > On Monday 02 November 2009 5:14:27 pm Alexander Best wrote:
> > > > John Baldwin schrieb am 2009-11-02:
> > > > > On Monday 02 November 2009 4:05:56 pm Alexander Best wrote:
> > > > > > John Baldwin schrieb am 2009-11-02:
> > > > > > > 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.

> > > > > > 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."

> > > > > 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. :)

> > > > 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. ;)

> > > 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.

> > 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.

> 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.

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.

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

[John Baldwin]

On Tuesday 03 November 2009 12:24:52 pm Ed Schouten wrote:

> Hi Alan,
>
> * Alan Cox <alan.l.cox@...> 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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."

[Ed Schouten-3]

* John Baldwin <jhb@...> 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.

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

[Alan Cox-5]

Ed Schouten wrote:
> * John Baldwin <jhb@...> 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


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

[Ed Schouten-3]

* Alan Cox <alc@...> 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

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

[Alan Cox-5]

Ed Schouten wrote:

> * Alan Cox <alc@...> 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

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

[Alexander Best]

Alan Cox schrieb am 2009-11-04:
> Ed Schouten wrote:
> >* Alan Cox <alc@...> 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
_______________________________________________
freebsd-hackers@... mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@..."