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

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

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 | 24 ++++++++++--------------
 arch/powerpc/kernel/signal_64.c | 20 ++++++++------------
 3 files changed, 33 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..132a092cd170 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,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -944,13 +938,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 +1363,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
 	int sigret;
 	unsigned long tramp;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -1402,9 +1398,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..e5b5f9738056 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,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsigned long newsp = 0;
 	long err = 0;
 	struct pt_regs *regs = tsk->thread.regs;
+	/* Save the thread's msr before get_tm_stackpointer() changes it */
+	unsigned long msr = regs->msr;
 
 	BUG_ON(tsk != current);
 
@@ -841,7 +836,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 +844,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.0

[PATCH 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.0

[PATCH 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.0

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

[Gustavo Romero]

Hi Gustavo,

Thanks for fixing that TM issue.

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> 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 | 24 ++++++++++--------------
>   arch/powerpc/kernel/signal_64.c | 20 ++++++++------------
>   3 files changed, 33 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..132a092cd170 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,8 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
>   	int sigret;
>   	unsigned long tramp;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -944,13 +938,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 +1363,8 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
>   	int sigret;
>   	unsigned long tramp;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -1402,9 +1398,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..e5b5f9738056 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,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	unsigned long newsp = 0;
>   	long err = 0;
>   	struct pt_regs *regs = tsk->thread.regs;
> +	/* Save the thread's msr before get_tm_stackpointer() changes it */
> +	unsigned long msr = regs->msr;
>   
>   	BUG_ON(tsk != current);
>   
> @@ -841,7 +836,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 +844,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
>   	{
> 

The change needs to be improved in case CONFIG_PPC_TRANSACTIONAL_MEM=n, like:

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 132a092cd170..1b090a76b444 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -904,8 +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);
  
@@ -1363,8 +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);
  
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index e5b5f9738056..84ed2e77ef9c 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -817,8 +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);

or -Werror=unused-variable will catch it like:

/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
/linux/arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' [-Werror=unused-variable]
   908 |  unsigned long msr = regs->msr;
       |                ^~~
/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
/linux/arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' [-Werror=unused-variable]
  1367 |  unsigned long msr = regs->msr;
       |


Feel free to send a v2 only after Mikey's review.

Otherwise, LGTM.

Reviewed-by: Gustavo Romero <gromero@linux.ibm.com>

Best regards,
Gustavo

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

[Gustavo Romero]

Hi Gustavo,

Nice catch on using userfaultfd() to make it deterministic.

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> 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");
> +}
> 

Reviewed-by: Gustavo Romero <gromero@linux.ibm.com>


Best regards,
Gustavo

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

[Gustavo Romero]

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:
> 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;
>   }
> 

Reviewed-by: Gustavo Romero <gromero@linux.ibm.com>


Best regards,
Gustavo

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3877 bytes --]

Hi Gustavo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on scottwood/next v5.5-rc7 next-20200120]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Luiz-Duarte/powerpc-tm-Clear-the-current-thread-s-MSR-TS-after-treclaim/20200118-034925
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-a001-20200121 (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 7.5.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.5.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/signal_32.c: In function 'handle_rt_signal32':
>> arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
   arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   cc1: all warnings being treated as errors
--
   arch/powerpc/kernel/signal_64.c: In function 'handle_rt_signal64':
>> arch/powerpc/kernel/signal_64.c:821:16: error: unused variable 'msr' [-Werror=unused-variable]
     unsigned long msr = regs->msr;
                   ^~~
   cc1: all warnings being treated as errors

vim +/msr +908 arch/powerpc/kernel/signal_32.c

   891	
   892	/*
   893	 * Set up a signal frame for a "real-time" signal handler
   894	 * (one which gets siginfo).
   895	 */
   896	int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
   897			       struct task_struct *tsk)
   898	{
   899		struct rt_sigframe __user *rt_sf;
   900		struct mcontext __user *frame;
   901		struct mcontext __user *tm_frame = NULL;
   902		void __user *addr;
   903		unsigned long newsp = 0;
   904		int sigret;
   905		unsigned long tramp;
   906		struct pt_regs *regs = tsk->thread.regs;
   907		/* Save the thread's msr before get_tm_stackpointer() changes it */
 > 908		unsigned long msr = regs->msr;
   909	
   910		BUG_ON(tsk != current);
   911	
   912		/* Set up Signal Frame */
   913		/* Put a Real Time Context onto stack */
   914		rt_sf = get_sigframe(ksig, get_tm_stackpointer(tsk), sizeof(*rt_sf), 1);
   915		addr = rt_sf;
   916		if (unlikely(rt_sf == NULL))
   917			goto badframe;
   918	
   919		/* Put the siginfo & fill in most of the ucontext */
   920		if (copy_siginfo_to_user(&rt_sf->info, &ksig->info)
   921		    || __put_user(0, &rt_sf->uc.uc_flags)
   922		    || __save_altstack(&rt_sf->uc.uc_stack, regs->gpr[1])
   923		    || __put_user(to_user_ptr(&rt_sf->uc.uc_mcontext),
   924			    &rt_sf->uc.uc_regs)
   925		    || put_sigset_t(&rt_sf->uc.uc_sigmask, oldset))
   926			goto badframe;
   927	
   928		/* Save user registers on the stack */
   929		frame = &rt_sf->uc.uc_mcontext;
   930		addr = frame;
   931		if (vdso32_rt_sigtramp && tsk->mm->context.vdso_base) {
   932			sigret = 0;
   933			tramp = tsk->mm->context.vdso_base + vdso32_rt_sigtramp;
   934		} else {
   935			sigret = __NR_rt_sigreturn;
   936			tramp = (unsigned long) frame->tramp;
   937		}
   938	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29883 bytes --]