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

From: Michael Ellerman <mpe@ellerman.id.au>
To: Sandipan Das <sandipan@linux.ibm.com>
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 22:35:19 +1000	[thread overview]
Message-ID: <87367mg9h4.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <20200508162332.65316-1-sandipan@linux.ibm.com>

Hi Sandipan,

Sandipan Das <sandipan@linux.ibm.com> writes:
> diff --git a/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> new file mode 100644
> index 000000000000..b346ad205e68
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/mm/pkey_exec_prot.c
> @@ -0,0 +1,326 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright 2020, Sandipan Das, IBM Corp.
> + *
> + * Test if applying execute protection on pages using memory
> + * protection keys works as expected.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <signal.h>
> +
> +#include <time.h>
> +#include <unistd.h>
> +#include <sys/mman.h>
> +
> +#include "utils.h"
> +
> +/* Override definitions as they might be inconsistent */
> +#undef PKEY_DISABLE_ACCESS
> +#define PKEY_DISABLE_ACCESS	0x3

Why would they be inconsistent?


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


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)
 {


cheers
