[PATCH v2] selftests: powerpc: Add test for execute-disabled pkeys

[Sandipan Das]

Hi Michael,

Thanks for your suggestions. I had a few questions regarding some
of them.

On 29/05/20 7:18 am, Michael Ellerman wrote:
>> [...]
>> +
>> +static void pkeyreg_set(unsigned long uamr)
>> +{
>> +	asm volatile("isync; mtspr	0xd, %0; isync;" : : "r"(uamr));
>> +}
> 
> You can use mtspr() there, but you'll need to add the isync's yourself.
> 

Would it make sense to add a new macro that adds the CSI instructions?
Something like this.

diff --git a/tools/testing/selftests/powerpc/include/reg.h b/tools/testing/selftests/powerpc/include/reg.h
index 022c5076b2c5..d60f66380cad 100644
--- a/tools/testing/selftests/powerpc/include/reg.h
+++ b/tools/testing/selftests/powerpc/include/reg.h
@@ -15,6 +15,10 @@
 #define mtspr(rn, v)   asm volatile("mtspr " _str(rn) ",%0" : \
                                    : "r" ((unsigned long)(v)) \
                                    : "memory")
+#define mtspr_csi(rn, v)       ({ \
+                       asm volatile("isync; mtspr " _str(rn) ",%0; isync;" : \
+                                   : "r" ((unsigned long)(v)) \
+                                   : "memory"); })
 
 #define mb()           asm volatile("sync" : : : "memory");
 #define barrier()      asm volatile("" : : : "memory");


I also noticed that two of the ptrace-related pkey tests were also not
using CSIs. I will fix those too.

>> [...]
>> +	/* The following two cases will avoid SEGV_PKUERR */
>> +	ftype = -1;
>> +	fpkey = -1;
>> +
>> +	/*
>> +	 * Read an instruction word from the address when AMR bits
>> +	 * are not set.
> 
> You should explain for people who aren't familiar with the ISA that "AMR
> bits not set" means "read/write access allowed".
> 
>> +	 *
>> +	 * This should not generate a fault as having PROT_EXEC
>> +	 * implicitly allows reads. The pkey currently restricts
> 
> Whether PROT_EXEC implies read is not well defined (see the man page).
> If you want to test this case I think you'd be better off specifying
> PROT_EXEC | PROT_READ explicitly.
> 

But I guess specifying PROT_EXEC | PROT_READ defeats the purpose? I can
tweak the passing condition though based on whether READ_IMPLIES_EXEC is
set in the personality.

> [...]
>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
>> +
>> +	/* The following three cases will generate SEGV_PKUERR */
>> +	ftype = PKEY_DISABLE_ACCESS;
>> +	fpkey = pkey;
>> +
>> +	/*
>> +	 * Read an instruction word from the address when AMR bits
>> +	 * are set.
>> +	 *
>> +	 * This should generate a pkey fault based on AMR bits only
>> +	 * as having PROT_EXEC implicitly allows reads.
> 
> Again would be better to specify PROT_READ IMHO.
> 

I can use a personality check here too.

>> +	 */
>> +	faults = 1;
>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>> +	printf("read from %p, pkey is execute-disabled, access-disabled\n",
>> +	       (void *) faddr);
>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>> +	i = *faddr;
>> +	FAIL_IF(faults != 0 || fcode != SEGV_PKUERR);
>> +
>> +	/*
>> +	 * Write an instruction word to the address when AMR bits
>> +	 * are set.
>> +	 *
>> +	 * This should generate two faults. First, a pkey fault based
>> +	 * on AMR bits and then an access fault based on PROT_EXEC.
>> +	 */
>> +	faults = 2;
> 
> Setting faults to the expected value and decrementing it in the fault
> handler is kind of weird.
> 
> I think it would be clearer if faults was just a zero-based counter of
> how many faults we've taken, and then you test that it's == 2 below.
> 
>> +	FAIL_IF(sys_pkey_mprotect(insns, pgsize, PROT_EXEC, pkey) != 0);
>> +	printf("write to %p, pkey is execute-disabled, access-disabled\n",
>> +	       (void *) faddr);
>> +	pkey_set_rights(pkey, PKEY_DISABLE_ACCESS);
>> +	*faddr = 0x60000000;	/* nop */
>> +	FAIL_IF(faults != 0 || fcode != SEGV_ACCERR);
> 
> ie. FAIL_IF(faults != 2 || ... )
> 

Agreed, it is weird. IIRC, I did this to make sure that if the test
kept getting repeated faults at the same address and exceeded the
maximum number of expected faults i.e. it gets another fault when
'faults' is already zero, then the signal handler will attempt to
let the program continue by giving all permissions to the page and
also the pkey. Would it make sense to just rename 'faults' to
something like 'remaining_faults'?


- Sandipan