Re: [PATCH] manual: Document mprotect and introduce section on memory protection

[Rical Jasan <ricaljasan at pacific dot net>]

On 11/14/2017 06:26 AM, Florian Weimer wrote:
> 2017-11-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	manual: Document mprotect
> 	* manual/memory.texi (Memory Protection): New section.
> 	* manual/llio.texi (Memory-mapped I/O): Reference this section.
> 	Remove duplicate documentation of PROT_* flags.

"this section" sounds like it refers to Memory-mapped I/O.  What about:

"Reference the new section.  Move documentation of PROT_* flags there."

> 
> diff --git a/manual/llio.texi b/manual/llio.texi
> index 10ad546723..e635c0eaa8 100644
> --- a/manual/llio.texi
> +++ b/manual/llio.texi
> @@ -1411,19 +1411,11 @@ is created, which is not removed by closing the file.
>  address is automatically removed.  The address you give may still be
>  changed, unless you use the @code{MAP_FIXED} flag.
>  
> -@vindex PROT_READ
> -@vindex PROT_WRITE
> -@vindex PROT_EXEC
>  @var{protect} contains flags that control what kind of access is
>  permitted.  They include @code{PROT_READ}, @code{PROT_WRITE}, and
> -@code{PROT_EXEC}, which permit reading, writing, and execution,
> -respectively.  Inappropriate access will cause a segfault (@pxref{Program
> -Error Signals}).
> -
> -Note that most hardware designs cannot support write permission without
> -read permission, and many do not distinguish read and execute permission.
> -Thus, you may receive wider permissions than you ask for, and mappings of
> -write-only files may be denied even if you do not use @code{PROT_READ}.
> +@code{PROT_EXEC}.  The special flag @code{PROT_NONE} reserve a region of

"reserves"

> +address space for future use.  The @code{mprotect} function can be used
> +to change the protection flags.  @xref{Memory Protection}.
>  
>  @var{flags} contains flags that control the nature of the map.
>  One of @code{MAP_SHARED} or @code{MAP_PRIVATE} must be specified.
> diff --git a/manual/memory.texi b/manual/memory.texi
> index 51a5f4e83c..9a9340cace 100644
> --- a/manual/memory.texi
> +++ b/manual/memory.texi
> @@ -17,6 +17,7 @@ and allocation of real memory.
>  * Memory Concepts::             An introduction to concepts and terminology.
>  * Memory Allocation::           Allocating storage for your program data
>  * Resizing the Data Segment::   @code{brk}, @code{sbrk}
> +* Memory Protection::      	Controlling access to memory regions.

Looks like a tab is throwing off the alignment.

>  * Locking Pages::               Preventing page faults
>  @end menu
>  
> @@ -3047,7 +3048,120 @@ of the data segment is.
>  
>  @end deftypefun
>  
> +@node Memory Protection
> +@section Memory Protection
> +@cindex memory protection
> +@cindex page protection
> +@cindex protection flags
>  
> +When a page is mapped using @code{mmap}, page protection flags can be
> +specified using the protection flags argument.  @xref{Memory-mapped
> +I/O}.
> +
> +The following flags are availabe.

"available:"

> +
> +@table @code
> +@item PROT_WRITE
> +@vindex PROT_WRITE

Make this an @vtable and drop the @vindex entries (@items in an @vtable
are automatically @vindex-ed).  The @items should then have @standards
beneath them with the relevant standard and header, which will also add
them to the Summary of Library Facilities.

> +The memory can be written to.
> +
> +@item PROT_READ
> +@vindex PROT_READ
> +The memory can be read.  On some architectures, this flag implies that

A personal preference, but I would drop "that" as superfluous.

> +the memory can be executed as well (as if @code{PROT_EXEC} had been
> +specified at the same time).

This seems a fair warning, but how would one know if their architecture
was affected?  Is there a good general method, other than simply trying
to execute something?

> +
> +@item PROT_EXEC
> +@vindex PROT_EXEC
> +The memory can be used store instructions which can then be executed.

"used to store"

> +On most architectures, this flag implies that the memory can be read (as

Similarly for "that" here, as above.

> +if @code{PROT_READ} had been specified).

The use of "most" with the READ/EXEC caveat here contrasts with the use
of "some" above, in a good way, if that was intentional.  I think the
relationship here is more intuitive, anyway, which the language seems to
suggest.  If there is a good way to tell, though, it would be nice to be
complete and provide some direction here as well, or maybe for both
after the @table.

> +

