[PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Gustavo Luiz Duarte]

After a treclaim, we expect to be in non-transactional state. If we don't
immediately clear the current thread's MSR[TS] and we get preempted, then
tm_recheckpoint_new_task() will recheckpoint and we get rescheduled in
suspended transaction state.

When handling a signal caught in transactional state, handle_rt_signal64()
calls get_tm_stackpointer() that treclaims the transaction using
tm_reclaim_current() but without clearing the thread's MSR[TS]. This can cause
the TM Bad Thing exception below if later we pagefault and get preempted trying
to access the user's sigframe, using __put_user(). Afterwards, when we are
rescheduled back into do_page_fault() (but now in suspended state since the
thread's MSR[TS] was not cleared), upon executing 'rfid' after completion of
the page fault handling, the exception is raised because a transition from
suspended to non-transactional state is invalid.

	Unexpected TM Bad Thing exception at c00000000000de44 (msr 0x8000000302a03031) tm_scratch=800000010280b033
	Oops: Unrecoverable exception, sig: 6 [#1]
	LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
	Modules linked in: nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6_tables ip_tables nft_compat ip_set nf_tables nfnetlink xts vmx_crypto sg virtio_balloon
	r_mod cdrom virtio_net net_failover virtio_blk virtio_scsi failover dm_mirror dm_region_hash dm_log dm_mod
	CPU: 25 PID: 15547 Comm: a.out Not tainted 5.4.0-rc2 #32
	NIP:  c00000000000de44 LR: c000000000034728 CTR: 0000000000000000
	REGS: c00000003fe7bd70 TRAP: 0700   Not tainted  (5.4.0-rc2)
	MSR:  8000000302a03031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[SE]>  CR: 44000884  XER: 00000000
	CFAR: c00000000000dda4 IRQMASK: 0
	PACATMSCRATCH: 800000010280b033
	GPR00: c000000000034728 c000000f65a17c80 c000000001662800 00007fffacf3fd78
	GPR04: 0000000000001000 0000000000001000 0000000000000000 c000000f611f8af0
	GPR08: 0000000000000000 0000000078006001 0000000000000000 000c000000000000
	GPR12: c000000f611f84b0 c00000003ffcb200 0000000000000000 0000000000000000
	GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
	GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000f611f8140
	GPR24: 0000000000000000 00007fffacf3fd68 c000000f65a17d90 c000000f611f7800
	GPR28: c000000f65a17e90 c000000f65a17e90 c000000001685e18 00007fffacf3f000
	NIP [c00000000000de44] fast_exception_return+0xf4/0x1b0
	LR [c000000000034728] handle_rt_signal64+0x78/0xc50
	Call Trace:
	[c000000f65a17c80] [c000000000034710] handle_rt_signal64+0x60/0xc50 (unreliable)
	[c000000f65a17d30] [c000000000023640] do_notify_resume+0x330/0x460
	[c000000f65a17e20] [c00000000000dcc4] ret_from_except_lite+0x70/0x74
	Instruction dump:
	7c4ff120 e8410170 7c5a03a6 38400000 f8410060 e8010070 e8410080 e8610088
	60000000 60000000 e8810090 e8210078 <4c000024> 48000000 e8610178 88ed0989
	---[ end trace 93094aa44b442f87 ]---

The simplified sequence of events that triggers the above exception is:

  ...				# userspace in NON-TRANSACTIONAL state
  tbegin			# userspace in TRANSACTIONAL state
  signal delivery		# kernelspace in SUSPENDED state
  handle_rt_signal64()
    get_tm_stackpointer()
      treclaim			# kernelspace in NON-TRANSACTIONAL state
    __put_user()
      page fault happens. We will never get back here because of the TM Bad Thing exception.

  page fault handling kicks in and we voluntarily preempt ourselves
  do_page_fault()
    __schedule()
      __switch_to(other_task)

  our task is rescheduled and we recheckpoint because the thread's MSR[TS] was not cleared
  __switch_to(our_task)
    switch_to_tm()
      tm_recheckpoint_new_task()
        trechkpt			# kernelspace in SUSPENDED state

  The page fault handling resumes, but now we are in suspended transaction state
  do_page_fault()    completes
  rfid     <----- trying to get back where the page fault happened (we were non-transactional back then)
  TM Bad Thing			# illegal transition from suspended to non-transactional

This patch fixes that issue by clearing the current thread's MSR[TS] just after
treclaim in get_tm_stackpointer() so that we stay in non-transactional state in
case we are preempted. In order to make treclaim and clearing the thread's
MSR[TS] atomic from a preemption perspective when CONFIG_PREEMPT is set,
preempt_disable/enable() is used. It's also necessary to save the previous
value of the thread's MSR before get_tm_stackpointer() is called so that it can
be exposed to the signal handler later in setup_tm_sigcontexts() to inform the
userspace MSR at the moment of the signal delivery.

Found with tm-signal-context-force-tm kernel selftest on P8 KVM.

v2: Fix build failure when tm is disabled.

Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
Cc: stable@vger.kernel.org # v3.9
Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
---
 arch/powerpc/kernel/signal.c    | 17 +++++++++++++++--
 arch/powerpc/kernel/signal_32.c | 28 ++++++++++++++--------------
 arch/powerpc/kernel/signal_64.c | 22 ++++++++++------------
 3 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
index e6c30cee6abf..1660be1061ac 100644
--- a/arch/powerpc/kernel/signal.c
+++ b/arch/powerpc/kernel/signal.c
@@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
 	 * normal/non-checkpointed stack pointer.
 	 */
 
+	unsigned long ret = tsk->thread.regs->gpr[1];
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	BUG_ON(tsk != current);
 
 	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
+		preempt_disable();
 		tm_reclaim_current(TM_CAUSE_SIGNAL);
 		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
-			return tsk->thread.ckpt_regs.gpr[1];
+			ret = tsk->thread.ckpt_regs.gpr[1];
+
+		/* If we treclaim, we must immediately clear the current
+		 * thread's TM bits. Otherwise we might be preempted and have
+		 * the live MSR[TS] changed behind our back
+		 * (tm_recheckpoint_new_task() would recheckpoint).
+		 * Besides, we enter the signal handler in non-transactional
+		 * state.
+		 */
+		tsk->thread.regs->msr &= ~MSR_TS_MASK;
+		preempt_enable();
 	}
 #endif
-	return tsk->thread.regs->gpr[1];
+	return ret;
 }
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 98600b276f76..1b090a76b444 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
  */
 static int save_tm_user_regs(struct pt_regs *regs,
 			     struct mcontext __user *frame,
-			     struct mcontext __user *tm_frame, int sigret)
+			     struct mcontext __user *tm_frame, int sigret,
+			     unsigned long msr)
 {
-	unsigned long msr = regs->msr;
-
 	WARN_ON(tm_suspend_disabled);
 
-	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
-	 * just indicates to userland that we were doing a transaction, but we
-	 * don't want to return in transactional state.  This also ensures
-	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
-	 */
-	regs->msr &= ~MSR_TS_MASK;
-
 	/* Save both sets of general registers */
 	if (save_general_regs(&current->thread.ckpt_regs, frame)
 	    || save_general_regs(regs, tm_frame))
@@ -912,6 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
+#endif
 
 	BUG_ON(tsk != current);
 
@@ -944,13 +940,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_frame = &rt_sf->uc_transact.uc_mcontext;
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		if (__put_user((unsigned long)&rt_sf->uc_transact,
 			       &rt_sf->uc.uc_link) ||
 		    __put_user((unsigned long)tm_frame,
 			       &rt_sf->uc_transact.uc_regs))
 			goto badframe;
-		if (save_tm_user_regs(regs, frame, tm_frame, sigret))
+		if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
 			goto badframe;
 	}
 	else
