* [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test
2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
@ 2020-01-16 22:05 ` Gustavo Luiz Duarte
2020-01-17 20:59 ` Gustavo Romero
2020-01-16 22:05 ` [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test Gustavo Luiz Duarte
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Gustavo Luiz Duarte @ 2020-01-16 22:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, Gustavo Luiz Duarte, gromero
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test
2020-01-16 22:05 ` [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test Gustavo Luiz Duarte
@ 2020-01-17 20:59 ` Gustavo Romero
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo Romero @ 2020-01-17 20:59 UTC (permalink / raw)
To: Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, gromero
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test
2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
2020-01-16 22:05 ` [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test Gustavo Luiz Duarte
@ 2020-01-16 22:05 ` Gustavo Luiz Duarte
2020-01-17 21:00 ` Gustavo Romero
2020-01-17 20:57 ` [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Romero
2020-01-21 14:30 ` kbuild test robot
3 siblings, 1 reply; 7+ messages in thread
From: Gustavo Luiz Duarte @ 2020-01-16 22:05 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, Gustavo Luiz Duarte, gromero
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test
2020-01-16 22:05 ` [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test Gustavo Luiz Duarte
@ 2020-01-17 21:00 ` Gustavo Romero
0 siblings, 0 replies; 7+ messages in thread
From: Gustavo Romero @ 2020-01-17 21:00 UTC (permalink / raw)
To: Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, gromero
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
2020-01-16 22:05 ` [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test Gustavo Luiz Duarte
2020-01-16 22:05 ` [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test Gustavo Luiz Duarte
@ 2020-01-17 20:57 ` Gustavo Romero
2020-01-21 14:30 ` kbuild test robot
3 siblings, 0 replies; 7+ messages in thread
From: Gustavo Romero @ 2020-01-17 20:57 UTC (permalink / raw)
To: Gustavo Luiz Duarte, linuxppc-dev; +Cc: mikey, stable, gromero
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(¤t->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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim
2020-01-16 22:05 [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Luiz Duarte
` (2 preceding siblings ...)
2020-01-17 20:57 ` [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim Gustavo Romero
@ 2020-01-21 14:30 ` kbuild test robot
3 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2020-01-21 14:30 UTC (permalink / raw)
To: Gustavo Luiz Duarte
Cc: mikey, kbuild-all, gromero, Gustavo Luiz Duarte, stable,
linuxppc-dev
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread