On 10/23/24 08:05, Kevin Brodsky wrote: ...> diff --git a/tools/testing/selftests/mm/pkey-x86.h b/tools/testing/selftests/mm/pkey-x86.h
index 5f28e26a2511..53ed9a336ffe 100644 --- a/tools/testing/selftests/mm/pkey-x86.h +++ b/tools/testing/selftests/mm/pkey-x86.h @@ -34,6 +34,8 @@ #define PAGE_SIZE 4096 #define MB (1<<20) +#define PKEY_ALLOW_NONE 0x55555555
Hi Kevin,
Looking at this in context, I think "PKEY_ALLOW_NONE" is not a great name. On one hand, we have:
PKEY_DISABLE_ACCESS PKEY_DISABLE_WRITE
which are values for *A* pkey.
But PKEY_ALLOW_NONE is a whole register value and spans permissions for many keys. We don't want folks trying to do something like:
pkey_alloc(flags, PKEY_ALLOW_NONE);
If I were naming it in x86 code, I'd probably call it:
PKRU_ALLOW_NONE
or something.
static inline void __page_o_noops(void) { /* 8-bytes of instruction * 512 bytes = 1 page */ diff --git a/tools/testing/selftests/mm/pkey_sighandler_tests.c b/tools/testing/selftests/mm/pkey_sighandler_tests.c index a8088b645ad6..b5e1767ee5d9 100644 --- a/tools/testing/selftests/mm/pkey_sighandler_tests.c +++ b/tools/testing/selftests/mm/pkey_sighandler_tests.c @@ -37,6 +37,8 @@ pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; pthread_cond_t cond = PTHREAD_COND_INITIALIZER; siginfo_t siginfo = {0}; +static u64 pkey_reg_no_access;
Ideally, this would be a real const or a #define because it really is static. Right? Or is there something dynamic about the ARM implementation's value?
...
* Setup alternate signal stack, which should be pkey_mprotect()ed by@@ -142,7 +145,8 @@ static void *thread_segv_maperr_ptr(void *ptr) syscall_raw(SYS_sigaltstack, (long)stack, 0, 0, 0, 0, 0); /* Disable MPK 0. Only MPK 1 is enabled. */
- __write_pkey_reg(0x55555551);
- pkey_reg = set_pkey_bits(pkey_reg_no_access, 1, 0);
- __write_pkey_reg(pkey_reg);
The existing magic numbers are not great, but could we do:
#define PKEY_ALLOW_ALL 0x0
So that this can be written like this:
pkey_reg = PKRU_ALLOW_NONE; pkey_reg = set_pkey_bits(pkey_reg, 1, PKEY_ALLOW_ALL);
That would get rid of the magic '0'.
/* Segfault */ *bad = 1; @@ -240,6 +244,7 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void) int pkey; int parent_pid = 0; int child_pid = 0;
- u64 pkey_reg;
sa.sa_flags = SA_SIGINFO | SA_ONSTACK; @@ -257,7 +262,9 @@ static void test_sigsegv_handler_with_different_pkey_for_stack(void) assert(stack != MAP_FAILED); /* Allow access to MPK 0 and MPK 1 */
- __write_pkey_reg(0x55555550);
- pkey_reg = set_pkey_bits(pkey_reg_no_access, 0, 0);
- pkey_reg = set_pkey_bits(pkey_reg, 1, 0);
- __write_pkey_reg(pkey_reg);
... and using the pattern from above, this is quite a bit more readable:
pkey_reg = PKRU_ALLOW_NONE; pkey_reg = set_pkey_bits(pkey_reg, 0, PKEY_ALLOW_ALL); pkey_reg = set_pkey_bits(pkey_reg, 1, PKEY_ALLOW_ALL);
...
- /* Only allow X for MPK 0 and nothing for other keys */
- pkey_reg_no_access = set_pkey_bits(PKEY_ALLOW_NONE, 0,
PKEY_DISABLE_ACCESS);
If the comment says "only allow X", then I'd expect the code to say:
pkey_reg_no_access = set_pkey_bits(PKEY_ALLOW_NONE, 0, PKEY_X);
... or something similar.