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

From: Sandipan Das <sandipan@linux.ibm.com>
To: Michael Ellerman <mpe@ellerman.id.au>
Cc: linux-kselftest@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
	linux-mm@kvack.org, linuxram@us.ibm.com,
	aneesh.kumar@linux.ibm.com, bauerman@linux.ibm.com,
	fweimer@redhat.com, ruscur@russell.cc
Subject: Re: [PATCH] selftests: powerpc: Add test for execute-disabled pkeys
Date: Tue, 26 May 2020 19:33:48 +0530	[thread overview]
Message-ID: <6b73bf3f-0d10-6e8c-acd9-27de53573dec@linux.ibm.com> (raw)
In-Reply-To: <87367mg9h4.fsf@mpe.ellerman.id.au>

Hi Michael,

On 26/05/20 6:05 pm, Michael Ellerman wrote:
> [...]
>> +
>> +/* Override definitions as they might be inconsistent */
>> +#undef PKEY_DISABLE_ACCESS
>> +#define PKEY_DISABLE_ACCESS	0x3
> 
> Why would they be inconsistent?
> 

The definition in sys/mman.h still uses the value specific to
Intel's implementation i.e. 1, when this should have been 3
for powerpc. I have seen this on Ubuntu 18.04 and 20.04.

> 
>> +/* Older distros might not define this */
>> +#ifndef SEGV_PKUERR
>> +#define SEGV_PKUERR	4
>> +#endif
> ...
>> +
>> +	/* Restore permissions in order to continue */
>> +	switch (fcode) {
>> +	case SEGV_ACCERR:
>> +		if (mprotect(insns, pgsize, PROT_READ | PROT_WRITE)) {
>> +			perror("mprotect");
>> +			goto fail;
>> +		}
>> +		break;
>> +	case SEGV_PKUERR:
>> +		if (sinfo->si_pkey != fpkey)
>> +			goto fail;
> 
> This doesn't compile on older distros, eg Ubuntu 16.04:
> 
>   pkey_exec_prot.c: In function 'segv_handler':
>   pkey_exec_prot.c:121:12: error: 'siginfo_t {aka struct <anonymous>}' has no member named 'si_pkey'
>      if (sinfo->si_pkey != fpkey)
>               ^
>   pkey_exec_prot.c:151:24: error: 'siginfo_t {aka struct <anonymous>}' has no member named 'si_pkey'
>      pkey_set_rights(sinfo->si_pkey, 0);
>                           ^
>   ../../lib.mk:142: recipe for target '/output/kselftest/powerpc/mm/pkey_exec_prot' failed
> 

Thanks for reporting this.

> 
> I think a reasonable solution is to use the absence of SEGV_PKUERR to
> basically turn the whole test into a nop at build time, eg:
> 
> diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> index b346ad205e68..218257b89fbb 100644
> --- a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> +++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> @@ -30,9 +30,7 @@
>  #define PKEY_DISABLE_EXECUTE   0x4
> 
>  /* Older distros might not define this */
> -#ifndef SEGV_PKUERR
> -#define SEGV_PKUERR    4
> -#endif
> +#ifdef SEGV_PKUERR
> 
>  #define SYS_pkey_mprotect      386
>  #define SYS_pkey_alloc         384
> @@ -319,6 +317,13 @@ static int test(void)
> 
>         return 0;
>  }
> +#else
> +static int test(void)
> +{
> +       printf("Test built with old libc lacking pkey support.\n");
> +       SKIP_IF(true);
> +}
> +#endif /* SEGV_PKUERR */
> 
>  int main(void)
>  {
> 
> 

Or can I use this from the pkey tests under selftests/vm?

static inline u32 *siginfo_get_pkey_ptr(siginfo_t *si)
{
#ifdef si_pkey
	return &si->si_pkey;
#else
	return (u32 *)(((u8 *)si) + si_pkey_offset);
#endif
}

Where si_pkey_offset is 0x20 for powerpc.

- Sandipan