[PATCH v2] powerpc/tm: Set MSR[TS] just prior to recheckpoint

[PATCH v2] powerpc/tm: Set MSR[TS] just prior to recheckpoint

[Breno Leitao]

On a signal handler return, the user could set a context with MSR[TS] bits
set, and these bits would be copied to task regs->msr.

At restore_tm_sigcontexts(), after current task regs->msr[TS] bits are set,
several __get_user() are called and then a recheckpoint is executed.

This is a problem since a page fault (in kernel space) could happen when
calling __get_user(). If it happens, the process MSR[TS] bits were
already set, but recheckpoint was not executed, and SPRs are still invalid.

The page fault can cause the current process to be de-scheduled, with
MSR[TS] active and without tm_recheckpoint() being called.  More
importantly, without TEXASR[FS] bit set also.

Since TEXASR might not have the FS bit set, and when the process is
scheduled back, it will try to reclaim, which will be aborted because of
the CPU is not in the suspended state, and, then, recheckpoint. This
recheckpoint will restore thread->texasr into TEXASR SPR, which might be
zero, hitting a BUG_ON().

	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
	cpu 0xb: Vector: 700 (Program Check) at [c00000041f1576d0]
	    pc: c000000000054550: restore_gprs+0xb0/0x180
	    lr: 0000000000000000
	    sp: c00000041f157950
	   msr: 8000000100021033
	  current = 0xc00000041f143000
	  paca    = 0xc00000000fb86300	 softe: 0	 irq_happened: 0x01
	    pid   = 1021, comm = kworker/11:1
	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
	Linux version 4.9.0-3-powerpc64le (debian-kernel@lists.debian.org) (gcc version 6.3.0 20170516 (Debian 6.3.0-18) ) #1 SMP Debian 4.9.30-2+deb9u2 (2017-06-26)
	enter ? for help
	[c00000041f157b30] c00000000001bc3c tm_recheckpoint.part.11+0x6c/0xa0
	[c00000041f157b70] c00000000001d184 __switch_to+0x1e4/0x4c0
	[c00000041f157bd0] c00000000082eeb8 __schedule+0x2f8/0x990
	[c00000041f157cb0] c00000000082f598 schedule+0x48/0xc0
	[c00000041f157ce0] c0000000000f0d28 worker_thread+0x148/0x610
	[c00000041f157d80] c0000000000f96b0 kthread+0x120/0x140
	[c00000041f157e30] c00000000000c0e0 ret_from_kernel_thread+0x5c/0x7c

This patch simply delays the MSR[TS] set, so, if there is any page fault in
the __get_user() section, it does not have regs->msr[TS] set, since the TM
structures are still invalid, thus avoiding doing TM operations for
in-kernel exceptions and possible process reschedule.

With this patch, the MSR[TS] will only be set just before recheckpointing
and setting TEXASR[FS] = 1, thus avoiding an interrupt with TM registers in
invalid state.

Other than that, if CONFIG_PREEMPT is set, there might be a preemption just
after setting MSR[TS] and before tm_recheckpoint(), thus, this block must
be atomic from a preemption perspective, thus, calling
preempt_disable/enable() on this code.

It is not possible to move tm_recheckpoint to happen earlier, because it is
required to get the checkpointed registers from userspace, with
__get_user(), thus, the only way to avoid this undesired behavior is
delaying the MSR[TS] set.

The 32-bits signal handler seems to be safe this current issue, but, it
might be exposed to the preemption issue, thus, disabling preemption in
this chunk of code.

Changes from v2:
 * Run the critical section with preempt_disable.

Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
Cc: stable@vger.kernel.org (v3.9+)
Signed-off-by: Breno Leitao <leitao@debian.org>

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index e6474a45cef5..fd59fef9931b 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -848,7 +848,23 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	/* If TM bits are set to the reserved value, it's an invalid context */
 	if (MSR_TM_RESV(msr_hi))
 		return 1;
