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

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 23/10/2024 18:51, Dave Hansen wrote:
> 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.

I agree, the naming is not ideal, I lacked inspiration! Maybe
PKEY_REG_ALLOW_NONE to remain generic?

>
>>  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?

It isn't dynamic no, the issue is that on architectures where pkeys
restrict execution we need to allow X for pkey 0. Of course it would be
possible to define PKEY_REG_ALLOW_ALL in such a way that X is allowed
for pkey 0, but I was concerned this might be misleading. No strong
opinion either way, happy to make it purely a macro, maybe with a better
name?

> ...
>>  	 * 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'.

Definitely better yes. But how about using Yury's uapi addition,
PKEY_UNRESTRICTED [1]?

[1]
https://lore.kernel.org/all/20241022120128.359652-1-yury.khrustalev@arm.com/

>
>>  	/* 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.

I could #define PKEY_X PKEY_DISABLE_ACCESS but is the mixture of
negative and positive polarity really helping? We cannot define PKEY_R
and PKEY_W so that (for instance) PKEY_R | PKEY_X does what it says.
Having to use PKEY_DISABLE_ACCESS to mean "X only" is not ideal, but
this is what userspace already has to do.

Either way if we define PKEY_REG_ALLOW_NONE or similar to allow X for
pkey 0 as suggested then this will go.

Thanks for the review!

Kevin