diff mbox series

manual: Use more precise wording to describe memory protection keys

Message ID 20241022073837.151355-1-yury.khrustalev@arm.com
State New
Headers show
Series manual: Use more precise wording to describe memory protection keys | expand

Commit Message

Yury Khrustalev Oct. 22, 2024, 7:38 a.m. UTC
Update the name of the argument in several pkey_*() functions that refers
to access restrictions rather than access rights: change (access) "rights"
to "restrictions".

Specify that the result of the pkey_get() should be checked using bitwise
operations rather than plain equals comparison.

Update corresponding header and function prototypes accordingly.

---

Is it OK for trunk?

Thanks,
Yury

---
 manual/memory.texi                         | 52 ++++++++++++----------
 sysdeps/unix/sysv/linux/bits/mman-shared.h | 13 +++---
 sysdeps/unix/sysv/linux/pkey_set.c         |  2 +-
 3 files changed, 36 insertions(+), 31 deletions(-)

Comments

Florian Weimer Oct. 22, 2024, 8:48 a.m. UTC | #1
* Yury Khrustalev:

> diff --git a/sysdeps/unix/sysv/linux/bits/mman-shared.h b/sysdeps/unix/sysv/linux/bits/mman-shared.h
> index d8ed4436b6..44bf52be7e 100644
> --- a/sysdeps/unix/sysv/linux/bits/mman-shared.h
> +++ b/sysdeps/unix/sysv/linux/bits/mman-shared.h
> @@ -42,8 +42,9 @@
>  #  define MLOCK_ONFAULT 1U
>  # endif
>  
> -/* Access rights for pkey_alloc.  */
> +/* Access restrictions for pkey_alloc.  */
>  # ifndef PKEY_DISABLE_ACCESS
> +#  define PKEY_UNRESTRICTED 0x0
>  #  define PKEY_DISABLE_ACCESS 0x1
>  #  define PKEY_DISABLE_WRITE 0x2
>  # endif

I'd prefer to have this in a separate patch from the documentation
updates.

I think you should add PKEY_UNRESTRICTED to the Linux UAPI headers
first.  They currently define the PKEY_DISABLE_* constants.

Thanks,
Florian
Yury Khrustalev Oct. 22, 2024, 12:48 p.m. UTC | #2
On Tue, Oct 22, 2024 at 10:48:26AM +0200, Florian Weimer wrote:

> > -/* Access rights for pkey_alloc.  */
> > +/* Access restrictions for pkey_alloc.  */
> >  # ifndef PKEY_DISABLE_ACCESS
> > +#  define PKEY_UNRESTRICTED 0x0
> >  #  define PKEY_DISABLE_ACCESS 0x1
> >  #  define PKEY_DISABLE_WRITE 0x2
> >  # endif
> 
> I'd prefer to have this in a separate patch from the documentation
> updates.
> 
> I think you should add PKEY_UNRESTRICTED to the Linux UAPI headers
> first.  They currently define the PKEY_DISABLE_* constants.

I've made separate commits for documentation and headers [1]. I've
also submitted patch for adding new macro to Linux UAPI headers [2],
so as soon as it is merged, I will send another patch to introduce
it to Glibc.

I hope this is OK?

[1] https://inbox.sourceware.org/libc-alpha/20241022124235.367746-1-yury.khrustalev@arm.com/
[2] https://lore.kernel.org/linux-arch/20241022120128.359652-1-yury.khrustalev@arm.com/

Thanks,
Yury
diff mbox series

Patch

diff --git a/manual/memory.texi b/manual/memory.texi
index 58683ee93d..e732272db1 100644
--- a/manual/memory.texi
+++ b/manual/memory.texi
@@ -3089,27 +3089,27 @@  during memory accesses.  New keys can be allocated with the
 @code{pkey_mprotect}.
 
 @item
