Received-SPF: pass (google.com: domain of linux-kernel+bounces-171238-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Thomas Gleixner <tglx@linutronix.de>
To: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>,
 linux-kernel@vger.kernel.org
Cc: x86@kernel.org, dave.hansen@linux.intel.com, mingo@kernel.org,
 keith.lucas@oracle.com, aruna.ramakrishna@oracle.com
Subject: Re: [PATCH v3 4/4] selftests/mm: Add new testcases for pkeys
In-Reply-To: <20240425180542.1042933-5-aruna.ramakrishna@oracle.com>
References: <20240425180542.1042933-1-aruna.ramakrishna@oracle.com>
 <20240425180542.1042933-5-aruna.ramakrishna@oracle.com>
Date: Tue, 07 May 2024 14:05:17 +0200
Message-ID: <875xvprfnm.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Thu, Apr 25 2024 at 18:05, Aruna Ramakrishna wrote:
> This commit adds a few new tests to exercise the signal handler flow,

"This commit" is equally useless as "This patch". See
Documentation/process/ and grep for "This patch".

> especially with pkey 0 disabled.
>
> Signed-off-by: Keith Lucas <keith.lucas@oracle.com>
> Signed-off-by: Aruna Ramakrishna <aruna.ramakrishna@oracle.com>
> ---
>  tools/testing/selftests/mm/Makefile           |   2 +
>  tools/testing/selftests/mm/pkey-helpers.h     |  11 +-
>  .../selftests/mm/pkey_sighandler_tests.c      | 315 ++++++++++++++++++
>  tools/testing/selftests/mm/protection_keys.c  |  10 -
>  4 files changed, 327 insertions(+), 11 deletions(-)
>  create mode 100644 tools/testing/selftests/mm/pkey_sighandler_tests.c
>
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index eb5f39a2668b..2f90344ad11e 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -82,6 +82,7 @@ CAN_BUILD_X86_64 := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_64bit_pr
>  CAN_BUILD_WITH_NOPIE := $(shell ./../x86/check_cc.sh "$(CC)" ../x86/trivial_program.c -no-pie)
>  
>  VMTARGETS := protection_keys
> +VMTARGETS := pkey_sighandler_tests
>  BINARIES_32 := $(VMTARGETS:%=%_32)
>  BINARIES_64 := $(VMTARGETS:%=%_64)
>  
> @@ -100,6 +101,7 @@ else
>  
>  ifneq (,$(findstring $(ARCH),ppc64))
>  TEST_GEN_FILES += protection_keys
> +TEST_GEN_FILES += pkey_sighandler_tests
>  endif
>  
>  endif
> diff --git a/tools/testing/selftests/mm/pkey-helpers.h b/tools/testing/selftests/mm/pkey-helpers.h
> index 1af3156a9db8..a0766e3d9ab6 100644
> --- a/tools/testing/selftests/mm/pkey-helpers.h
> +++ b/tools/testing/selftests/mm/pkey-helpers.h
> @@ -79,7 +79,16 @@ extern void abort_hooks(void);
>  	}					\
>  } while (0)
>  
> -__attribute__((noinline)) int read_ptr(int *ptr);
> +#define barrier() __asm__ __volatile__("": : :"memory")

#include <linux/compiler.h>

> +__attribute__((noinline)) int read_ptr(int *ptr)
> +{
> +	        /*
> +		 *          * Keep GCC from optimizing this away somehow
> +		 *                   */

That comment is completely malformatted.

> +	        barrier();
> +		        return *ptr;

White space damage.

> +
> +static inline __attribute__((always_inline)) long
> +syscall_raw(long n, long a1, long a2, long a3, long a4, long a5, long a6)
> +{

What for? syscall(2) provides exactly what you want, no?

> +	unsigned long ret;
> +	register long r10 asm("r10") = a4;
> +	register long r8 asm("r8") = a5;
> +	register long r9 asm("r9") = a6;
> +	asm volatile ("syscall"
> +		      : "=a"(ret)
> +		      : "a"(n), "D"(a1), "S"(a2), "d"(a3), "r"(r10), "r"(r8), "r"(r9)
> +		      : "rcx", "r11", "memory");

Aside of that this breaks on 32-bit builds.

> +	return ret;
> +}
> +

> +static void *thread_segv_with_pkey0_disabled(void *ptr)
> +{
> +	/* Disable MPK 0 (and all others too) */
> +	__write_pkey_reg(0x55555555);
> +
> +        /* Segfault (with SEGV_MAPERR) */

Please use tabs for indentation. (All over the place)

> +	*(int *) (0x1) = 1;
> +	return NULL;
> +}
> +
> +/*
> + * Verify that the sigsegv handler is invoked when pkey 0 is disabled.
> + * Note that the new thread stack and the alternate signal stack is
> + * protected by MPK 0.
> + */
> +static void test_sigsegv_handler_with_pkey0_disabled(void)
> +{
> +	struct sigaction sa;
> +
> +	sa.sa_flags = SA_SIGINFO;
> +
> +	sa.sa_sigaction = sigsegv_handler;
> +	sigemptyset(&sa.sa_mask);
> +	if (sigaction(SIGSEGV, &sa, NULL) == -1) {
> +		perror("sigaction");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	memset(&siginfo, 0, sizeof(siginfo));
> +
> +	pthread_t thr;
> +	pthread_attr_t attr;
> +	pthread_attr_init(&attr);
> +	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
> +
> +	pthread_create(&thr, &attr, thread_segv_with_pkey0_disabled, NULL);
> +
> +	pthread_mutex_lock(&mutex);
> +	while(siginfo.si_signo == 0)
> +		pthread_cond_wait(&cond, &mutex);
> +	pthread_mutex_unlock(&mutex);
> +
> +	assert(siginfo.si_signo == SIGSEGV);
> +	assert(siginfo.si_code == SEGV_MAPERR);
> +	assert(siginfo.si_addr == (void *)1);

This should not use assert(). This wants to use proper kselftest result
and exit mechanisms all over the place.

> +	printf("%s passed!\n", __func__);

Ditto for printf().

> +}
> +/*
> + * Verify that the sigsegv handler that uses an alternate signal stack
> + * is correctly invoked for a thread which uses a non-zero MPK to protect
> + * its own stack, and disables all other MPKs (including 0).
> + */
> +static void test_sigsegv_handler_with_different_pkey_for_stack(void)
> +{
> +	struct sigaction sa;
> +	static stack_t sigstack;
> +        void *stack;
> +	int pkey;
> +	int parentPid = 0;
> +	int childPid = 0;

No camel case please

> +int main(int argc, char *argv[])
> +{
> +	size_t i;

size_t? What's wrong with int?

> +
> +	for (i = 0; i < sizeof(pkey_tests)/sizeof(pkey_tests[0]); i++) {

                        ARRAY_SIZE() ?

> +		(*pkey_tests[i])();
> +	}

Pointless brackets.

Thanks,

        tglx