@@ -1369,6 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
+#endif
 
 	BUG_ON(tsk != current);
 
@@ -1402,9 +1402,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	tm_mctx = &frame->mctx_transact;
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
-				      sigret))
+				      sigret, msr))
 			goto badframe;
 	}
 	else
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 117515564ec7..84ed2e77ef9c 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 				 struct sigcontext __user *tm_sc,
 				 struct task_struct *tsk,
-				 int signr, sigset_t *set, unsigned long handler)
+				 int signr, sigset_t *set, unsigned long handler,
+				 unsigned long msr)
 {
 	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
 	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
@@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 	elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
 #endif
 	struct pt_regs *regs = tsk->thread.regs;
-	unsigned long msr = tsk->thread.regs->msr;
 	long err = 0;
 
 	BUG_ON(tsk != current);
 
-	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
+	BUG_ON(!MSR_TM_ACTIVE(msr));
 
 	WARN_ON(tm_suspend_disabled);
 
@@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 	 */
 	msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
 
-	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
-	 * just indicates to userland that we were doing a transaction, but we
-	 * don't want to return in transactional state.  This also ensures
-	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
-	 */
-	regs->msr &= ~MSR_TS_MASK;
-
 #ifdef CONFIG_ALTIVEC
 	err |= __put_user(v_regs, &sc->v_regs);
 	err |= __put_user(tm_v_regs, &tm_sc->v_regs);
@@ -824,6 +817,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsigned long newsp = 0;
 	long err = 0;
 	struct pt_regs *regs = tsk->thread.regs;
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
+#endif
 
 	BUG_ON(tsk != current);
 
@@ -841,7 +838,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	err |= __put_user(0, &frame->uc.uc_flags);
 	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	if (MSR_TM_ACTIVE(regs->msr)) {
+	if (MSR_TM_ACTIVE(msr)) {
 		/* The ucontext_t passed to userland points to the second
 		 * ucontext_t (for transactional state) with its uc_link ptr.
 		 */
@@ -849,7 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
 					    &frame->uc_transact.uc_mcontext,
 					    tsk, ksig->sig, NULL,
-					    (unsigned long)ksig->ka.sa.sa_handler);
+					    (unsigned long)ksig->ka.sa.sa_handler,
+					    msr);
 	} else
 #endif
 	{
-- 
2.21.1

[PATCH v2 2/3] selftests/powerpc: Add tm-signal-pagefault test

[Gustavo Luiz Duarte]

This test triggers a TM Bad Thing by raising a signal in transactional state
and forcing a pagefault to happen in kernelspace when the kernel signal
handling code first touches the user signal stack.

This is inspired by the test tm-signal-context-force-tm but uses userfaultfd to
make the test deterministic. While this test always triggers the bug in one
run, I had to execute tm-signal-context-force-tm several times (the test runs
5000 times each execution) to trigger the same bug.

tm-signal-context-force-tm is kept instead of replaced because, while this test
is more reliable and triggers the same bug, tm-signal-context-force-tm has a
better coverage, in the sense that by running the test several times it might
trigger the pagefault and/or be preempted at different places.

Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
---
 tools/testing/selftests/powerpc/tm/.gitignore |   1 +
 tools/testing/selftests/powerpc/tm/Makefile   |   3 +-
 .../powerpc/tm/tm-signal-pagefault.c          | 272 ++++++++++++++++++
 3 files changed, 275 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-signal-pagefault.c

diff --git a/tools/testing/selftests/powerpc/tm/.gitignore b/tools/testing/selftests/powerpc/tm/.gitignore
index 98f2708d86cc..e1c72a4a3e91 100644
--- a/tools/testing/selftests/powerpc/tm/.gitignore
+++ b/tools/testing/selftests/powerpc/tm/.gitignore
@@ -13,6 +13,7 @@ tm-signal-context-chk-vmx
 tm-signal-context-chk-vsx
 tm-signal-context-force-tm
 tm-signal-sigreturn-nt
+tm-signal-pagefault
 tm-vmx-unavail
 tm-unavailable
 tm-trap
diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index b15a1a325bd0..b1d99736f8b8 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -5,7 +5,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 tm-signal-sigreturn-nt \
-	tm-signal-context-force-tm tm-poison
+	tm-signal-context-force-tm tm-poison tm-signal-pagefault
 
 top_srcdir = ../../../../..
 include ../../lib.mk
@@ -22,6 +22,7 @@ $(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-context-force-tm: CFLAGS += -pthread -m64
+$(OUTPUT)/tm-signal-pagefault: CFLAGS += -pthread -m64
 
 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-pagefault.c b/tools/testing/selftests/powerpc/tm/tm-signal-pagefault.c
new file mode 100644
index 000000000000..3a2166101d94
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-pagefault.c
@@ -0,0 +1,272 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020, Gustavo Luiz Duarte, IBM Corp.
+ *
+ * This test starts a transaction and triggers a signal, forcing a pagefault to
+ * happen when the kernel signal handling code touches the user signal stack.
+ *
+ * In order to avoid pre-faulting the signal stack memory and to force the
+ * pagefault to happen precisely in the kernel signal handling code, the
+ * pagefault handling is done in userspace using the userfaultfd facility.
+ *
+ * Further pagefaults are triggered by crafting the signal handler's ucontext
+ * to point to additional memory regions managed by the userfaultfd, so using
+ * the same mechanism used to avoid pre-faulting the signal stack memory.
+ *
+ * On failure (bug is present) kernel crashes or never returns control back to
+ * userspace. If bug is not present, tests completes almost immediately.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <linux/userfaultfd.h>
+#include <poll.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <sys/syscall.h>
+#include <fcntl.h>
+#include <sys/mman.h>
+#include <pthread.h>
+#include <signal.h>
+
+#include "tm.h"
+
+
+#define UF_MEM_SIZE 655360	/* 10 x 64k pages */
+
+/* Memory handled by userfaultfd */
+static char *uf_mem;
+static size_t uf_mem_offset = 0;
+
+/*
+ * Data that will be copied into the faulting pages (instead of zero-filled
+ * pages). This is used to make the test more reliable and avoid segfaulting
+ * when we return from the signal handler. Since we are making the signal
+ * handler's ucontext point to newly allocated memory, when that memory is
+ * paged-in it will contain the expected content.
+ */
+static char backing_mem[UF_MEM_SIZE];
+
+static size_t pagesize;
+
+/*
+ * Return a chunk of at least 'size' bytes of memory that will be handled by
+ * userfaultfd. If 'backing_data' is not NULL, its content will be save to
+ * 'backing_mem' and then copied into the faulting pages when the page fault
+ * is handled.
+ */
+void *get_uf_mem(size_t size, void *backing_data)
+{
+	void *ret;
+
+	if (uf_mem_offset + size > UF_MEM_SIZE) {
+		fprintf(stderr, "Requesting more uf_mem than expected!\n");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = &uf_mem[uf_mem_offset];
+
+	/* Save the data that will be copied into the faulting page */
+	if (backing_data != NULL)
+		memcpy(&backing_mem[uf_mem_offset], backing_data, size);
+
+	/* Reserve the requested amount of uf_mem */
+	uf_mem_offset += size;
+	/* Keep uf_mem_offset aligned to the page size (round up) */
+	uf_mem_offset = (uf_mem_offset + pagesize - 1) & ~(pagesize - 1);
+
+	return ret;
+}
+
+void *fault_handler_thread(void *arg)
+{
+	struct uffd_msg msg;	/* Data read from userfaultfd */
+	long uffd;		/* userfaultfd file descriptor */
+	struct uffdio_copy uffdio_copy;
+	struct pollfd pollfd;
+	ssize_t nread, offset;
+
+	uffd = (long) arg;
+
+	for (;;) {
+		pollfd.fd = uffd;
+		pollfd.events = POLLIN;
+		if (poll(&pollfd, 1, -1) == -1) {
+			perror("poll() failed");
+			exit(EXIT_FAILURE);
+		}
+
+		nread = read(uffd, &msg, sizeof(msg));
+		if (nread == 0) {
+			fprintf(stderr, "read(): EOF on userfaultfd\n");
+			exit(EXIT_FAILURE);
+		}
+
+		if (nread == -1) {
+			perror("read() failed");
+			exit(EXIT_FAILURE);
+		}
+
+		/* We expect only one kind of event */
+		if (msg.event != UFFD_EVENT_PAGEFAULT) {
+			fprintf(stderr, "Unexpected event on userfaultfd\n");
+			exit(EXIT_FAILURE);
+		}
+
+		/*
+		 * We need to handle page faults in units of pages(!).
+		 * So, round faulting address down to page boundary.
+		 */
+		uffdio_copy.dst = msg.arg.pagefault.address & ~(pagesize-1);
+
+		offset = (char *) uffdio_copy.dst - uf_mem;
+		uffdio_copy.src = (unsigned long) &backing_mem[offset];
+
+		uffdio_copy.len = pagesize;
+		uffdio_copy.mode = 0;
+		uffdio_copy.copy = 0;
+		if (ioctl(uffd, UFFDIO_COPY, &uffdio_copy) == -1) {
+			perror("ioctl-UFFDIO_COPY failed");
+			exit(EXIT_FAILURE);
+		}
+	}
+}
+
+void setup_uf_mem(void)
+{
+	long uffd;		/* userfaultfd file descriptor */
+	pthread_t thr;
+	struct uffdio_api uffdio_api;
+	struct uffdio_register uffdio_register;
+	int ret;
+
+	pagesize = sysconf(_SC_PAGE_SIZE);
+
+	/* Create and enable userfaultfd object */
+	uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
+	if (uffd == -1) {
+		perror("userfaultfd() failed");
+		exit(EXIT_FAILURE);
+	}
+	uffdio_api.api = UFFD_API;
+	uffdio_api.features = 0;
+	if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
+		perror("ioctl-UFFDIO_API failed");
+		exit(EXIT_FAILURE);
+	}
+
+	/*
+	 * Create a private anonymous mapping. The memory will be demand-zero
+	 * paged, that is, not yet allocated. When we actually touch the memory
+	 * the related page will be allocated via the userfaultfd mechanism.
+	 */
+	uf_mem = mmap(NULL, UF_MEM_SIZE, PROT_READ | PROT_WRITE,
+		      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	if (uf_mem == MAP_FAILED) {
+		perror("mmap() failed");
+		exit(EXIT_FAILURE);
+	}
+
+	/*
+	 * Register the memory range of the mapping we've just mapped to be
+	 * handled by the userfaultfd object. In 'mode' we request to track
+	 * missing pages (i.e. pages that have not yet been faulted-in).
+	 */
+	uffdio_register.range.start = (unsigned long) uf_mem;
+	uffdio_register.range.len = UF_MEM_SIZE;
+	uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
+	if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
+		perror("ioctl-UFFDIO_REGISTER");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Create a thread that will process the userfaultfd events */
+	ret = pthread_create(&thr, NULL, fault_handler_thread, (void *) uffd);
+	if (ret != 0) {
+		fprintf(stderr, "pthread_create(): Error. Returned %d\n", ret);
+		exit(EXIT_FAILURE);
+	}
+}
+
+/*
+ * Assumption: the signal was delivered while userspace was in transactional or
+ * suspended state, i.e. uc->uc_link != NULL.
+ */
+void signal_handler(int signo, siginfo_t *si, void *uc)
+{
+	ucontext_t *ucp = uc;
+
+	/* Skip 'trap' after returning, otherwise we get a SIGTRAP again */
+	ucp->uc_link->uc_mcontext.regs->nip += 4;
+
+	ucp->uc_mcontext.v_regs =
+		get_uf_mem(sizeof(elf_vrreg_t), ucp->uc_mcontext.v_regs);
+
+	ucp->uc_link->uc_mcontext.v_regs =
+		get_uf_mem(sizeof(elf_vrreg_t), ucp->uc_link->uc_mcontext.v_regs);
+
+	ucp->uc_link = get_uf_mem(sizeof(ucontext_t), ucp->uc_link);
+}
+
+int tm_signal_pagefault(void)
+{
+	struct sigaction sa;
+	stack_t ss;
+
+	SKIP_IF(!have_htm());
+
+	setup_uf_mem();
+
+	/*
+	 * Set an alternative stack that will generate a page fault when the
+	 * signal is raised. The page fault will be treated via userfaultfd,
+	 * i.e. via fault_handler_thread.
+	 */
+	ss.ss_sp = get_uf_mem(SIGSTKSZ, NULL);
+	ss.ss_size = SIGSTKSZ;
+	ss.ss_flags = 0;
+	if (sigaltstack(&ss, NULL) == -1) {
+		perror("sigaltstack() failed");
+		exit(EXIT_FAILURE);
+	}
+
+	sa.sa_flags = SA_SIGINFO | SA_ONSTACK;
+	sa.sa_sigaction = signal_handler;
+	if (sigaction(SIGTRAP, &sa, NULL) == -1) {
+		perror("sigaction() failed");
+		exit(EXIT_FAILURE);
+	}
+
+	/* Trigger a SIGTRAP in transactional state */
+	asm __volatile__(
+			"tbegin.;"
+			"beq    1f;"
+			"trap;"
+			"1: ;"
+			: : : "memory");
+
+	/* Trigger a SIGTRAP in suspended state */
+	asm __volatile__(
+			"tbegin.;"
+			"beq    1f;"
+			"tsuspend.;"
+			"trap;"
+			"tresume.;"
+			"1: ;"
+			: : : "memory");
+
+	return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+	/*
+	 * Depending on kernel config, the TM Bad Thing might not result in a
+	 * crash, instead the kernel never returns control back to userspace, so
+	 * set a tight timeout. If the test passes it completes almost
+	 * immediately.
+	 */
+	test_harness_set_timeout(2);
+	return test_harness(tm_signal_pagefault, "tm_signal_pagefault");
+}
-- 
2.21.1

[PATCH v2 3/3] selftests/powerpc: Don't rely on segfault to rerun the test

[Gustavo Luiz Duarte]

The test case tm-signal-context-force-tm expects a segfault to happen on
returning from signal handler, and then does a setcontext() to run the test
again. However, the test doesn't always segfault, causing the test to run a
single time.

This patch fixes the test by putting it within a loop and jumping, via
setcontext, just prior to the loop in case it segfaults. This way we get the
desired behavior (run the test COUNT_MAX times) regardless if it segfaults or
not. This also reduces the use of setcontext for control flow logic, keeping it
only in the segfault handler.

Also, since 'count' is changed within the signal handler, it is declared as
volatile to prevent any compiler optimization getting confused with
asynchronous changes.

Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
---
 .../powerpc/tm/tm-signal-context-force-tm.c   | 79 +++++++++----------
 1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
index 31717625f318..9ff7bdb6d47a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
@@ -42,9 +42,10 @@
 #endif
 
 /* Setting contexts because the test will crash and we want to recover */
-ucontext_t init_context, main_context;
+ucontext_t init_context;
 
-static int count, first_time;
+/* count is changed in the signal handler, so it must be volatile */
+static volatile int count;
 
 void usr_signal_handler(int signo, siginfo_t *si, void *uc)
 {
@@ -98,11 +99,6 @@ void usr_signal_handler(int signo, siginfo_t *si, void *uc)
 
 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 */
@@ -126,37 +122,40 @@ void tm_trap_test(void)
 	 */
 	getcontext(&init_context);
 
-	/* Allocated an 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 an alternative stack to generate a page fault when
-	 * the signal is raised.
-	 */
-	if (sigaltstack(&ss, NULL)) {
-		perror("sigaltstack\n");
-		exit(-1);
+	while (count < COUNT_MAX) {
+		/* Allocated an 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 an 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 might segfault, avoid it to retest */
+		sigaction(SIGSEGV, &seg_sa, NULL);
+
+		raise(SIGUSR1);
+		count++;
 	}
-
-	/* 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_context_force_tm(void)
@@ -169,11 +168,7 @@ int tm_signal_context_force_tm(void)
 	 */
 	SKIP_IF(!is_ppc64le());
 
-	/* Will get back here after COUNT_MAX interactions */
-	getcontext(&main_context);
-
-	if (!first_time++)
-		tm_trap_test();
+	tm_trap_test();
 
 	return EXIT_SUCCESS;
 }
-- 
2.21.1

Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Michael Neuling]

Other than the minor things below that I think you need, the patch good with me.

Acked-by: Michael Neuling <mikey@neuling.org>

> Subject: Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

The subject should mention "signals".

On Mon, 2020-02-03 at 13:09 -0300, Gustavo Luiz Duarte wrote:
> After a treclaim, we expect to be in non-transactional state. If we don't
> immediately clear the current thread's MSR[TS] and we get preempted, then
> tm_recheckpoint_new_task() will recheckpoint and we get rescheduled in
> suspended transaction state.

It's not "immediately", it's before re-enabling preemption. 

There is a similar comment in the code that needs to be fixed too.

> When handling a signal caught in transactional state, handle_rt_signal64()
> calls get_tm_stackpointer() that treclaims the transaction using
> tm_reclaim_current() but without clearing the thread's MSR[TS]. This can cause
> the TM Bad Thing exception below if later we pagefault and get preempted trying
> to access the user's sigframe, using __put_user(). Afterwards, when we are
> rescheduled back into do_page_fault() (but now in suspended state since the
> thread's MSR[TS] was not cleared), upon executing 'rfid' after completion of
> the page fault handling, the exception is raised because a transition from
> suspended to non-transactional state is invalid.
> 
> 	Unexpected TM Bad Thing exception at c00000000000de44 (msr 0x8000000302a03031) tm_scratch=800000010280b033
> 	Oops: Unrecoverable exception, sig: 6 [#1]
> 	LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> 	Modules linked in: nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6_tables ip_tables nft_compat ip_set nf_tables nfnetlink xts vmx_crypto sg virtio_balloon
> 	r_mod cdrom virtio_net net_failover virtio_blk virtio_scsi failover dm_mirror dm_region_hash dm_log dm_mod
> 	CPU: 25 PID: 15547 Comm: a.out Not tainted 5.4.0-rc2 #32
> 	NIP:  c00000000000de44 LR: c000000000034728 CTR: 0000000000000000
> 	REGS: c00000003fe7bd70 TRAP: 0700   Not tainted  (5.4.0-rc2)
> 	MSR:  8000000302a03031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[SE]>  CR: 44000884  XER: 00000000
> 	CFAR: c00000000000dda4 IRQMASK: 0
> 	PACATMSCRATCH: 800000010280b033
> 	GPR00: c000000000034728 c000000f65a17c80 c000000001662800 00007fffacf3fd78
> 	GPR04: 0000000000001000 0000000000001000 0000000000000000 c000000f611f8af0
> 	GPR08: 0000000000000000 0000000078006001 0000000000000000 000c000000000000
> 	GPR12: c000000f611f84b0 c00000003ffcb200 0000000000000000 0000000000000000
> 	GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> 	GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000f611f8140
> 	GPR24: 0000000000000000 00007fffacf3fd68 c000000f65a17d90 c000000f611f7800
> 	GPR28: c000000f65a17e90 c000000f65a17e90 c000000001685e18 00007fffacf3f000
> 	NIP [c00000000000de44] fast_exception_return+0xf4/0x1b0
> 	LR [c000000000034728] handle_rt_signal64+0x78/0xc50
> 	Call Trace:
> 	[c000000f65a17c80] [c000000000034710] handle_rt_signal64+0x60/0xc50 (unreliable)
> 	[c000000f65a17d30] [c000000000023640] do_notify_resume+0x330/0x460
> 	[c000000f65a17e20] [c00000000000dcc4] ret_from_except_lite+0x70/0x74
> 	Instruction dump:
> 	7c4ff120 e8410170 7c5a03a6 38400000 f8410060 e8010070 e8410080 e8610088
> 	60000000 60000000 e8810090 e8210078 <4c000024> 48000000 e8610178 88ed0989
> 	---[ end trace 93094aa44b442f87 ]---
> 
> The simplified sequence of events that triggers the above exception is:
> 
>   ...				# userspace in NON-TRANSACTIONAL state
>   tbegin			# userspace in TRANSACTIONAL state
>   signal delivery		# kernelspace in SUSPENDED state
>   handle_rt_signal64()
>     get_tm_stackpointer()
>       treclaim			# kernelspace in NON-TRANSACTIONAL state
>     __put_user()
>       page fault happens. We will never get back here because of the TM Bad Thing exception.
> 
>   page fault handling kicks in and we voluntarily preempt ourselves
>   do_page_fault()
>     __schedule()
>       __switch_to(other_task)
> 
>   our task is rescheduled and we recheckpoint because the thread's MSR[TS] was not cleared
>   __switch_to(our_task)
>     switch_to_tm()
>       tm_recheckpoint_new_task()
>         trechkpt			# kernelspace in SUSPENDED state
> 
>   The page fault handling resumes, but now we are in suspended transaction state
>   do_page_fault()    completes
>   rfid     <----- trying to get back where the page fault happened (we were non-transactional back then)
>   TM Bad Thing			# illegal transition from suspended to non-transactional
> 
> This patch fixes that issue by clearing the current thread's MSR[TS] just after
> treclaim in get_tm_stackpointer() so that we stay in non-transactional state in
> case we are preempted. In order to make treclaim and clearing the thread's
> MSR[TS] atomic from a preemption perspective when CONFIG_PREEMPT is set,
> preempt_disable/enable() is used. It's also necessary to save the previous
> value of the thread's MSR before get_tm_stackpointer() is called so that it can
> be exposed to the signal handler later in setup_tm_sigcontexts() to inform the
> userspace MSR at the moment of the signal delivery.
> 
> Found with tm-signal-context-force-tm kernel selftest on P8 KVM.

Why are you mentioning KVM?

> 
> v2: Fix build failure when tm is disabled.
> 
> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
> Cc: stable@vger.kernel.org # v3.9
> Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
> ---
>  arch/powerpc/kernel/signal.c    | 17 +++++++++++++++--
>  arch/powerpc/kernel/signal_32.c | 28 ++++++++++++++--------------
>  arch/powerpc/kernel/signal_64.c | 22 ++++++++++------------
>  3 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> index e6c30cee6abf..1660be1061ac 100644
> --- a/arch/powerpc/kernel/signal.c
> +++ b/arch/powerpc/kernel/signal.c
> @@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
>  	 * normal/non-checkpointed stack pointer.
>  	 */
>  
> +	unsigned long ret = tsk->thread.regs->gpr[1];
> +
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	BUG_ON(tsk != current);
>  
>  	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
> +		preempt_disable();
>  		tm_reclaim_current(TM_CAUSE_SIGNAL);
>  		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
> -			return tsk->thread.ckpt_regs.gpr[1];
> +			ret = tsk->thread.ckpt_regs.gpr[1];
> +
> +		/* If we treclaim, we must immediately clear the current
> +		 * thread's TM bits. Otherwise we might be preempted and have
> +		 * the live MSR[TS] changed behind our back
> +		 * (tm_recheckpoint_new_task() would recheckpoint).
> +		 * Besides, we enter the signal handler in non-transactional
> +		 * state.
> +		 */
> +		tsk->thread.regs->msr &= ~MSR_TS_MASK;
> +		preempt_enable();
>  	}
>  #endif
> -	return tsk->thread.regs->gpr[1];
> +	return ret;
>  }
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 98600b276f76..1b090a76b444 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>   */
>  static int save_tm_user_regs(struct pt_regs *regs,
>  			     struct mcontext __user *frame,
> -			     struct mcontext __user *tm_frame, int sigret)
> +			     struct mcontext __user *tm_frame, int sigret,
> +			     unsigned long msr)
>  {
> -	unsigned long msr = regs->msr;
> -
>  	WARN_ON(tm_suspend_disabled);
>  
> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
> -	 * just indicates to userland that we were doing a transaction, but we
> -	 * don't want to return in transactional state.  This also ensures
> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> -	 */
> -	regs->msr &= ~MSR_TS_MASK;
> -
>  	/* Save both sets of general registers */
>  	if (save_general_regs(&current->thread.ckpt_regs, frame)
>  	    || save_general_regs(regs, tm_frame))
> @@ -912,6 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>  	int sigret;
>  	unsigned long tramp;
>  	struct pt_regs *regs = tsk->thread.regs;
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
> +#endif
>  
>  	BUG_ON(tsk != current);
>  
> @@ -944,13 +940,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>  
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	tm_frame = &rt_sf->uc_transact.uc_mcontext;
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>  		if (__put_user((unsigned long)&rt_sf->uc_transact,
>  			       &rt_sf->uc.uc_link) ||
>  		    __put_user((unsigned long)tm_frame,
>  			       &rt_sf->uc_transact.uc_regs))
>  			goto badframe;
> -		if (save_tm_user_regs(regs, frame, tm_frame, sigret))
> +		if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
>  			goto badframe;
>  	}
>  	else
> @@ -1369,6 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>  	int sigret;
>  	unsigned long tramp;
>  	struct pt_regs *regs = tsk->thread.regs;
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
> +#endif
>  
>  	BUG_ON(tsk != current);
>  
> @@ -1402,9 +1402,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>  
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	tm_mctx = &frame->mctx_transact;
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>  		if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
> -				      sigret))
> +				      sigret, msr))
>  			goto badframe;
>  	}
>  	else
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 117515564ec7..84ed2e77ef9c 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>  				 struct sigcontext __user *tm_sc,
>  				 struct task_struct *tsk,
> -				 int signr, sigset_t *set, unsigned long handler)
> +				 int signr, sigset_t *set, unsigned long handler,
> +				 unsigned long msr)
>  {
>  	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
>  	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>  	elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
>  #endif
>  	struct pt_regs *regs = tsk->thread.regs;
> -	unsigned long msr = tsk->thread.regs->msr;
>  	long err = 0;
>  
>  	BUG_ON(tsk != current);
>  
> -	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
> +	BUG_ON(!MSR_TM_ACTIVE(msr));
>  
>  	WARN_ON(tm_suspend_disabled);
>  
> @@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>  	 */
>  	msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
>  
> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
> -	 * just indicates to userland that we were doing a transaction, but we
> -	 * don't want to return in transactional state.  This also ensures
> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
> -	 */
> -	regs->msr &= ~MSR_TS_MASK;
> -
>  #ifdef CONFIG_ALTIVEC
>  	err |= __put_user(v_regs, &sc->v_regs);
>  	err |= __put_user(tm_v_regs, &tm_sc->v_regs);
> @@ -824,6 +817,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	unsigned long newsp = 0;
>  	long err = 0;
>  	struct pt_regs *regs = tsk->thread.regs;
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
> +#endif
>  
>  	BUG_ON(tsk != current);
>  
> @@ -841,7 +838,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	err |= __put_user(0, &frame->uc.uc_flags);
>  	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> -	if (MSR_TM_ACTIVE(regs->msr)) {
> +	if (MSR_TM_ACTIVE(msr)) {
>  		/* The ucontext_t passed to userland points to the second
>  		 * ucontext_t (for transactional state) with its uc_link ptr.
>  		 */
> @@ -849,7 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
>  					    &frame->uc_transact.uc_mcontext,
>  					    tsk, ksig->sig, NULL,
> -					    (unsigned long)ksig->ka.sa.sa_handler);
> +					    (unsigned long)ksig->ka.sa.sa_handler,
> +					    msr);
>  	} else
>  #endif
>  	{

Re: [PATCH v2 2/3] selftests/powerpc: Add tm-signal-pagefault test

[Michael Ellerman]

Gustavo Luiz Duarte <gustavold@linux.ibm.com> writes:
> This test triggers a TM Bad Thing by raising a signal in transactional state
> and forcing a pagefault to happen in kernelspace when the kernel signal
> handling code first touches the user signal stack.
>
> This is inspired by the test tm-signal-context-force-tm but uses userfaultfd to
> make the test deterministic. While this test always triggers the bug in one
> run, I had to execute tm-signal-context-force-tm several times (the test runs
> 5000 times each execution) to trigger the same bug.

Using userfaultfd is a very nice touch. But it's not always enabled,
which leads to eg:

  root@mpe-ubuntu-le:~# /home/michael/tm-signal-pagefault 
  test: tm_signal_pagefault
  tags: git_version:v5.5-9354-gc1e346e7fc44
  userfaultfd() failed: Function not implemented
  failure: tm_signal_pagefault

It would be nice if that resulted in a skip, not a failure.

It looks like it shouldn't be too hard to skip if the userfaultfd call
returns ENOSYS.

cheers

Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context").

The bot has tested the following trees: v5.5.1, v5.4.17, v4.19.101, v4.14.169, v4.9.212, v4.4.212.

v5.5.1: Build OK!
v5.4.17: Build OK!
v4.19.101: Build OK!
v4.14.169: Failed to apply! Possible dependencies:
    1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
    92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")

v4.9.212: Failed to apply! Possible dependencies:
    1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
    92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")

v4.4.212: Failed to apply! Possible dependencies:
    1c200e63d055 ("powerpc/tm: Fix endianness flip on trap")
    92fb8690bd04 ("powerpc/tm: P9 disable transactionally suspended sigcontexts")
    a7d623d4d053 ("powerpc: Move part of giveup_vsx into c")
    b86fd2bd0302 ("powerpc: Simplify TM restore checks")
    d11994314b2b ("powerpc: signals: Stop using current in signal code")
    d96f234f47af ("powerpc: Avoid load hit store in setup_sigcontext()")
    e1c0d66fcb17 ("powerpc: Set used_(vsr|vr|spe) in sigreturn path when MSR bits are active")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

-- 
Thanks,
Sasha

Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Gustavo Luiz Duarte]