-	/* Pull in the MSR TM bits from the user context */
+
+	/*
+	 * Disabling preemption, since it is unsafe to be preempted
+	 * with MSR[TS] set without recheckpointing.
+	 */
+	preempt_disable();
+
+	/*
+	 * CAUTION:
+	 * After regs->MSR[TS] being updated, make sure that get_user(),
+	 * put_user() or similar functions are *not* called. These
+	 * functions can generate page faults which will cause the process
+	 * to be de-scheduled with MSR[TS] set but without calling
+	 * tm_recheckpoint(). This can cause a bug.
+	 *
+	 * Pull in the MSR TM bits from the user context
+	 */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
 	/* Now, recheckpoint.  This loads up all of the checkpointed (older)
 	 * registers, including FP and V[S]Rs.  After recheckpointing, the
@@ -873,6 +889,8 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	}
 #endif
 
+	preempt_enable();
+
 	return 0;
 }
 #endif
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 83d51bf586c7..bbd1c73243d7 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -467,20 +467,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	if (MSR_TM_RESV(msr))
 		return -EINVAL;
 
-	/* pull in MSR TS bits from user context */
-	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
-
-	/*
-	 * Ensure that TM is enabled in regs->msr before we leave the signal
-	 * handler. It could be the case that (a) user disabled the TM bit
-	 * through the manipulation of the MSR bits in uc_mcontext or (b) the
-	 * TM bit was disabled because a sufficient number of context switches
-	 * happened whilst in the signal handler and load_tm overflowed,
-	 * disabling the TM bit. In either case we can end up with an illegal
-	 * TM state leading to a TM Bad Thing when we return to userspace.
-	 */
-	regs->msr |= MSR_TM;
-
 	/* pull in MSR LE from user context */
 	regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
 
@@ -572,6 +558,34 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	tm_enable();
 	/* Make sure the transaction is marked as failed */
 	tsk->thread.tm_texasr |= TEXASR_FS;
+
+	/*
+	 * Disabling preemption, since it is unsafe to be preempted
+	 * with MSR[TS] set without recheckpointing.
+	 */
+	preempt_disable();
+
+	/* pull in MSR TS bits from user context */
+	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
+
+	/*
+	 * Ensure that TM is enabled in regs->msr before we leave the signal
+	 * handler. It could be the case that (a) user disabled the TM bit
+	 * through the manipulation of the MSR bits in uc_mcontext or (b) the
+	 * TM bit was disabled because a sufficient number of context switches
+	 * happened whilst in the signal handler and load_tm overflowed,
+	 * disabling the TM bit. In either case we can end up with an illegal
+	 * TM state leading to a TM Bad Thing when we return to userspace.
+	 *
+	 * CAUTION:
+	 * After regs->MSR[TS] being updated, make sure that get_user(),
+	 * put_user() or similar functions are *not* called. These
+	 * functions can generate page faults which will cause the process
+	 * to be de-scheduled with MSR[TS] set but without calling
+	 * tm_recheckpoint(). This can cause a bug.
+	 */
+	regs->msr |= MSR_TM;
+
 	/* This loads the checkpointed FP/VEC state, if used */
 	tm_recheckpoint(&tsk->thread);
 
@@ -585,6 +599,8 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 		regs->msr |= MSR_VEC;
 	}
 
+	preempt_enable();
+
 	return err;
 }
 #endif
-- 
2.19.0

[PATCH v2] powerpc/tm: Set MSR[TS] just prior to recheckpoint

[Breno Leitao]

On a signal handler return, the user could set a context with MSR[TS] bits
set, and these bits would be copied to task regs->msr.

At restore_tm_sigcontexts(), after current task regs->msr[TS] bits are set,
several __get_user() are called and then a recheckpoint is executed.

This is a problem since a page fault (in kernel space) could happen when
calling __get_user(). If it happens, the process MSR[TS] bits were
already set, but recheckpoint was not executed, and SPRs are still invalid.

The page fault can cause the current process to be de-scheduled, with
MSR[TS] active and without tm_recheckpoint() being called.  More
importantly, without TEXASR[FS] bit set also.

