Re: [PATCH v2 4/5] selftests/mm: Use generic pkey register manipulation

[Dave Hansen <dave.hansen@intel.com>]

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.