From: Ira Weiny <redacted>
While evaluating the possibility of defining a new type for pkeys within the
kernel I found a couple of minor bugs.
Because these patches clean up the return codes from system calls I'm sending
this out RFC hoping that users will speak up if anything breaks.
I'm not too concerned about pkey_free() because it is unlikely that anyone is
checking the return code. Interestingly enough, glibc recommends not calling
pkey_free() because it does not change the access rights to the key and may be
subsequently allocated again.[1][2]
The pkey_alloc() is more concerning. However, I checked the Chrome source and
it does not differentiate among the return codes and maps all errors into
kNoMemoryProtectionKey.
glibc says it returns ENOSYS if the system does not support pkeys but I don't
see where ENOSYS is returned? AFAICS it just returns what the kernel returns.
So it is probably up to user of glibc.
In addition I've enhanced the pkey tests to verify and test the changes.
Thanks to Rick Edgecombe and Sohil Mehta for internal review.
[1] Quote from manual/memory.texi:
Calling this function does not change the access rights 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.
[2] PKS had a similar issue and went to statically allocated keys instead.
Ira Weiny (6):
testing/pkeys: Add command line options
testing/pkeys: Don't use uninitialized variable
testing/pkeys: Add additional test for pkey_alloc()
pkeys: Lift pkey hardware check for pkey_alloc()
pkeys: Up level pkey_free() checks
pkeys: Change mm_pkey_free() to void
arch/powerpc/include/asm/pkeys.h | 18 ++---
arch/x86/include/asm/pkeys.h | 7 +-
include/linux/pkeys.h | 5 +-
mm/mprotect.c | 13 +++-
tools/testing/selftests/vm/pkey-helpers.h | 7 +-
tools/testing/selftests/vm/protection_keys.c | 75 +++++++++++++++++---
6 files changed, 86 insertions(+), 39 deletions(-)
base-commit: 874c8ca1e60b2c564a48f7e7acc40d328d5c8733
--
2.35.1
From: Ira Weiny <redacted>
It is more convenient to use command line options for debug and
iterations vs changing the code and recompiling.
Add command line options for debug level and number of iterations.
$ ./protection_keys_64 -h
Usage: ./protection_keys_64 [-h,-d,-i <iter>]
--help,-h This help
--debug,-d Increase debug level for each -d
--iterations,-i <iter> repeate test <iter> times
default: 22
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
tools/testing/selftests/vm/pkey-helpers.h | 7 +--
tools/testing/selftests/vm/protection_keys.c | 59 +++++++++++++++++---
2 files changed, 55 insertions(+), 11 deletions(-)
@@ -480,7 +484,7 @@ int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot,dprintf2("SYS_mprotect_key sret: %d\n",sret);dprintf2("SYS_mprotect_key prot: 0x%lx\n",orig_prot);dprintf2("SYS_mprotect_key failed, errno: %d\n",errno);-if(DEBUG_LEVEL>=2)+if(debug_level>=2)perror("SYS_mprotect_pkey");}returnsret;
@@ -1116,7 +1120,7 @@ void test_kernel_write_of_write_disabled_region(int *ptr, u16 pkey)pkey_write_deny(pkey);ret=read(test_fd,ptr,100);dprintf1("read ret: %d\n",ret);-if(ret<0&&(DEBUG_LEVEL>0))+if(ret<0&&(debug_level>0))perror("verbose read result (OK for this to be bad)");pkey_assert(ret);}
From: Ira Weiny <redacted>
err was being used in test_pkey_alloc_exhaust() prior to being assigned.
errno is useful to know after a failed alloc_pkey() call.
Change err to errno in the debug print.
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
tools/testing/selftests/vm/protection_keys.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Ira Weiny <redacted>
When pkeys are not available on the hardware pkey_alloc() has specific
behavior which was previously untested.
Add test for this.
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
tools/testing/selftests/vm/protection_keys.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
From: Ira Weiny <redacted>
pkey_alloc() is documented to return ENOSPC when the hardware does not
support pkeys. On x86, pkey_alloc() incorrectly returns EINVAL.
This is because mm_pkey_alloc() does not check for pkey support before
returning a key. Therefore, if the keys are not exhausted pkey_alloc()
continues on to call arch_set_user_pkey_access(). Unfortunately, when
arch_set_user_pkey_access() detects the failed support it overwrites the
ENOSPC return value with EINVAL.
Ensure consistent behavior across architectures by lifting this check to
the core mm code.
Remove a couple of 'we' references in code comments as well.
Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <redacted>
Cc: Sohil Mehta <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: linux-api@vger.kernel.org
Fixes: e8c24d3a23a4 ("x86/pkeys: Allocation/free syscalls")
Signed-off-by: Ira Weiny <redacted>
---
Thanks to Sohil for pointing out that the commit message could be more
clear WRT how EINVAL is returned incorrectly.
---
arch/powerpc/include/asm/pkeys.h | 8 +++-----
mm/mprotect.c | 3 +++
2 files changed, 6 insertions(+), 5 deletions(-)
From: Ira Weiny <redacted>
Now that the pkey arch support is no longer checked in mm_pkey_free()
there is no reason to have it return int.
Change the return value to void.
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Suggested-by: Sohil Mehta <redacted>
Signed-off-by: Ira Weiny <redacted>
---
arch/powerpc/include/asm/pkeys.h | 4 +---
arch/x86/include/asm/pkeys.h | 4 +---
include/linux/pkeys.h | 5 +----
mm/mprotect.c | 6 ++++--
4 files changed, 7 insertions(+), 12 deletions(-)
From: Ira Weiny <redacted>
x86 is missing a hardware check for pkey support in pkey_free(). While
the net result is the same (-EINVAL returned), pkey_free() has well
defined behavior which will be easier to maintain in one place.
For powerpc the return code is -1 rather than -EINVAL. This changes
that behavior slightly but this is very unlikely to break any user
space.
Lift the checks for pkey_free() to the core mm code and ensure
consistency with returning -EINVAL.
Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-api@vger.kernel.org
Cc: Sohil Mehta <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
Thanks to Sohil for suggesting I mention the powerpc return value in the
commit message.
Also Sohil suggested changing mm_pkey_free() from int to void. This is
added as a separate patch with his suggested by.
---
arch/powerpc/include/asm/pkeys.h | 6 ------
arch/x86/include/asm/pkeys.h | 3 ---
mm/mprotect.c | 8 ++++++--
3 files changed, 6 insertions(+), 11 deletions(-)
Le 11/06/2022 à 01:35, ira.weiny@intel.com a écrit :
quoted hunk
From: Ira Weiny <redacted>
x86 is missing a hardware check for pkey support in pkey_free(). While
the net result is the same (-EINVAL returned), pkey_free() has well
defined behavior which will be easier to maintain in one place.
For powerpc the return code is -1 rather than -EINVAL. This changes
that behavior slightly but this is very unlikely to break any user
space.
Lift the checks for pkey_free() to the core mm code and ensure
consistency with returning -EINVAL.
Cc: ahaas@chromium.org
Cc: clemensb@chromium.org
Cc: gdeepti@chromium.org
Cc: jkummerow@chromium.org
Cc: manoskouk@chromium.org
Cc: thibaudm@chromium.org
Cc: Florian Weimer <redacted>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-api@vger.kernel.org
Cc: Sohil Mehta <redacted>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Aneesh Kumar K.V <redacted>
Signed-off-by: Ira Weiny <redacted>
---
Thanks to Sohil for suggesting I mention the powerpc return value in the
commit message.
Also Sohil suggested changing mm_pkey_free() from int to void. This is
added as a separate patch with his suggested by.
---
arch/powerpc/include/asm/pkeys.h | 6 ------
arch/x86/include/asm/pkeys.h | 3 ---
mm/mprotect.c | 8 ++++++--
3 files changed, 6 insertions(+), 11 deletions(-)
Make it explicit, do 'return -EINVAL'
Once that is done, is there any point in having a fallback version of
mm_pkey_free() which returns -EINVAL ?
mmap_write_lock(current->mm);
- ret = mm_pkey_free(current->mm, pkey);
+ if (mm_pkey_is_allocated(current->mm, pkey))
+ ret = mm_pkey_free(current->mm, pkey);
glibc says it returns ENOSYS if the system does not support pkeys but I don't
see where ENOSYS is returned? AFAICS it just returns what the kernel returns.
So it is probably up to user of glibc.
Implementation of the pkeys system calls is arch specific and
conditional. See kernel/sys_ni.c
glibc is probably talking about ENOSYS being returned when the
architecture doesn't have support or the CONFIG option is disabled on
supported architectures.
Thanks,
Sohil
Add command line options for debug level and number of iterations.
$ ./protection_keys_64 -h
Usage: ./protection_keys_64 [-h,-d,-i <iter>]
--help,-h This help
--debug,-d Increase debug level for each -d
Is this mechanism (of counting d's) commonplace in other selftests as
well? Looking at the test code for pkeys the debug levels run from 1-5.
That feels like quite a few d's to input :)
Would it be easier to input the number in the command line directly?
Either way it would be useful to know the debug range in the help.
Maybe something like:
--debug,-d Increase debug level for each -d (1-5)
The patch seems fine to me otherwise.
--iterations,-i <iter> repeate test <iter> times
default: 22
What is errno referring to over here? There are a few things happening
in alloc_pkey(). I guess it would show the latest error that happened.
Does errno need to be set to 0 before the call?
Also, would it be useful to print the return value (new_pkey) from
alloc_pkey() here?
From: Ira Weiny <hidden> Date: 2022-06-13 23:42:14
On Mon, Jun 13, 2022 at 03:31:02PM -0700, Mehta, Sohil wrote:
On 6/10/2022 4:35 PM, ira.weiny@intel.com wrote:
quoted
Add command line options for debug level and number of iterations.
$ ./protection_keys_64 -h
Usage: ./protection_keys_64 [-h,-d,-i <iter>]
--help,-h This help
--debug,-d Increase debug level for each -d
Is this mechanism (of counting d's) commonplace in other selftests as well?
Looking at the test code for pkeys the debug levels run from 1-5. That feels
like quite a few d's to input :)
I've seen (and used) it before yes. See ibnetdiscover.
...
# Debugging flags
-d raise the IB debugging level. May be used several times (-ddd or -d -d -d).
...
-v increase the application verbosity level. May be used several times (-vv or -v -v -v)
...
- https://linux.die.net/man/8/ibnetdiscover
But a much more mainstream example I can think of is verbosity level with
lspci.
16:29:12 > lspci -h
...
Display options:
-v Be verbose (-vv or -vvv for higher verbosity)
...
Would it be easier to input the number in the command line directly?
Either way it would be useful to know the debug range in the help.
Maybe something like:
--debug,-d Increase debug level for each -d (1-5)
I'm inclined not to do this because it would encode the max debug level. On
the other hand I'm not sure why there are 5 levels now. ;-)
Having the multiple options specified was an easy way to maintain the large
number of levels.
Ira
The patch seems fine to me otherwise.
quoted
--iterations,-i <iter> repeate test <iter> times
default: 22
What is errno referring to over here? There are a few things happening in
alloc_pkey().
Good point, but the only system call in alloc_pkey() is pkey_alloc() so it will
be the errno from there.
In test_pkey_alloc_exhaust() we are expecting the errno to be from pkey_alloc()
...
if ((new_pkey == -1) && (errno == ENOSPC)) {
...
I guess it would show the latest error that happened. Does
errno need to be set to 0 before the call?
Maybe. Now that I look again errno is printed just below at level 2.
dprintf2("%s() errno: %d ENOSPC: %d\n", __func__, errno, ENOSPC);
I missed that.
Also, would it be useful to print the return value (new_pkey) from
alloc_pkey() here?
Yea that might be useful. Perhaps change err to new_pkey instead since errno
is already printed.
Ira
This assert fails on a kernel with
CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS disabled.
Since pkey_alloc() is an architecture dependent syscall, ENOSYS is
returned instead of ENOSPC when support is disabled at compile time.
See kernel/sys_ni.c
This brings us to an interesting question.
Should we have different return error codes when compile support is
disabled vs when runtime support is missing?
Here is the current behavior for pkey_alloc():
No compile time support -> return ENOSYS
No runtime support (but compile time support present) -> return ENOSPC
I would think applications would prefer the same error code. But, I am
not sure if we can achieve this now due to ABI reasons.
From: Dave Hansen <hidden> Date: 2022-06-16 20:24:31
On 6/16/22 12:25, Sohil Mehta wrote:
Should we have different return error codes when compile support is
disabled vs when runtime support is missing?
It doesn't *really* matter. Programs have to be able to run on old
kernels which will return ENOSYS. So, _when_ new kernels return ENOSYS
or ENOSPC is pretty immaterial.