Since TEXASR might not have the FS bit set, and when the process is
scheduled back, it will try to reclaim, which will be aborted because of
the CPU is not in the suspended state, and, then, recheckpoint. This
recheckpoint will restore thread->texasr into TEXASR SPR, which might be
zero, hitting a BUG_ON().

	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
	cpu 0xb: Vector: 700 (Program Check) at [c00000041f1576d0]
	    pc: c000000000054550: restore_gprs+0xb0/0x180
	    lr: 0000000000000000
	    sp: c00000041f157950
	   msr: 8000000100021033
	  current = 0xc00000041f143000
	  paca    = 0xc00000000fb86300	 softe: 0	 irq_happened: 0x01
	    pid   = 1021, comm = kworker/11:1
	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
	Linux version 4.9.0-3-powerpc64le (debian-kernel@lists.debian.org) (gcc version 6.3.0 20170516 (Debian 6.3.0-18) ) #1 SMP Debian 4.9.30-2+deb9u2 (2017-06-26)
	enter ? for help
	[c00000041f157b30] c00000000001bc3c tm_recheckpoint.part.11+0x6c/0xa0
	[c00000041f157b70] c00000000001d184 __switch_to+0x1e4/0x4c0
	[c00000041f157bd0] c00000000082eeb8 __schedule+0x2f8/0x990
	[c00000041f157cb0] c00000000082f598 schedule+0x48/0xc0
	[c00000041f157ce0] c0000000000f0d28 worker_thread+0x148/0x610
	[c00000041f157d80] c0000000000f96b0 kthread+0x120/0x140
	[c00000041f157e30] c00000000000c0e0 ret_from_kernel_thread+0x5c/0x7c

This patch simply delays the MSR[TS] set, so, if there is any page fault in
the __get_user() section, it does not have regs->msr[TS] set, since the TM
structures are still invalid, thus avoiding doing TM operations for
in-kernel exceptions and possible process reschedule.

With this patch, the MSR[TS] will only be set just before recheckpointing
and setting TEXASR[FS] = 1, thus avoiding an interrupt with TM registers in
invalid state.

Other than that, if CONFIG_PREEMPT is set, there might be a preemption just
after setting MSR[TS] and before tm_recheckpoint(), thus, this block must
be atomic from a preemption perspective, thus, calling
preempt_disable/enable() on this code.

It is not possible to move tm_recheckpoint to happen earlier, because it is
required to get the checkpointed registers from userspace, with
__get_user(), thus, the only way to avoid this undesired behavior is
delaying the MSR[TS] set.

The 32-bits signal handler seems to be safe this current issue, but, it
might be exposed to the preemption issue, thus, disabling preemption in
this chunk of code.

Changes from v2:
 * Run the critical section with preempt_disable.

Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
Cc: stable@vger.kernel.org (v3.9+)
Signed-off-by: Breno Leitao <leitao@debian.org>

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index e6474a45cef5..fd59fef9931b 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -848,7 +848,23 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	/* If TM bits are set to the reserved value, it's an invalid context */
 	if (MSR_TM_RESV(msr_hi))
 		return 1;
-	/* Pull in the MSR TM bits from the user context */
+
+	/*
+	 * Disabling preemption, since it is unsafe to be preempted
+	 * with MSR[TS] set without recheckpointing.
+	 */
+	preempt_disable();
+
+	/*
+	 * CAUTION:
+	 * After regs->MSR[TS] being updated, make sure that get_user(),
+	 * put_user() or similar functions are *not* called. These
+	 * functions can generate page faults which will cause the process
+	 * to be de-scheduled with MSR[TS] set but without calling
+	 * tm_recheckpoint(). This can cause a bug.
+	 *
+	 * Pull in the MSR TM bits from the user context
+	 */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
 	/* Now, recheckpoint.  This loads up all of the checkpointed (older)
 	 * registers, including FP and V[S]Rs.  After recheckpointing, the
@@ -873,6 +889,8 @@ static long restore_tm_user_regs(struct pt_regs *regs,
 	}
 #endif
 
+	preempt_enable();
+
 	return 0;
 }
 #endif
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 83d51bf586c7..bbd1c73243d7 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -467,20 +467,6 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	if (MSR_TM_RESV(msr))
 		return -EINVAL;
 
-	/* pull in MSR TS bits from user context */
-	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
-
-	/*
-	 * Ensure that TM is enabled in regs->msr before we leave the signal
-	 * handler. It could be the case that (a) user disabled the TM bit
-	 * through the manipulation of the MSR bits in uc_mcontext or (b) the
-	 * TM bit was disabled because a sufficient number of context switches
-	 * happened whilst in the signal handler and load_tm overflowed,
-	 * disabling the TM bit. In either case we can end up with an illegal
-	 * TM state leading to a TM Bad Thing when we return to userspace.
-	 */
-	regs->msr |= MSR_TM;
-
 	/* pull in MSR LE from user context */
 	regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
 
