Document MAP_GUARD.
ClosedPublic

Authored by kib on Jun 22 2017, 5:15 PM.

Details

Reviewers
alc
emaste
markj
pho
bjk
Group Reviewers
manpages

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
kib created this revision.Jun 22 2017, 5:15 PM
emaste added inline comments.Jun 22 2017, 5:44 PM
lib/libc/sys/mmap.2
204

"within its address range" or "overapping its address range" or something like that?
and in light of the below (MAP_FIXED), what about:

The guard prevents mappings without the MAP_FIXED flag from overlapping its address range.
The range may become part of a mapping after the guard is unmapped, or another mapping is created with the MAP_FIXED flag.

209

results in the SIGSEGV signal being delivered to the thread.

213

Guards allow the reservations to be created in the address space, which can be replaced by actual mappings later on.

For instance, the system uses guards to prevent the inadvertent use of regions into which stacks created with MAP_STACK will automatically grow.

markj added inline comments.Jun 22 2017, 5:47 PM
lib/libc/sys/mmap.2
203

"a mapping"

204

I would write this sentence as

"mmap(2) will not create mappings in the address range of a guard unless the request specifies MAP_FIXED. Guards can be destroyed with munmap(2)."

In particular, since we're being careful to distinguish between guards and mappings, it seems strange to state that a guard can be unmapped.

209

"results in the delivery of a SIGSEGV signal to the thread"

213

"Guards allow a process to create reservations in its address space, which..."

215

"the system"

emaste added inline comments.Jun 22 2017, 6:02 PM
lib/libc/sys/mmap.2
204

Sounds good.

kib marked 6 inline comments as done.Jun 22 2017, 6:05 PM
kib updated this revision to Diff 29955.Jun 22 2017, 6:08 PM

I combined the feedback, selecting the parts from each. I also added half-sentence mentioning that stacks are not mapped fully in advance.

emaste accepted this revision.Jun 22 2017, 6:17 PM
emaste added inline comments.
lib/libc/sys/mmap.2
215

which can later be replaced by actual mappings.

This revision is now accepted and ready to land.Jun 22 2017, 6:17 PM
kib updated this revision to Diff 29956.Jun 22 2017, 6:22 PM
kib edited edge metadata.
kib marked an inline comment as done.

Ed' note.

This revision now requires review to proceed.Jun 22 2017, 6:22 PM
emaste accepted this revision.Jun 22 2017, 6:24 PM
emaste added a subscriber: wblock.

Sounds good to me; if @wblock or other manpage folks have input it can be incorporated into the commit or as a follow-on.

This revision is now accepted and ready to land.Jun 22 2017, 6:24 PM
markj accepted this revision.Jun 22 2017, 6:36 PM
bjk accepted this revision.Jun 23 2017, 3:42 AM
bjk added a subscriber: bjk.

Looks fine to this manpages member

lib/libc/sys/mmap.2
212

I mayyyybe could see "the thread" being ambiguous to some hypothetical reader (as opposed to "the thread performing the access"), but it's very hard to imagine this causing confusion.

alc edited edge metadata.Jun 23 2017, 5:47 PM

I think that there are a few things missing here.

  1. If I'm using MAP_STACK, do I have to use MAP_GUARD in some way? I don't think the words "the system uses guards" are specific enough. That said, the way that I would handle this is to update the description of MAP_STACK.
  1. What happens if I specify MAP_GUARD in conjunction with a "prot" other than PROT_NONE or an "fd" other than -1?
  1. What happens to a guard on a fork()?
lib/libc/sys/mmap.2
212

I don't see the harm in tweaking the sentence to avoid any possible confusion: "Any memory access by a thread to the guarded range results in the delivery of a SIGSEGV to that thread."

216–219

I would suggest moving this (minus the "For instance,") to the description of MAP_STACK.

kib updated this revision to Diff 30005.Jun 23 2017, 6:05 PM
kib edited edge metadata.

Move note about MAP_STACK use of MAP_GUARD to MAP_STACK block.
Document EINVAL on invalid MAP_GUARD call, explicitely listing the requirements.

Also remove MAP_HASSEMAPHORE, this will be the preliminary conflict. The option is in sys/mman.h, it is not useful but AFAIK cannot be easily removed.

This revision now requires review to proceed.Jun 23 2017, 6:05 PM
emaste accepted this revision.Jun 23 2017, 6:09 PM
This revision is now accepted and ready to land.Jun 23 2017, 6:09 PM
markj accepted this revision.Jun 23 2017, 6:11 PM
alc added inline comments.Jun 23 2017, 7:32 PM
lib/libc/sys/mmap.2
481

I would drop the "either". It's generally accepted to mean exclusivity, and you want an inclusive "or" here.

483

Drop this "or". The final "or" suffices.

kib updated this revision to Diff 30012.Jun 23 2017, 8:04 PM
kib edited edge metadata.

Edit the description of an error from MAP_GUARD.

This revision now requires review to proceed.Jun 23 2017, 8:04 PM
emaste accepted this revision.Jun 23 2017, 9:44 PM
This revision is now accepted and ready to land.Jun 23 2017, 9:44 PM
alc added a comment.Jun 23 2017, 9:56 PM

This needs updating:

[EINVAL]           None of MAP_ANON, MAP_PRIVATE, MAP_SHARED, or
                   MAP_STACK was specified.  At least one of these flags
                   must be included.
alc added a comment.Jun 23 2017, 10:09 PM

Here is an unrelated typo in the map page:

"truncated and the process later accesses a pages that is wholly within"

"pages" should be "page".

bjk accepted this revision.Jun 24 2017, 4:03 AM
kib updated this revision to Diff 30027.Jun 24 2017, 9:54 AM
kib edited edge metadata.

More errors explanations.
Fix unrelated typo.

This revision now requires review to proceed.Jun 24 2017, 9:54 AM
pho accepted this revision.Jun 24 2017, 10:45 AM

LGTM

This revision is now accepted and ready to land.Jun 24 2017, 10:45 AM
alc accepted this revision.Jun 24 2017, 3:41 PM
kib closed this revision.Jun 24 2017, 5:10 PM
wblock added inline comments.Jul 7 2017, 5:48 PM
lib/libc/sys/mmap.2
335

This is complex. Suggested:

Stacks created with
.Dv MAP_STACK
automatically grow.
Guards prevent inadvertent use of the regions into which those stacks can
grow without requiring mapping the whole stack in advance.