Re: [PATCH 2/5] selftests/powerpc: Add TM signal return selftest

[Michael Ellerman <mpe@ellerman.id.au>]

On Fri, 2015-11-13 at 15:57 +1100, Michael Neuling wrote:

> Test the kernel's signal return code to ensure that it doesn't crash
> when both the transactional and suspend MSR bits are set in the signal
> context.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> ---
>  tools/testing/selftests/powerpc/tm/Makefile        |  2 +-
>  .../selftests/powerpc/tm/tm-signal-msr-resv.c      | 64 ++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> 
> diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
> index 4bea62a..0c28db7 100644
> --- a/tools/testing/selftests/powerpc/tm/Makefile
> +++ b/tools/testing/selftests/powerpc/tm/Makefile
> @@ -1,4 +1,4 @@
> -TEST_PROGS := tm-resched-dscr tm-syscall
> +TEST_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv
>  
>  all: $(TEST_PROGS)
>  
> diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> new file mode 100644
> index 0000000..14705ae
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv.c
> @@ -0,0 +1,64 @@
> +/*
> + * Copyright 2015, Michael Neuling, IBM Corp.
> + * Licensed under GPLv2.
> + *
> + * Test the kernel's signal return code to ensure that it doesn't
> + * crash when both the transactional and suspend MSR bits are set in
> + * the signal context.

That's a good start, a little blurb on how we do the test is nice too. For our
future selves.

eg. We send ourselves a SIGUSR1, and in the handler we write the illegal MSR
value, then when we return from the signal the kernel should detect that and
kill us with a SIGSEGV.

> + */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <signal.h>
> +
> +#include "utils.h"
> +
> +struct sigaction act;
> +
> +void signal_segv(int signum, siginfo_t *info, void *uc)
> +{
> +	printf("PASSED\n");
> +	exit(0);

You're not allowed to call exit() from a signal handler :D

You can call _exit(), I guess you can't just set a flag and return like you'd
normally do in a handler?

> +}
> +
> +void signal_usr1(int signum, siginfo_t *info, void *uc)
> +{
> +	ucontext_t *ucp = uc;
> +
> +	/* link tm checkpointed context to normal context */
> +	ucp->uc_link = ucp;
> +	/* set all TM bits */
> +#ifdef __powerpc64__
> +	ucp->uc_mcontext.gp_regs[PT_MSR] |= (7ULL << 32);
> +#else
> +	ucp->uc_mcontext.regs->gpr[PT_MSR] |= (7ULL);
> +#endif
> +	/* Should segv on return becuase of invalid context */

On return of what? This handler or sigaction() below ?

> +	act.sa_sigaction = signal_segv;
> +	if (sigaction(SIGSEGV, &act, NULL) < 0) {
> +		perror("sigaction sigsegv");
> +		exit(1);
> +	}

I'm not clear why we're setting up the handler for SEGV in the handler for
USR1. Is that required to trip the bug? (not that I understood).

> +}
> +
> +int tm_signal_msr_resv()
> +{
> +
> +	act.sa_sigaction = signal_usr1;
> +	sigemptyset(&act.sa_mask);
> +	act.sa_flags = SA_SIGINFO;
> +	if (sigaction(SIGUSR1, &act, NULL) < 0) {
> +		perror("sigaction sigusr1");
> +		exit(1);
> +	}
> +
> +	raise(SIGUSR1);
> +
> +	printf("FAILED\n");

The harness will print a failure message for you, so this is not really
helpful. What is helpful is a message that says *why* it failed. eg: "Expected
to take SEGV but didn't?" or something like that.

> +	return 1;
> +}
> +
> +int main(void)
> +{
> +	return test_harness(tm_signal_msr_resv, "tm_signal_msr_resv");
> +}

cheers