@@ -572,6 +558,34 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 	tm_enable();
 	/* Make sure the transaction is marked as failed */
 	tsk->thread.tm_texasr |= TEXASR_FS;
+
+	/*
+	 * Disabling preemption, since it is unsafe to be preempted
+	 * with MSR[TS] set without recheckpointing.
+	 */
+	preempt_disable();
+
+	/* pull in MSR TS bits from user context */
+	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr & MSR_TS_MASK);
+
+	/*
+	 * Ensure that TM is enabled in regs->msr before we leave the signal
+	 * handler. It could be the case that (a) user disabled the TM bit
+	 * through the manipulation of the MSR bits in uc_mcontext or (b) the
+	 * TM bit was disabled because a sufficient number of context switches
+	 * happened whilst in the signal handler and load_tm overflowed,
+	 * disabling the TM bit. In either case we can end up with an illegal
+	 * TM state leading to a TM Bad Thing when we return to userspace.
+	 *
+	 * CAUTION:
+	 * After regs->MSR[TS] being updated, make sure that get_user(),
+	 * put_user() or similar functions are *not* called. These
+	 * functions can generate page faults which will cause the process
+	 * to be de-scheduled with MSR[TS] set but without calling
+	 * tm_recheckpoint(). This can cause a bug.
+	 */
+	regs->msr |= MSR_TM;
+
 	/* This loads the checkpointed FP/VEC state, if used */
 	tm_recheckpoint(&tsk->thread);
 
@@ -585,6 +599,8 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 		regs->msr |= MSR_VEC;
 	}
 
+	preempt_enable();
+
 	return err;
 }
 #endif
-- 
2.19.0

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

[Breno Leitao]

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.

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.

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 */
+
+#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");
+}
-- 
2.19.0

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

[Michael Neuling]

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");
> +}

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

[Breno Leitao]

Hi Mikey,

On 11/29/18 12:11 AM, Michael Neuling wrote:
> 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.

Yes, it sounded interesting and I implemented the test using madvice(). Thanks
for the suggestion!

The current approach didn't seem to improve the amount of page faults at it
seems that MADV_DONTNEED makes no difference when using a Lazy page loading.
This is the test I did, where 'original' is my current patch and 'madvice` is
the patch below:

  Performance counter stats for './original':

                 0      major-faults                                                
           125,100      minor-faults                                                

       2.575479619 seconds time elapsed


  Performance counter stats for './madvice':

                 0      major-faults                                                
           125,099      minor-faults         



Other than that, I didn't see any improvements in the reproduction rate also, although
it is a bit challenging to measure, since it crashes the machine and I can't run a
full statistical model.

This is the current patch I compared to the original one

---