-Each thread has a set of separate access right restriction for each
-protection key.  These access rights can be manipulated using the
+Each thread has a set of separate access restrictions for each
+protection key.  These access restrictions can be manipulated using the
 @code{pkey_set} and @code{pkey_get} functions.
 
 @item
 During a memory access, the system obtains the protection key for the
-accessed page and uses that to determine the applicable access rights,
+accessed page and uses that to determine the applicable access restrictions,
 as configured for the current thread.  If the access is restricted, a
 segmentation fault is the result ((@pxref{Program Error Signals}).
 These checks happen in addition to the @code{PROT_}* protection flags
 set by @code{mprotect} or @code{pkey_mprotect}.
 @end itemize
 
-New threads and subprocesses inherit the access rights of the current
+New threads and subprocesses inherit the access restrictions of the current
 thread.  If a protection key is allocated subsequently, existing threads
 (except the current) will use an unspecified system default for the
-access rights associated with newly allocated keys.
+access restrictions associated with newly allocated keys.
 
-Upon entering a signal handler, the system resets the access rights of
+Upon entering a signal handler, the system resets the access restrictions of
 the current thread so that pages with the default key can be accessed,
-but the access rights for other protection keys are unspecified.
+but the access restrictions for other protection keys are unspecified.
 
 Applications are expected to allocate a key once using
 @code{pkey_alloc}, and apply the key to memory regions which need
@@ -3151,14 +3151,14 @@  it again:
 
 In this example, a negative key value indicates that no key had been
 allocated, which means that the system lacks support for memory
-protection keys and it is not necessary to change the the access rights
+protection keys and it is not necessary to change the the access restrictions
 of the current thread (because it always has access).
 
 Compared to using @code{mprotect} to change the page protection flags,
 this approach has two advantages: It is thread-safe in the sense that
-the access rights are only changed for the current thread, so another
-thread which changes its own access rights concurrently to gain access
-to the mapping will not suddenly see its access rights revoked.  And
+the access restrictions are only changed for the current thread, so another
+thread which changes its own access restrictions concurrently to gain access
+to the mapping will not suddenly see its access restrictions updated.  And
 @code{pkey_set} typically does not involve a call into the kernel and a
 context switch, so it is more efficient.
 
@@ -3166,9 +3166,9 @@  context switch, so it is more efficient.
 @standards{Linux, sys/mman.h}
 @safety{@prelim{}@mtsafe{}@assafe{}@acunsafe{@acucorrupt{}}}
 Allocate a new protection key.  The @var{flags} argument is reserved and
-must be zero.  The @var{restrictions} argument specifies access rights
+must be zero.  The @var{restrictions} argument specifies access restrictions
 which are applied to the current thread (as if with @code{pkey_set}
-below).  Access rights of other threads are not changed.
+below).  Access restrictions of other threads are not changed.
 
 The function returns the new protection key, a non-negative number, or
 @math{-1} on error.
@@ -3203,7 +3203,7 @@  in which memory protection keys are disabled.
 Deallocate the protection key, so that it can be reused by
 @code{pkey_alloc}.
 
-Calling this function does not change the access rights of the freed
+Calling this function does not change the access restrictions of the freed
 protection key.  The calling thread and other threads may retain access
 to it, even if it is subsequently allocated again.  For this reason, it
 is not recommended to call the @code{pkey_free} function.
@@ -3251,14 +3251,14 @@  not @math{-1}.
 @end table
 @end deftypefun
 
-@deftypefun int pkey_set (int @var{key}, unsigned int @var{rights})
+@deftypefun int pkey_set (int @var{key}, unsigned int @var{restrictions})
 @standards{Linux, sys/mman.h}
 @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
-Change the access rights of the current thread for memory pages with the
-protection key @var{key} to @var{rights}.  If @var{rights} is zero, no
-additional access restrictions on top of the page protection flags are
-applied.  Otherwise, @var{rights} is a combination of the following
-flags:
+Change the access restrictions of the current thread for memory pages with the
+protection key @var{key} to @var{restrictions}.  If @var{restrictions} is
+@code{PKEY_UNRESTRICTED} (zero), no additional access restrictions on top of
+the page protection flags are applied.  Otherwise, @var{restrictions} is a
+combination of the following flags:
 
 @vtable @code
 @item PKEY_DISABLE_WRITE
