Re: [PATCH] [RFC] add manpages for Memory Protection Keys

["Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Dave,

On 03/10/2016 08:43 PM, Dave Hansen wrote:
> Michael, thanks again for the detailed review.  I've tried to respond to
> all your comments.  Here's an incremental patch from the last one.  I'll
> also resend the entire thing shortly.

Okay. (I'll wait for the resend before further review.)

> 	https://www.sr71.net/~dave/intel/pkey-man-20160310.0.patch
> 
> Just a few questions to clarify some of your review comments below...
> 
> On 03/10/2016 09:07 AM, Michael Kerrisk (man-pages) wrote:
>> On 03/09/2016 10:40 PM, Dave Hansen wrote:
>>> +.PP
>>> +.I flags
>>> +may contain zero or more disable operations:
>>> +.TP
>>> +.B PKEY_DISABLE_ACCESS
>>> +Disable all data access to memory covered by the returned protection key.
>>> +.TP
>>> +.B PKEY_DISABLE_WRITE
>>> +Disable write access to memory covered by the returned protection key.
>>> +.SH RETURN VALUE
>>> +On success,
>>> +.BR pkey_alloc ()
>>> +returns a positive protection key value.
>>> +.BR pkey_free ()
>>> +returns zero.
>>> +On error, \-1 is returned, and
>>> +.I errno
>>> +is set appropriately.
>>> +.SH ERRORS
>>> +.TP
>>> +.B EINVAL
>>> +.IR pkey ,
>>> +.IR flags ,
>>> +or
>>> +.I access_rights
>>> +is invalid.
>>> +.TP
>>> +.B ENOSPC
>>
>> At the start of the following paragraph, add
>>
>> .(RB pkey_alloc ())
>>
>> so that the reader knows that this error applies only for that syscall.
> 
> I'm not seeing that actually affect the formatting in the resulting
> manpage as I view it.  Is that really the syntax you want?  Is there
> another example bit of syntax I should be looking for?

Sorry!

.RB ( pkey_alloc ())

> ...
>> In a previous review of the pages, I asked:
>>
>> [[
>> And I have a question (and the answer probably should 
>> be documented in the manual page).  What happens when 
>> one signal handler interrupts the execution of another? 
>> Do pkey_set() calls in the first handler persist into the 
>> second handler? I presume not, but it would be good to 
>> be a little more explicit about this.
>> ]]
>>
>> I think this point does need to be covered in the man page.
> 
> I did reword some things in response to this review comment, but not
> thoroughly enough. :)
> 
> How about the following as a change to the existing second paragraph?
> 
> The effects of a call to
> .BR pkey_set ()
> from a signal handler will not persist when the signal handler
> returns.  This is true whether the return is to a normal,
> non-signal context, or to another signal handler.

How about this:

The effects of a call to
.BR pkey_set ()
from a signal handler will not persist when control passes out of
the signal handler.
This is true both when the handler returns to a normal,
nonsignal context, and when the signal handler is interrupted
by another signal handler.


> 
> ...
>>> +
>>> +To use this feature, the processor must support it, and Linux
>>> +must contain support for the feature on a given processor.  As of
>>> +early 2016 only future Intel x86 processors are supported, and this
>>> +hardware supports 16 protection keys in each process.  However,
>>> +pkey 0 is used as the default key, so a maximum of 15 are available
>>> +for actual application use.
>>
>> Is there a recommended way for an application to discover whether the
>> system supports pkeys? If so, that should be documented here.
> 
> I've added the following text:
> 
> Any application wanting to use protection keys needs to be able
> to function without them.  They might be unavailable because the
> hardware the application runs on does not support them, the

s/hardware/hardware that/
(for easier parsing)

> kernel code does not contain support, the kernel support has been
> disabled, or because the keys have all been allocated, perhaps by
> a library the application is using.  It is recommended that
> applications wanting to use protection keys should simply call
> .BR pkey_alloc()

s/()/ ()/

> instead of attempting to detect support for the
> feature in any other way.
> 
> Hardware support for protection keys may be enumerated with
> the cpuid instruction.  Details on how to do this can be
> found in the Intel Software Developers Manual.  The kernel
> performs this enumeration and exposes the information in
> /proc/cpuinfo under the "flags" field.  "pku" in this field
> indicates hardware support for protection keys and "ospke"
> indicates that the kernel contains and has enabled protection
> keys support,.
> 
> ...
>>> +.BR pkey_mprotect (2),
>>> +.BR pkey_alloc (2),
>>> +.BR pkey_free (2),
>>> +.BR pkey_set (2),
>>> +.BR pkey_get (2),
>>
>> Would it be possible to get a small, complete working example program
>> in one of these pages? The axample could show how pkeys override
>> traditional memory protections. I appreciate that the rest of us do
>> not yet have suitable hardware, but presumably you do.
> 
> Sure, I'll include one in pkey(7).

Thanks.

Cheers,

Michael



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html