commit 082a9fe29412943adfa2d6a363f44bac8e81d0ce
Author: Breno Leitao <leitao@debian.org>
Date:   Tue Nov 13 18:02:57 2018 -0500

    selftests/powerpc: New TM signal self test
    
    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. In order to force more page faults at
    signal context, the signal handler and the ucontext are being mapped into a
    MADV_DONTNEED memory chunks.
    
    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.
    
    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.
    
    Signed-off-by: Breno Leitao <leitao@debian.org>

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..496596f3c1bf
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-force-msr.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018, Breno Leitao, Gustavo Romero, IBM Corp.
+ *
+ * This test raises a SIGUSR1 signal, and toggle the MSR[TS]
+ * fields at the signal handler. With MSR[TS] being set, the kernel will
+ * force a recheckpoint, which may cause a segfault when returning to
+ * user space. Since the kernel needs to re-run, the segfault needs to be
+ * caught and handled.
+ *
+ * In order to continue the test even after a segfault, the context is
+ * saved prior to the signal being raised, and it is restored when there is
+ * a segmentation fault. This happens for COUNT_MAX times.
+ */
+
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <string.h>
+#include <ucontext.h>
+#include <unistd.h>
+#include <sys/mman.h>
+
+#include "tm.h"
+#include "utils.h"
+#include "reg.h"
+
+#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 usr_signal_handler(int signo, siginfo_t *si, void *uc)
+{
+	ucontext_t *ucp = uc;
+	int ret;
+
+	/*
+	 * 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 = mmap(NULL, sizeof(ucontext_t),
+			    PROT_READ | PROT_WRITE,
+			    MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+	if (ucp->uc_link == (void *)-1) {
+		perror("Mmap failed");
+		exit(-1);
+	}
+
+	/* Forcing the page to be allocated in a page fault */
+	ret = madvise(ucp->uc_link, sizeof(ucontext_t), MADV_DONTNEED);
+	if (ret) {
+		perror("madvise failed");
+		exit(-1);
+	}
+
+	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 usr_sa, seg_sa;
+	stack_t ss;
+
+	usr_sa.sa_flags = SA_SIGINFO | SA_ONSTACK;
+	usr_sa.sa_sigaction = usr_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);
+
+	/* Allocated am alternative signal stack area */
+	ss.ss_sp = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+	ss.ss_size = SIGSTKSZ;
+	ss.ss_flags = 0;
+
+	if (ss.ss_sp == (void *)-1) {
+		perror("mmap error\n");
+		exit(-1);
+	}
+
+	/* Force the allocation through a page fault */
+	if (madvise(ss.ss_sp, SIGSTKSZ, MADV_DONTNEED)) {
+		perror("madvise\n");
+		exit(-1);
+	}
+
+	/* Setting a alternative stack to generate a page fault when
+	 * the signal is raised.
+	 */
+	if (sigaltstack(&ss, NULL)) {
+		perror("sigaltstack\n");
+		exit(-1);
+	}
+
+	/* The signal handler will enable MSR_TS */
+	sigaction(SIGUSR1, &usr_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");
+}

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

[Michael Ellerman]

Breno Leitao <leitao@debian.org> writes:

> 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.
>
> 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.

Hi Breno,

Thanks for the test, some of these TM tests are getting pretty advanced! :)

Unfortunately it doesn't build in a few configurations.

On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:

  tm-signal-force-msr.c: In function 'trap_signal_handler':
  tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
    ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
                     ^~~~~~~
                     uc_regs
  tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
   #define __MASK(X)       (1UL<<(X))
                               ^~
  tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
   #define MSR_TS_S        __MASK(MSR_TS_S_LG) /* Transaction Suspended */
                           ^~~~~~
  tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
    ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
                                        ^~~~~~~~

And using powerpc64le-linux-gnu-gcc I get:

  In file included from /usr/powerpc64le-linux-gnu/include/string.h:494,
                   from tm-signal-force-msr.c:10:
  In function 'memcpy',
      inlined from 'trap_signal_handler' at tm-signal-force-msr.c:39:2:
  /usr/powerpc64le-linux-gnu/include/bits/string_fortified.h:34:10: error: '__builtin_memcpy' accessing 1272 bytes at offsets 8 and 168 overlaps 1112 bytes at offset 168 [-Werror=restrict]
     return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

cheers

Re: [v2] powerpc/tm: Set MSR[TS] just prior to recheckpoint

[Michael Ellerman]

