Re: [PATCH] selftests/powerpc: New TM signal self test

[Michael Neuling <mikey@neuling.org>]

On Wed, 2018-11-28 at 11:23 -0200, Breno Leitao wrote:
> A new self test that forces MSR[TS] to be set without calling any TM
> instruction. This test also tries to cause a page fault at a signal
> handler, exactly between MSR[TS] set and tm_recheckpoint(), forcing
> thread->texasr to be rewritten with TEXASR[FS] = 0, which will cause a BUG
> when tm_recheckpoint() is called.
> 
> This test is not deterministic since it is hard to guarantee that the page
> access will cause a page fault. Tests have shown that the bug could be
> exposed with few interactions in a buggy kernel. This test is configured to
> loop 5000x, having a good chance to hit the kernel issue in just one run.
> This self test takes less than two seconds to run.

You could try using sigaltstack() to put the ucontext somewhere else. Then you
could play tricks with that memory to try to force a fault.
madvise()+MADV_DONTNEED or fadvise()+POSIX_FADV_DONTNEED might do the trick.

This is more extra credit to make it more reliable. Not a requirement.


> This test uses set/getcontext because the kernel will recheckpoint
> zeroed structures, causing the test to segfault, which is undesired because
> the test needs to rerun, so, there is a signal handler for SIGSEGV which
> will restart the test.

Please put this description at the top of the test also.

Other than that, it looks good.

Mikey

> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  tools/testing/selftests/powerpc/tm/.gitignore |   1 +
>  tools/testing/selftests/powerpc/tm/Makefile   |   3 +-
>  .../powerpc/tm/tm-signal-force-msr.c          | 115 ++++++++++++++++++
>  3 files changed, 118 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
> 
> diff --git a/tools/testing/selftests/powerpc/tm/.gitignore
> b/tools/testing/selftests/powerpc/tm/.gitignore
> index c3ee8393dae8..89679822ebc9 100644
> --- a/tools/testing/selftests/powerpc/tm/.gitignore
> +++ b/tools/testing/selftests/powerpc/tm/.gitignore
> @@ -11,6 +11,7 @@ tm-signal-context-chk-fpu
>  tm-signal-context-chk-gpr
>  tm-signal-context-chk-vmx
>  tm-signal-context-chk-vsx
> +tm-signal-force-msr
>  tm-vmx-unavail
>  tm-unavailable
>  tm-trap
> diff --git a/tools/testing/selftests/powerpc/tm/Makefile
> b/tools/testing/selftests/powerpc/tm/Makefile
> index 9fc2cf6fbc92..58a2ebd13958 100644
> --- a/tools/testing/selftests/powerpc/tm/Makefile
> +++ b/tools/testing/selftests/powerpc/tm/Makefile
> @@ -4,7 +4,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr tm-
> signal-context-chk-fpu
>  
>  TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv tm-signal-
> stack \
>  	tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable tm-trap 
> \
> -	$(SIGNAL_CONTEXT_CHK_TESTS) tm-sigreturn
> +	$(SIGNAL_CONTEXT_CHK_TESTS) tm-sigreturn tm-signal-force-msr
>  
>  top_srcdir = ../../../../..
>  include ../../lib.mk
> @@ -20,6 +20,7 @@ $(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
>  $(OUTPUT)/tm-resched-dscr: ../pmu/lib.c
>  $(OUTPUT)/tm-unavailable: CFLAGS += -O0 -pthread -m64 -Wno-
> error=uninitialized -mvsx
>  $(OUTPUT)/tm-trap: CFLAGS += -O0 -pthread -m64
> +$(OUTPUT)/tm-signal-force-msr: CFLAGS += -pthread
>  
>  SIGNAL_CONTEXT_CHK_TESTS := $(patsubst
> %,$(OUTPUT)/%,$(SIGNAL_CONTEXT_CHK_TESTS))
>  $(SIGNAL_CONTEXT_CHK_TESTS): tm-signal.S
> diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
> b/tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
> new file mode 100644
> index 000000000000..4441d61c2328
> --- /dev/null
> +++ b/tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018, Breno Leitao, Gustavo Romero, IBM Corp.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <ucontext.h>
> +#include <unistd.h>
> +
> +#include "tm.h"
> +#include "utils.h"
> +
> +#define __MASK(X)       (1UL<<(X))
> +#define MSR_TS_S_LG     33                  /* Trans Mem state: Suspended */
> +#define MSR_TM          __MASK(MSR_TM_LG)   /* Transactional Mem Available */
> +#define MSR_TS_S        __MASK(MSR_TS_S_LG) /* Transaction Suspended */

Surely we have these defined somewhere else in selftests? 

> +
> +#define COUNT_MAX       5000                /* Number of interactions */
> +
> +/* Setting contexts because the test will crash and we want to recover */
> +ucontext_t init_context, main_context;
> +
> +static int count, first_time;
> +
> +void trap_signal_handler(int signo, siginfo_t *si, void *uc)
> +{
> +	ucontext_t *ucp = uc;
> +
> +	/*
> +	 * Allocating memory in a signal handler, and never freeing it on
> +	 * purpose, forcing the heap increase, so, the memory leak is what
> +	 * we want here.
> +	 */
> +	ucp->uc_link = malloc(sizeof(ucontext_t));
> +	memcpy(&ucp->uc_link, &ucp->uc_mcontext, sizeof(ucp->uc_mcontext));
> +
> +	/* Forcing to enable MSR[TM] */
> +	ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
> +
> +	/*
> +	 * A fork inside a signal handler seems to be more efficient than a
> +	 * fork() prior to the signal being raised.
> +	 */
> +	if (fork() == 0) {
> +		/*
> +		 * Both child and parent will return, but, child returns
> +		 * with count set so it will exit in the next segfault.
> +		 * Parent will continue to loop.
> +		 */
> +		count = COUNT_MAX;
> +	}
> +
> +	/*
> +	 * If the change above does not hit the bug, it will cause a
> +	 * segmentation fault, since the ck structures are NULL.
> +	 */
> +}
> +
> +void seg_signal_handler(int signo, siginfo_t *si, void *uc)
> +{
> +	if (count == COUNT_MAX) {
> +		/* Return to tm_signal_force_msr() and exit */
> +		setcontext(&main_context);
> +	}
> +
> +	count++;
> +	/* Reexecute the test */
> +	setcontext(&init_context);
> +}
> +
> +void tm_trap_test(void)
> +{
> +	struct sigaction trap_sa, seg_sa;
> +
> +	trap_sa.sa_flags = SA_SIGINFO;
> +	trap_sa.sa_sigaction = trap_signal_handler;
> +
> +	seg_sa.sa_flags = SA_SIGINFO;
> +	seg_sa.sa_sigaction = seg_signal_handler;
> +
> +	/*
> +	 * Set initial context. Will get back here from
> +	 * seg_signal_handler()
> +	 */
> +	getcontext(&init_context);
> +
> +	/* The signal handler will enable MSR_TS */
> +	sigaction(SIGUSR1, &trap_sa, NULL);
> +	/* If it does not crash, it will segfault, avoid it to retest */
> +	sigaction(SIGSEGV, &seg_sa, NULL);
> +
> +	raise(SIGUSR1);
> +}
> +
> +int tm_signal_force_msr(void)
> +{
> +	SKIP_IF(!have_htm());
> +
> +	/* Will get back here after COUNT_MAX interactions */
> +	getcontext(&main_context);
> +
> +	if (!first_time++)
> +		tm_trap_test();
> +
> +	return EXIT_SUCCESS;
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	test_harness(tm_signal_force_msr, "tm_signal_force_msr");
> +}