On 2/5/20 1:58 AM, Michael Neuling wrote:
> Other than the minor things below that I think you need, the patch good with me.
> 
> Acked-by: Michael Neuling <mikey@neuling.org>
> 
>> Subject: Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
> 
> The subject should mention "signals".

How about "powerpc/tm: Clear the current thread's MSR[TS] when 
transaction is reclaimed on signal delivery"  ?

> 
> On Mon, 2020-02-03 at 13:09 -0300, Gustavo Luiz Duarte wrote:
>> After a treclaim, we expect to be in non-transactional state. If we don't
>> immediately clear the current thread's MSR[TS] and we get preempted, then
>> tm_recheckpoint_new_task() will recheckpoint and we get rescheduled in
>> suspended transaction state.
> 
> It's not "immediately", it's before re-enabling preemption.
> 
> There is a similar comment in the code that needs to be fixed too.

OK.

> 
>> When handling a signal caught in transactional state, handle_rt_signal64()
>> calls get_tm_stackpointer() that treclaims the transaction using
>> tm_reclaim_current() but without clearing the thread's MSR[TS]. This can cause
>> the TM Bad Thing exception below if later we pagefault and get preempted trying
>> to access the user's sigframe, using __put_user(). Afterwards, when we are
>> rescheduled back into do_page_fault() (but now in suspended state since the
>> thread's MSR[TS] was not cleared), upon executing 'rfid' after completion of
>> the page fault handling, the exception is raised because a transition from
>> suspended to non-transactional state is invalid.
>>
>> 	Unexpected TM Bad Thing exception at c00000000000de44 (msr 0x8000000302a03031) tm_scratch=800000010280b033
>> 	Oops: Unrecoverable exception, sig: 6 [#1]
>> 	LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
>> 	Modules linked in: nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6_tables ip_tables nft_compat ip_set nf_tables nfnetlink xts vmx_crypto sg virtio_balloon
>> 	r_mod cdrom virtio_net net_failover virtio_blk virtio_scsi failover dm_mirror dm_region_hash dm_log dm_mod
>> 	CPU: 25 PID: 15547 Comm: a.out Not tainted 5.4.0-rc2 #32
>> 	NIP:  c00000000000de44 LR: c000000000034728 CTR: 0000000000000000
>> 	REGS: c00000003fe7bd70 TRAP: 0700   Not tainted  (5.4.0-rc2)
>> 	MSR:  8000000302a03031 <SF,VEC,VSX,FP,ME,IR,DR,LE,TM[SE]>  CR: 44000884  XER: 00000000
>> 	CFAR: c00000000000dda4 IRQMASK: 0
>> 	PACATMSCRATCH: 800000010280b033
>> 	GPR00: c000000000034728 c000000f65a17c80 c000000001662800 00007fffacf3fd78
>> 	GPR04: 0000000000001000 0000000000001000 0000000000000000 c000000f611f8af0
>> 	GPR08: 0000000000000000 0000000078006001 0000000000000000 000c000000000000
>> 	GPR12: c000000f611f84b0 c00000003ffcb200 0000000000000000 0000000000000000
>> 	GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
>> 	GPR20: 0000000000000000 0000000000000000 0000000000000000 c000000f611f8140
>> 	GPR24: 0000000000000000 00007fffacf3fd68 c000000f65a17d90 c000000f611f7800
>> 	GPR28: c000000f65a17e90 c000000f65a17e90 c000000001685e18 00007fffacf3f000
>> 	NIP [c00000000000de44] fast_exception_return+0xf4/0x1b0
>> 	LR [c000000000034728] handle_rt_signal64+0x78/0xc50
>> 	Call Trace:
>> 	[c000000f65a17c80] [c000000000034710] handle_rt_signal64+0x60/0xc50 (unreliable)
>> 	[c000000f65a17d30] [c000000000023640] do_notify_resume+0x330/0x460
>> 	[c000000f65a17e20] [c00000000000dcc4] ret_from_except_lite+0x70/0x74
>> 	Instruction dump:
>> 	7c4ff120 e8410170 7c5a03a6 38400000 f8410060 e8010070 e8410080 e8610088
>> 	60000000 60000000 e8810090 e8210078 <4c000024> 48000000 e8610178 88ed0989
>> 	---[ end trace 93094aa44b442f87 ]---
>>
>> The simplified sequence of events that triggers the above exception is:
>>
>>    ...				# userspace in NON-TRANSACTIONAL state
>>    tbegin			# userspace in TRANSACTIONAL state
>>    signal delivery		# kernelspace in SUSPENDED state
>>    handle_rt_signal64()
>>      get_tm_stackpointer()
>>        treclaim			# kernelspace in NON-TRANSACTIONAL state
>>      __put_user()
>>        page fault happens. We will never get back here because of the TM Bad Thing exception.
>>
>>    page fault handling kicks in and we voluntarily preempt ourselves
>>    do_page_fault()
>>      __schedule()
>>        __switch_to(other_task)
>>
>>    our task is rescheduled and we recheckpoint because the thread's MSR[TS] was not cleared
>>    __switch_to(our_task)
>>      switch_to_tm()
>>        tm_recheckpoint_new_task()
>>          trechkpt			# kernelspace in SUSPENDED state
>>
>>    The page fault handling resumes, but now we are in suspended transaction state
>>    do_page_fault()    completes
>>    rfid     <----- trying to get back where the page fault happened (we were non-transactional back then)
>>    TM Bad Thing			# illegal transition from suspended to non-transactional
>>
>> This patch fixes that issue by clearing the current thread's MSR[TS] just after
>> treclaim in get_tm_stackpointer() so that we stay in non-transactional state in
>> case we are preempted. In order to make treclaim and clearing the thread's
>> MSR[TS] atomic from a preemption perspective when CONFIG_PREEMPT is set,
>> preempt_disable/enable() is used. It's also necessary to save the previous
>> value of the thread's MSR before get_tm_stackpointer() is called so that it can
>> be exposed to the signal handler later in setup_tm_sigcontexts() to inform the
>> userspace MSR at the moment of the signal delivery.
>>
>> Found with tm-signal-context-force-tm kernel selftest on P8 KVM.
> 
> Why are you mentioning KVM?

That is just what I used... I agree that the issue has nothing to do 
with KVM. I will remove that on v3.

> 
>>
>> v2: Fix build failure when tm is disabled.
>>
>> Fixes: 2b0a576d15e0 ("powerpc: Add new transactional memory state to the signal context")
>> Cc: stable@vger.kernel.org # v3.9
>> Signed-off-by: Gustavo Luiz Duarte <gustavold@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/signal.c    | 17 +++++++++++++++--
>>   arch/powerpc/kernel/signal_32.c | 28 ++++++++++++++--------------
>>   arch/powerpc/kernel/signal_64.c | 22 ++++++++++------------
>>   3 files changed, 39 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
>> index e6c30cee6abf..1660be1061ac 100644
>> --- a/arch/powerpc/kernel/signal.c
>> +++ b/arch/powerpc/kernel/signal.c
>> @@ -200,14 +200,27 @@ unsigned long get_tm_stackpointer(struct task_struct *tsk)
>>   	 * normal/non-checkpointed stack pointer.
>>   	 */
>>   
>> +	unsigned long ret = tsk->thread.regs->gpr[1];
>> +
>>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>   	BUG_ON(tsk != current);
>>   
>>   	if (MSR_TM_ACTIVE(tsk->thread.regs->msr)) {
>> +		preempt_disable();
>>   		tm_reclaim_current(TM_CAUSE_SIGNAL);
>>   		if (MSR_TM_TRANSACTIONAL(tsk->thread.regs->msr))
>> -			return tsk->thread.ckpt_regs.gpr[1];
>> +			ret = tsk->thread.ckpt_regs.gpr[1];
>> +
>> +		/* If we treclaim, we must immediately clear the current
>> +		 * thread's TM bits. Otherwise we might be preempted and have
>> +		 * the live MSR[TS] changed behind our back
>> +		 * (tm_recheckpoint_new_task() would recheckpoint).
>> +		 * Besides, we enter the signal handler in non-transactional
>> +		 * state.
>> +		 */
>> +		tsk->thread.regs->msr &= ~MSR_TS_MASK;
>> +		preempt_enable();
>>   	}
>>   #endif
>> -	return tsk->thread.regs->gpr[1];
>> +	return ret;
>>   }
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index 98600b276f76..1b090a76b444 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -489,19 +489,11 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>>    */
>>   static int save_tm_user_regs(struct pt_regs *regs,
>>   			     struct mcontext __user *frame,
>> -			     struct mcontext __user *tm_frame, int sigret)
>> +			     struct mcontext __user *tm_frame, int sigret,
>> +			     unsigned long msr)
>>   {
>> -	unsigned long msr = regs->msr;
>> -
>>   	WARN_ON(tm_suspend_disabled);
>>   
>> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
>> -	 * just indicates to userland that we were doing a transaction, but we
>> -	 * don't want to return in transactional state.  This also ensures
>> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
>> -	 */
>> -	regs->msr &= ~MSR_TS_MASK;
>> -
>>   	/* Save both sets of general registers */
>>   	if (save_general_regs(&current->thread.ckpt_regs, frame)
>>   	    || save_general_regs(regs, tm_frame))
>> @@ -912,6 +904,10 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>>   	int sigret;
>>   	unsigned long tramp;
>>   	struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
>> +	unsigned long msr = regs->msr;
>> +#endif
>>   
>>   	BUG_ON(tsk != current);
>>   
>> @@ -944,13 +940,13 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>>   
>>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>   	tm_frame = &rt_sf->uc_transact.uc_mcontext;
>> -	if (MSR_TM_ACTIVE(regs->msr)) {
>> +	if (MSR_TM_ACTIVE(msr)) {
>>   		if (__put_user((unsigned long)&rt_sf->uc_transact,
>>   			       &rt_sf->uc.uc_link) ||
>>   		    __put_user((unsigned long)tm_frame,
>>   			       &rt_sf->uc_transact.uc_regs))
>>   			goto badframe;
>> -		if (save_tm_user_regs(regs, frame, tm_frame, sigret))
>> +		if (save_tm_user_regs(regs, frame, tm_frame, sigret, msr))
>>   			goto badframe;
>>   	}
>>   	else
>> @@ -1369,6 +1365,10 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>>   	int sigret;
>>   	unsigned long tramp;
>>   	struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
>> +	unsigned long msr = regs->msr;
>> +#endif
>>   
>>   	BUG_ON(tsk != current);
>>   
>> @@ -1402,9 +1402,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>>   
>>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>   	tm_mctx = &frame->mctx_transact;
>> -	if (MSR_TM_ACTIVE(regs->msr)) {
>> +	if (MSR_TM_ACTIVE(msr)) {
>>   		if (save_tm_user_regs(regs, &frame->mctx, &frame->mctx_transact,
>> -				      sigret))
>> +				      sigret, msr))
>>   			goto badframe;
>>   	}
>>   	else
>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> index 117515564ec7..84ed2e77ef9c 100644
>> --- a/arch/powerpc/kernel/signal_64.c
>> +++ b/arch/powerpc/kernel/signal_64.c
>> @@ -192,7 +192,8 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>>   static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>>   				 struct sigcontext __user *tm_sc,
>>   				 struct task_struct *tsk,
>> -				 int signr, sigset_t *set, unsigned long handler)
>> +				 int signr, sigset_t *set, unsigned long handler,
>> +				 unsigned long msr)
>>   {
>>   	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
>>   	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
>> @@ -207,12 +208,11 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>>   	elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
>>   #endif
>>   	struct pt_regs *regs = tsk->thread.regs;
>> -	unsigned long msr = tsk->thread.regs->msr;
>>   	long err = 0;
>>   
>>   	BUG_ON(tsk != current);
>>   
>> -	BUG_ON(!MSR_TM_ACTIVE(regs->msr));
>> +	BUG_ON(!MSR_TM_ACTIVE(msr));
>>   
>>   	WARN_ON(tm_suspend_disabled);
>>   
>> @@ -222,13 +222,6 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>>   	 */
>>   	msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
>>   
>> -	/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
>> -	 * just indicates to userland that we were doing a transaction, but we
>> -	 * don't want to return in transactional state.  This also ensures
>> -	 * that flush_fp_to_thread won't set TIF_RESTORE_TM again.
>> -	 */
>> -	regs->msr &= ~MSR_TS_MASK;
>> -
>>   #ifdef CONFIG_ALTIVEC
>>   	err |= __put_user(v_regs, &sc->v_regs);
>>   	err |= __put_user(tm_v_regs, &tm_sc->v_regs);
>> @@ -824,6 +817,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>   	unsigned long newsp = 0;
>>   	long err = 0;
>>   	struct pt_regs *regs = tsk->thread.regs;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
>> +	unsigned long msr = regs->msr;
>> +#endif
>>   
>>   	BUG_ON(tsk != current);
>>   
>> @@ -841,7 +838,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>   	err |= __put_user(0, &frame->uc.uc_flags);
>>   	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
>>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> -	if (MSR_TM_ACTIVE(regs->msr)) {
>> +	if (MSR_TM_ACTIVE(msr)) {
>>   		/* The ucontext_t passed to userland points to the second
>>   		 * ucontext_t (for transactional state) with its uc_link ptr.
>>   		 */
>> @@ -849,7 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>   		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
>>   					    &frame->uc_transact.uc_mcontext,
>>   					    tsk, ksig->sig, NULL,
>> -					    (unsigned long)ksig->ka.sa.sa_handler);
>> +					    (unsigned long)ksig->ka.sa.sa_handler,
>> +					    msr);
>>   	} else
>>   #endif
>>   	{
> 

Re: [PATCH v2 2/3] selftests/powerpc: Add tm-signal-pagefault test

[Gustavo Luiz Duarte]

On 2/5/20 2:27 AM, Michael Ellerman wrote:
> Gustavo Luiz Duarte <gustavold@linux.ibm.com> writes:
>> This test triggers a TM Bad Thing by raising a signal in transactional state
>> and forcing a pagefault to happen in kernelspace when the kernel signal
>> handling code first touches the user signal stack.
>>
>> This is inspired by the test tm-signal-context-force-tm but uses userfaultfd to
>> make the test deterministic. While this test always triggers the bug in one
>> run, I had to execute tm-signal-context-force-tm several times (the test runs
>> 5000 times each execution) to trigger the same bug.
> 
> Using userfaultfd is a very nice touch. But it's not always enabled,
> which leads to eg:
> 
>    root@mpe-ubuntu-le:~# /home/michael/tm-signal-pagefault
>    test: tm_signal_pagefault
>    tags: git_version:v5.5-9354-gc1e346e7fc44
>    userfaultfd() failed: Function not implemented
>    failure: tm_signal_pagefault
> 
> It would be nice if that resulted in a skip, not a failure.
> 
> It looks like it shouldn't be too hard to skip if the userfaultfd call
> returns ENOSYS.

Good point. I will fix that on v3.

> 
> cheers
> 

Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Michael Neuling]

On Thu, 2020-02-06 at 19:13 -0300, Gustavo Luiz Duarte wrote:
> 
> On 2/5/20 1:58 AM, Michael Neuling wrote:
> > Other than the minor things below that I think you need, the patch good with
> > me.
> > 
> > Acked-by: Michael Neuling <mikey@neuling.org>
> > 
> > > Subject: Re: [PATCH v2 1/3] powerpc/tm: Clear the current thread's MSR[TS]
> > > after treclaim
> > 
> > The subject should mention "signals".
> 
> How about "powerpc/tm: Clear the current thread's MSR[TS] when 
> transaction is reclaimed on signal delivery"  ?

mpe also likes the word "fix" in the subject if it's a fix. So maybe:

 powerpc/tm: Fix clearing MSR[TS] in current when reclaiming on signal delivery

Thanks,
Mikey