On Wed, 2018-11-21 at 19:21:09 UTC, Breno Leitao wrote:
> On a signal handler return, the user could set a context with MSR[TS] bits
> set, and these bits would be copied to task regs->msr.
> 
> At restore_tm_sigcontexts(), after current task regs->msr[TS] bits are set,
> several __get_user() are called and then a recheckpoint is executed.
> 
> This is a problem since a page fault (in kernel space) could happen when
> calling __get_user(). If it happens, the process MSR[TS] bits were
> already set, but recheckpoint was not executed, and SPRs are still invalid.
> 
> The page fault can cause the current process to be de-scheduled, with
> MSR[TS] active and without tm_recheckpoint() being called.  More
> importantly, without TEXASR[FS] bit set also.
> 
> Since TEXASR might not have the FS bit set, and when the process is
> scheduled back, it will try to reclaim, which will be aborted because of
> the CPU is not in the suspended state, and, then, recheckpoint. This
> recheckpoint will restore thread->texasr into TEXASR SPR, which might be
> zero, hitting a BUG_ON().
> 
> 	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
> 	cpu 0xb: Vector: 700 (Program Check) at [c00000041f1576d0]
> 	    pc: c000000000054550: restore_gprs+0xb0/0x180
> 	    lr: 0000000000000000
> 	    sp: c00000041f157950
> 	   msr: 8000000100021033
> 	  current = 0xc00000041f143000
> 	  paca    = 0xc00000000fb86300	 softe: 0	 irq_happened: 0x01
> 	    pid   = 1021, comm = kworker/11:1
> 	kernel BUG at /build/linux-sf3Co9/linux-4.9.30/arch/powerpc/kernel/tm.S:434!
> 	Linux version 4.9.0-3-powerpc64le (debian-kernel@lists.debian.org) (gcc version 6.3.0 20170516 (Debian 6.3.0-18) ) #1 SMP Debian 4.9.30-2+deb9u2 (2017-06-26)
> 	enter ? for help
> 	[c00000041f157b30] c00000000001bc3c tm_recheckpoint.part.11+0x6c/0xa0
> 	[c00000041f157b70] c00000000001d184 __switch_to+0x1e4/0x4c0
> 	[c00000041f157bd0] c00000000082eeb8 __schedule+0x2f8/0x990
> 	[c00000041f157cb0] c00000000082f598 schedule+0x48/0xc0
> 	[c00000041f157ce0] c0000000000f0d28 worker_thread+0x148/0x610
> 	[c00000041f157d80] c0000000000f96b0 kthread+0x120/0x140
> 	[c00000041f157e30] c00000000000c0e0 ret_from_kernel_thread+0x5c/0x7c
> 
> This patch simply delays the MSR[TS] set, so, if there is any page fault in
> the __get_user() section, it does not have regs->msr[TS] set, since the TM
> structures are still invalid, thus avoiding doing TM operations for
> in-kernel exceptions and possible process reschedule.
> 
> With this patch, the MSR[TS] will only be set just before recheckpointing
> and setting TEXASR[FS] = 1, thus avoiding an interrupt with TM registers in
> invalid state.
> 
> Other than that, if CONFIG_PREEMPT is set, there might be a preemption just
> after setting MSR[TS] and before tm_recheckpoint(), thus, this block must
> be atomic from a preemption perspective, thus, calling
> preempt_disable/enable() on this code.
> 
> It is not possible to move tm_recheckpoint to happen earlier, because it is
> required to get the checkpointed registers from userspace, with
> __get_user(), thus, the only way to avoid this undesired behavior is
> delaying the MSR[TS] set.
> 
> The 32-bits signal handler seems to be safe this current issue, but, it
> might be exposed to the preemption issue, thus, disabling preemption in
> this chunk of code.
> 
> Changes from v2:
>  * Run the critical section with preempt_disable.
> 
> Fixes: 87b4e5393af7 ("powerpc/tm: Fix return of active 64bit signals")
> Cc: stable@vger.kernel.org (v3.9+)
> Signed-off-by: Breno Leitao <leitao@debian.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/e1c3743e1a20647c53b719dbf28b48

cheers

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

[Breno Leitao]

Hi Michael,

On 12/20/18 10:51 AM, Michael Ellerman wrote:
> Breno Leitao <leitao@debian.org> writes:
> 
>> 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.
>>
>> 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.
> And reference the ucontext->mcontext MSR using UCONTEXT_MSR() macro.
> Hi Breno,
> 
> Thanks for the test, some of these TM tests are getting pretty advanced! :)
> 
> Unfortunately it doesn't build in a few configurations.
> 
> On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:
> 
>   tm-signal-force-msr.c: In function 'trap_signal_handler':
>   tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
>     ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
>                      ^~~~~~~
>                      uc_regs
>   tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
>    #define __MASK(X)       (1UL<<(X))
>                                ^~
>   tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
>    #define MSR_TS_S        __MASK(MSR_TS_S_LG) /* Transaction Suspended */
>                            ^~~~~~
>   tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
>     ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
>                                         ^~~~~~~~
> 

