| Message ID | 20241209095019.1732120-12-kevin.brodsky@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pkeys kselftests improvements | expand |
On Mon, Dec 09, 2024 at 09:50:16AM +0000, Kevin Brodsky wrote: Hi Kevin, Andrew, > diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile > index 814b17a43385..1f0743d9459d 100644 > --- a/tools/testing/selftests/mm/Makefile > +++ b/tools/testing/selftests/mm/Makefile > @@ -147,8 +147,8 @@ TEST_FILES += write_hugetlb_memory.sh > > include ../lib.mk > > -$(TEST_GEN_PROGS): vm_util.c thp_settings.c > -$(TEST_GEN_FILES): vm_util.c thp_settings.c > +$(TEST_GEN_PROGS): vm_util.c thp_settings.c pkey_util.c > +$(TEST_GEN_FILES): vm_util.c thp_settings.c pkey_util.c This patch breaks s390 in -next with the below error messages: In file included from pkey_util.c:5: pkey-helpers.h:109:2: error: #error Architecture not supported 109 | #error Architecture not supported | ^~~~~ pkey-helpers.h: In function ‘set_pkey_bits’: pkey-helpers.h:126:21: error: implicit declaration of function ‘pkey_bit_position’ [-Wimplicit-function-declaration] 126 | u32 shift = pkey_bit_position(pkey); | ^~~~~~~~~~~~~~~~~ pkey-helpers.h: In function ‘_read_pkey_reg’: pkey-helpers.h:151:24: error: implicit declaration of function ‘__read_pkey_reg’; did you mean ‘_read_pkey_reg’? [-Wimplicit-function-declaration] 151 | u64 pkey_reg = __read_pkey_reg(); | ^~~~~~~~~~~~~~~ | _read_pkey_reg pkey-helpers.h: In function ‘write_pkey_reg’: pkey-helpers.h:165:18: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 3 has type ‘int’ [-Wformat=] 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 166 | __read_pkey_reg(), pkey_reg); | ~~~~~~~~~~~~~~~~~ | | | int pkey-helpers.h:62:32: note: in definition of macro ‘dprintf_level’ 62 | sigsafe_printf(args); \ | ^~~~ pkey-helpers.h:165:9: note: in expansion of macro ‘dprintf4’ 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ^~~~~~~~ pkey-helpers.h:165:39: note: format string is defined here 165 | dprintf4("%s() changing %016llx to %016llx\n", __func__, | ~~~~~~^ | | | long long unsigned int | %016x pkey-helpers.h:169:9: error: implicit declaration of function ‘__write_pkey_reg’; did you mean ‘write_pkey_reg’? [-Wimplicit-function-declaration] 169 | __write_pkey_reg(pkey_reg); | ^~~~~~~~~~~~~~~~ | write_pkey_reg pkey-helpers.h:171:18: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘int’ [-Wformat=] 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 172 | pkey_reg, __read_pkey_reg()); | ~~~~~~~~~~~~~~~~~ | | | int pkey-helpers.h:62:32: note: in definition of macro ‘dprintf_level’ 62 | sigsafe_printf(args); \ | ^~~~ pkey-helpers.h:171:9: note: in expansion of macro ‘dprintf4’ 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ^~~~~~~~ pkey-helpers.h:171:47: note: format string is defined here 171 | dprintf4("%s(%016llx) pkey_reg: %016llx\n", __func__, | ~~~~~~^ | | | long long unsigned int | %016x pkey-helpers.h: In function ‘is_pkeys_supported’: pkey-helpers.h:207:14: error: implicit declaration of function ‘cpu_has_pkeys’; did you mean ‘kernel_has_pkeys’? [-Wimplicit-function-declaration] 207 | if (!cpu_has_pkeys()) { | ^~~~~~~~~~~~~ | kernel_has_pkeys Please, let me know if I am missing something. > $(OUTPUT)/uffd-stress: uffd-common.c > $(OUTPUT)/uffd-unit-tests: uffd-common.c Thanks!
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile index 814b17a43385..1f0743d9459d 100644 --- a/tools/testing/selftests/mm/Makefile +++ b/tools/testing/selftests/mm/Makefile @@ -147,8 +147,8 @@ TEST_FILES += write_hugetlb_memory.sh include ../lib.mk -$(TEST_GEN_PROGS): vm_util.c thp_settings.c -$(TEST_GEN_FILES): vm_util.c thp_settings.c +$(TEST_GEN_PROGS): vm_util.c thp_settings.c pkey_util.c +$(TEST_GEN_FILES): vm_util.c thp_settings.c pkey_util.c $(OUTPUT)/uffd-stress: uffd-common.c $(OUTPUT)/uffd-unit-tests: uffd-common.c diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h index 6f0ab7b42738..f080e97b39be 100644 --- a/tools/testing/selftests/mm/pkey-helpers.h +++ b/tools/testing/selftests/mm/pkey-helpers.h @@ -89,6 +89,8 @@ extern void abort_hooks(void); int sys_pkey_alloc(unsigned long flags, unsigned long init_val); int sys_pkey_free(unsigned long pkey); +int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, + unsigned long pkey); /* For functions called from protection_keys.c only */ noinline int read_ptr(int *ptr); diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index e1aaeb65cfae..c73cee192b88 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -311,8 +311,8 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void) __write_pkey_reg(pkey_reg); /* Protect the new stack with MPK 1 */ - pkey = pkey_alloc(0, 0); - pkey_mprotect(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); + pkey = sys_pkey_alloc(0, 0); + sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); /* Set up alternate signal stack that will use the default MPK */ sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, @@ -484,8 +484,8 @@ static void test_pkru_sigreturn(void) __write_pkey_reg(pkey_reg); /* Protect the stack with MPK 2 */ - pkey = pkey_alloc(0, 0); - pkey_mprotect(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); + pkey = sys_pkey_alloc(0, 0); + sys_mprotect_pkey(stack, STACK_SIZE, PROT_READ | PROT_WRITE, pkey); /* Set up alternate signal stack that will use the default MPK */ sigstack.ss_sp = mmap(0, STACK_SIZE, PROT_READ | PROT_WRITE | PROT_EXEC, diff --git a/tools/testing/selftests/mm/pkey_util.c b/tools/testing/selftests/mm/pkey_util.c new file mode 100644 index 000000000000..ca4ad0d44ab2 --- /dev/null +++ b/tools/testing/selftests/mm/pkey_util.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <sys/syscall.h> +#include <unistd.h> + +#include "pkey-helpers.h" + +int sys_pkey_alloc(unsigned long flags, unsigned long init_val) +{ + int ret = syscall(SYS_pkey_alloc, flags, init_val); + dprintf1("%s(flags=%lx, init_val=%lx) syscall ret: %d errno: %d\n", + __func__, flags, init_val, ret, errno); + return ret; +} + +int sys_pkey_free(unsigned long pkey) +{ + int ret = syscall(SYS_pkey_free, pkey); + dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret); + return ret; +} + +int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, + unsigned long pkey) +{ + int sret; + + dprintf2("%s(0x%p, %zx, prot=%lx, pkey=%lx)\n", __func__, + ptr, size, orig_prot, pkey); + + errno = 0; + sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey); + if (errno) { + 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) + perror("SYS_mprotect_pkey"); + } + return sret; +} diff --git a/tools/testing/selftests/mm/protection_keys.c b/tools/testing/selftests/mm/protection_keys.c index f43cf3b75d8e..3688571e6b39 100644 --- a/tools/testing/selftests/mm/protection_keys.c +++ b/tools/testing/selftests/mm/protection_keys.c @@ -460,34 +460,6 @@ static pid_t fork_lazy_child(void) return forkret; } -int sys_mprotect_pkey(void *ptr, size_t size, unsigned long orig_prot, - unsigned long pkey) -{ - int sret; - - dprintf2("%s(0x%p, %zx, prot=%lx, pkey=%lx)\n", __func__, - ptr, size, orig_prot, pkey); - - errno = 0; - sret = syscall(__NR_pkey_mprotect, ptr, size, orig_prot, pkey); - if (errno) { - 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) - perror("SYS_mprotect_pkey"); - } - return sret; -} - -int sys_pkey_alloc(unsigned long flags, unsigned long init_val) -{ - int ret = syscall(SYS_pkey_alloc, flags, init_val); - dprintf1("%s(flags=%lx, init_val=%lx) syscall ret: %d errno: %d\n", - __func__, flags, init_val, ret, errno); - return ret; -} - static int alloc_pkey(void) { int ret; @@ -534,13 +506,6 @@ static int alloc_pkey(void) return ret; } -int sys_pkey_free(unsigned long pkey) -{ - int ret = syscall(SYS_pkey_free, pkey); - dprintf1("%s(pkey=%ld) syscall ret: %d\n", __func__, pkey, ret); - return ret; -} - /* * I had a bug where pkey bits could be set by mprotect() but * not cleared. This ensures we get lots of random bit sets
sys_pkey_alloc, sys_pkey_free and sys_mprotect_pkey are currently used in protections_keys.c, while pkey_sighandler_tests.c calls the libc wrappers directly (e.g. pkey_mprotect()). This is probably ok when using glibc (those symbols appeared a while ago), but Musl does not currently provide them. The logging in the helpers from pkey-helpers.h can also come in handy. Make things more consistent by using the sys_pkey helpers in pkey_sighandler_tests.c too. To that end their implementation is moved to a common .c file (pkey_util.c). This also enables calling is_pkeys_supported() outside of protections_keys.c, since it relies on sys_pkey_{alloc,free}. Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com> --- tools/testing/selftests/mm/Makefile | 4 +- tools/testing/selftests/mm/pkey-helpers.h | 2 + .../selftests/mm/pkey_sighandler_tests.c | 8 ++-- tools/testing/selftests/mm/pkey_util.c | 40 +++++++++++++++++++ tools/testing/selftests/mm/protection_keys.c | 35 ---------------- 5 files changed, 48 insertions(+), 41 deletions(-) create mode 100644 tools/testing/selftests/mm/pkey_util.c