Would an `@cindex anonymous mappings' make sense here?  (We need one
above MAP_ANON, but that's beyond the scope of this patch.)

> +@item PROT_NONE
> +@vindex PROT_NONE
> +This flag must be specified on its own.
> +
> +The memory is reserved, but cannot be read, written, or executed.  If
> +this flag is specified in a call to @code{mmap}, a virtual memory area
> +will be set aside for future use in the process, and @code{mmap} calls
> +without the @code{MAP_FIXED} flag will not use it for subsequent
> +allocations.  For anonymous mappings, the kernel will not reserve any
> +physical memory for the allocation, though.
> +@end table
> +
> +The operating system may keep of these flags separately even if the

"keep track"

> +underlying hardware treats them the same for the purposes of access
> +checking (as it happens with @code{PROT_READ} and @code{PROT_EXEC} on

I would just say, "as happens".

> +some platforms).
> +
> +Inappropriate access will cause a segfault (@pxref{Program Error
> +Signals}).
> +
> +After allocation, protection flags can be changed using the
> +@code{mprotect} function.
> +
> +@deftypefun int mprotect (void *@var{address}, size_t @var{length}, int @var{protection})
> +@standards{POSIX, sys/mman.h}
> +@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
> +
> +A successful call to the @code{mprotect} function changes the protection
> +flags of at least @var{length} bytes of memory, starting at
> +@var{address}.
> +
> +@var{address} must be aligned to the page size for the mapping.  The
> +system page size can be obtained using @samp{sysconf (_SC_PAGE_SIZE)}.

"_SC_PAGESIZE"

I believe we avoid function calls in sentences, so this should either be
reworded or made into an @[small]example.  Rewording might make it
easier to more tightly bind the reference with an @pxref.  Perhaps:

"The system page size can be obtained by calling @code{sysconf} with the
@code{_SC_PAGESIZE} parameter (@pxref{Sysconf Definition})."

> +@xref{Sysconf Definition}.  The system page size is the granularity in
> +which the page protection of anonymous memory mappings and most file
> +mappings can be changed.  Memory which is mapped from special files or
> +devices may have larger page granularity than the system page size and
> +may require larger alignment.
> +
> +@var{length} is the number of bytes whose protection flags must be
> +changed.  It is automatically rounded up to the next multiple of the
> +system page size.
> +
> +@var{protection} is a combination of the @code{PROT_}* described above.

"the @code{PROT_*} flags described above."  (Two changes: the addition
of "flags", and the manual puts the wildcard within the brace.)

Regarding the @table below, the manual has ingrained in me the
traditional introductory phrase:

"The following @code{errno} error conditions are defined for this function:"

> +
> +@table @code
> +@item ENOMEM
> +The system was not able to allocate resources to fulfill the request.
> +This can happen if there is not enough physical memory in the system for
> +the allocation of backing storage (if the memory was initially mapped
> +with @code{PROT_NONE}).  The error can also occur if the new protection

Will calling mprotect always cause backing storage to be allocated if it
wasn't already?  If so, that might be mentioned above.

Also, the PROT_NONE description says virtual memory is reserved, but the
use of "though" in the sentence about anonymous mappings makes it sound
like this error would only be encountered if PROT_NONE was used with
MAP_ANON, so it might be good to clarify whether the use of PROT_NONE
without MAP_ANON will, won't, or may allocate backing storage.

> +flags would cause the memory region to be split from its neighbors, and
> +the process limit for the number of such distinct memory regions would
> +be exceeded.

Is there a way to know that limit, and the process' current count?  May
be TMI, but these kinds of warnings always make me wonder.

> +
> +@item EINVAL
> +@var{address} is not properly aligned to a page boundary for the
> +mapping, or @var{length} (after rounding up to the system page size) is
> +not a multiple of the applicable page size for the mapping, or the
> +combination of flags in @var{protection} is not valid.
> +
> +@item EACCES
> +The file for a file-based mapping was not opened with flags which match
> +@var{protection}.

Isn't mprotect supposed to set the flags to protection?  Is there
somewhere we can reference what is special about file-based mappings
such that they can prevent the use of mprotect?  How is this different
from EPERM, below?  The content below the @deftypefun makes it sound
like this might only be returned by some systems.

> +
> +@item EPERM
> +The system security policy does not allow a mapping with the specified
> +flags.  For example, mappings which are both @code{PROT_EXEC} and
> +@code{PROT_WRITE} at the same time might not be allowed.
> +@end table
> +@end deftypefun
> +
> +If the @code{mprotect} function is used to make a region of memory
> +inaccessible by specifying the @code{PROT_NONE} protection flag and
> +access is later restored, the memory retains its previous contents.
> +
> +On some systems, it may not be possible to specify additional flags
> +which were not present when the mapping was first created.  For example,
> +an attempt make a region of memory executable could fail if the initial

"attempt to make"

> +protection flags where @samp{PROT_READ | PROT_WRITE}.

"were"

> +
> +In general, the @code{mprotect} function can be used to change any
> +process memory, no matter how it was allocated.  However, portable use
> +of the function requires that it is only used with memory regions
> +returned by @code{mmap} or @code{mmap64}.
>  
>  @node Locking Pages
>  @section Locking Pages
> 

I think this is quality documentation.  Thank you.

Rical