That is because I missed the -m64 compilation flag on Makefile. I understand
that this test only make sense when compiled in 64 bits. Do you agree?

I might also add a macro to address ucontext->mcontext MSR. This will avoid
problems like that in the future.

 index ae43a614835d..7636bf45d5d5 100644
 --- a/tools/testing/selftests/powerpc/include/utils.h
 +++ b/tools/testing/selftests/powerpc/include/utils.h
 @@ -102,8 +102,10 @@ do {


  #if defined(__powerpc64__)
  #define UCONTEXT_NIA(UC)       (UC)->uc_mcontext.gp_regs[PT_NIP]
 +#define UCONTEXT_MSR(UC)       (UC)->uc_mcontext.gp_regs[PT_MSR]
  #elif defined(__powerpc__)
  #define UCONTEXT_NIA(UC)       (UC)->uc_mcontext.uc_regs->gregs[PT_NIP]
 +#define UCONTEXT_MSR(UC)       (UC)->uc_mcontext.uc_regs->gregs[PT_MSR]
  #else
  #error implement UCONTEXT_NIA
  #endif

> And using powerpc64le-linux-gnu-gcc I get:
> 
>   In file included from /usr/powerpc64le-linux-gnu/include/string.h:494,
>                    from tm-signal-force-msr.c:10:
>   In function 'memcpy',
>       inlined from 'trap_signal_handler' at tm-signal-force-msr.c:39:2:
>   /usr/powerpc64le-linux-gnu/include/bits/string_fortified.h:34:10: error: '__builtin_memcpy' accessing 1272 bytes at offsets 8 and 168 overlaps 1112 bytes at offset 168 [-Werror=restrict]
>      return __builtin___memcpy_chk (__dest, __src, __len, __bos0 (__dest));
>             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Damn, that is because I do not know how to use C pointers. Fixing it on v3 also.

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

[Michael Ellerman]

Breno Leitao <leitao@debian.org> writes:
> On 12/20/18 10:51 AM, Michael Ellerman wrote:
>> Breno Leitao <leitao@debian.org> writes:
>> 
>>> 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.
>>>
>>> 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.
>> And reference the ucontext->mcontext MSR using UCONTEXT_MSR() macro.
>> Hi Breno,
>> 
>> Thanks for the test, some of these TM tests are getting pretty advanced! :)
>> 
>> Unfortunately it doesn't build in a few configurations.
>> 
>> On Ubuntu 18.10 built with powerpc-linux-gnu-gcc I get:
>> 
>>   tm-signal-force-msr.c: In function 'trap_signal_handler':
>>   tm-signal-force-msr.c:42:19: error: 'union uc_regs_ptr' has no member named 'gp_regs'; did you mean 'uc_regs'?
>>     ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
>>                      ^~~~~~~
>>                      uc_regs
>>   tm-signal-force-msr.c:17:29: error: left shift count >= width of type [-Werror=shift-count-overflow]
>>    #define __MASK(X)       (1UL<<(X))
>>                                ^~
>>   tm-signal-force-msr.c:20:25: note: in expansion of macro '__MASK'
>>    #define MSR_TS_S        __MASK(MSR_TS_S_LG) /* Transaction Suspended */
>>                            ^~~~~~
>>   tm-signal-force-msr.c:42:38: note: in expansion of macro 'MSR_TS_S'
>>     ucp->uc_mcontext.gp_regs[PT_MSR] |= MSR_TS_S;
>>                                         ^~~~~~~~
>> 
>
> That is because I missed the -m64 compilation flag on Makefile. I understand
> that this test only make sense when compiled in 64 bits. Do you agree?

I think the test could work as a 32-bit binary on a 64-bit kernel, but I
don't mind if you force it to build 64-bit.

cheers