@@ -3290,18 +3290,22 @@  function:
 
 @table @code
 @item EINVAL
-The system does not support the access rights restrictions expressed in
-the @var{rights} argument.
+The system does not support the access restrictions expressed in
+the @var{restrictions} argument.
 @end table
 @end deftypefun
 
 @deftypefun int pkey_get (int @var{key})
 @standards{Linux, sys/mman.h}
 @safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
-Return the access rights of the current thread for memory pages with
-protection key @var{key}.  The return value is zero or a combination of
+Return the access restrictions of the current thread for memory pages
+with protection key @var{key}.  The return value is zero or a combination of
 the @code{PKEY_DISABLE_}* flags; see the @code{pkey_set} function.
 
+The returned value should be checked for presence or absence of specific flags
+using bitwise operations.  Comparing the returned value with any of the flags
+or their combination using equals will almost certainly fail.
+
 Calling the @code{pkey_get} function with a protection key which was not
 allocated by @code{pkey_alloc} results in undefined behavior.  This
 means that calling this function on systems which do not support memory
diff --git a/sysdeps/unix/sysv/linux/bits/mman-shared.h b/sysdeps/unix/sysv/linux/bits/mman-shared.h
index d8ed4436b6..44bf52be7e 100644
--- a/sysdeps/unix/sysv/linux/bits/mman-shared.h
+++ b/sysdeps/unix/sysv/linux/bits/mman-shared.h
@@ -42,8 +42,9 @@ 
 #  define MLOCK_ONFAULT 1U
 # endif
 
-/* Access rights for pkey_alloc.  */
+/* Access restrictions for pkey_alloc.  */
 # ifndef PKEY_DISABLE_ACCESS
+#  define PKEY_UNRESTRICTED 0x0
 #  define PKEY_DISABLE_ACCESS 0x1
 #  define PKEY_DISABLE_WRITE 0x2
 # endif
@@ -59,16 +60,16 @@  int memfd_create (const char *__name, unsigned int __flags) __THROW;
 int mlock2 (const void *__addr, size_t __length, unsigned int __flags) __THROW;
 
 /* Allocate a new protection key, with the PKEY_DISABLE_* bits
-   specified in ACCESS_RIGHTS.  The protection key mask for the
+   specified in RESTRICTIONS.  The protection key mask for the
    current thread is updated to match the access privilege for the new
    key.  */
-int pkey_alloc (unsigned int __flags, unsigned int __access_rights) __THROW;
+int pkey_alloc (unsigned int __flags, unsigned int __restrictions) __THROW;
 
-/* Update the access rights for the current thread for KEY, which must
+/* Update the access restrictions for the current thread for KEY, which must
    have been allocated using pkey_alloc.  */
-int pkey_set (int __key, unsigned int __access_rights) __THROW;
+int pkey_set (int __key, unsigned int __restrictions) __THROW;
 
-/* Return the access rights for the current thread for KEY, which must
+/* Return the restrictions for the current thread for KEY, which must
    have been allocated using pkey_alloc.  */
 int pkey_get (int __key) __THROW;
 
diff --git a/sysdeps/unix/sysv/linux/pkey_set.c b/sysdeps/unix/sysv/linux/pkey_set.c
index 30463ef89b..cd1f806803 100644
--- a/sysdeps/unix/sysv/linux/pkey_set.c
+++ b/sysdeps/unix/sysv/linux/pkey_set.c
@@ -20,7 +20,7 @@ 
 #include <sys/mman.h>
 
 int
-__pkey_set (int key, unsigned int access_rights)
+__pkey_set (int key, unsigned int restrictions)
 {
   __set_errno (ENOSYS);
   return -1;