[PATCH v2 0/2] riscv: signal: fix the FP state restore in rt_sigreturn()

[PATCH v2 0/2] riscv: signal: fix the FP state restore in rt_sigreturn()

[Nickolai Zeldovich]

If rt_sigreturn() is preempted or sleeps on a page fault while it copies
the FP registers from the signal frame, the interrupted code resumes
with the signal handler's FP registers. Patch 1 fixes this, patch 2 adds
a selftest that reproduces it with userfaultfd.

Tested under QEMU with riscv64 defconfig plus CONFIG_USERFAULTFD: the
new test fails 10 of 10 runs on master, with preempt=lazy and
preempt=none, and passes 10 of 10 with patch 1, also on top of Guobin
Zhang's "riscv: signal: protect regs->status RMW from concurrent
preemption" v2 [1]; the two apply in either order.

This series was prepared with the help of an AI tool.

Changes in v2:
- Disable preemption around __fstate_clean() (Andy Chiu)
- Add a selftest (Andy Chiu)
- Shorten the commit message

v1: https://lore.kernel.org/all/20261016160708.790397-1-nickolai@csail.mit.edu/
[1] https://lore.kernel.org/all/20261020-vector_fpu_regs_status_rmw_fix-v2-1-474a59155e96@intel.com/

Nickolai Zeldovich (2):
  riscv: signal: mark the FP state clean before restoring it from the
    frame
  selftests: riscv: test FP state restore when rt_sigreturn() faults

 arch/riscv/kernel/signal.c                    |   7 +
 .../selftests/riscv/sigreturn/.gitignore      |   1 +
 .../selftests/riscv/sigreturn/Makefile        |   5 +-
 .../selftests/riscv/sigreturn/fp_restore.c    | 146 ++++++++++++++++++
 4 files changed, 158 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/riscv/sigreturn/fp_restore.c


base-commit: 36aad21d50d1af3f6954bb0c988257a908202477
-- 
2.53.0

[PATCH v2 1/2] riscv: signal: mark the FP state clean before restoring it from the frame

[Nickolai Zeldovich]

If rt_sigreturn() is preempted or sleeps on a page fault while it copies
the FP registers from the signal frame, the interrupted code resumes
with the signal handler's FP registers instead of its own.

restore_fp_state() copies the frame into current->thread.fstate and then
loads it with fstate_restore(). If the handler used the FPU, FS is still
Dirty during the copy, so a context switch makes fstate_save() store the
live registers, the handler's, over the partly copied image.

Mark the FP state clean before the copy, as commit c27fa53b858b ("riscv:
Fix vector state restore in rt_sigreturn()") did for the vector state.
Preemption is disabled around it because the scheduler also writes
regs->status.

Fixes: e2c0cdfba7f6 ("RISC-V: User-facing API")
Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
 arch/riscv/kernel/signal.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
index 59784dc117e4..11edfc128ac3 100644
--- a/arch/riscv/kernel/signal.c
+++ b/arch/riscv/kernel/signal.c
@@ -47,6 +47,13 @@ static long restore_fp_state(struct pt_regs *regs,
 	long err;
 	struct __riscv_d_ext_state __user *state = &sc_fpregs->d;
 
+	/* Discard the handler's FP registers before the copy can sleep */
+	if ((regs->status & SR_FS) != SR_FS_OFF) {
+		preempt_disable();
+		__fstate_clean(regs);
+		preempt_enable();
+	}
+
 	err = __copy_from_user(&current->thread.fstate, state, sizeof(*state));
 	if (unlikely(err))
 		return err;
-- 
2.53.0

[PATCH v2 2/2] selftests: riscv: test FP state restore when rt_sigreturn() faults

[Nickolai Zeldovich]

Add a test for the previous patch: the signal frame sits on a
userfaultfd page that the handler drops after dirtying the FP
registers, so rt_sigreturn() sleeps in the middle of copying the FP
registers from the frame. The interrupted code must get its own
registers back.

The test is skipped without the D extension or when userfaultfd is not
available to the caller.

Signed-off-by: Nickolai Zeldovich <nickolai@csail.mit.edu>
---
 .../selftests/riscv/sigreturn/.gitignore      |   1 +
 .../selftests/riscv/sigreturn/Makefile        |   5 +-
 .../selftests/riscv/sigreturn/fp_restore.c    | 146 ++++++++++++++++++
 3 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/riscv/sigreturn/fp_restore.c

diff --git a/tools/testing/selftests/riscv/sigreturn/.gitignore b/tools/testing/selftests/riscv/sigreturn/.gitignore
index 35002b8ae780..8ed282ecb618 100644
--- a/tools/testing/selftests/riscv/sigreturn/.gitignore
+++ b/tools/testing/selftests/riscv/sigreturn/.gitignore
@@ -1 +1,2 @@
+fp_restore
 sigreturn
diff --git a/tools/testing/selftests/riscv/sigreturn/Makefile b/tools/testing/selftests/riscv/sigreturn/Makefile
index 8c77508641f3..ac7ae7a7c2ae 100644
--- a/tools/testing/selftests/riscv/sigreturn/Makefile
+++ b/tools/testing/selftests/riscv/sigreturn/Makefile
@@ -4,9 +4,12 @@
 
 CFLAGS += -I$(top_srcdir)/tools/include
 
-TEST_GEN_PROGS := sigreturn
+TEST_GEN_PROGS := sigreturn fp_restore
 
 include ../../lib.mk
 
 $(OUTPUT)/sigreturn: sigreturn.c
 	$(CC) -static -o $@ $(CFLAGS) $(LDFLAGS) $^
+
+$(OUTPUT)/fp_restore: fp_restore.c
+	$(CC) -static -pthread -o $@ $(CFLAGS) $(LDFLAGS) $^
diff --git a/tools/testing/selftests/riscv/sigreturn/fp_restore.c b/tools/testing/selftests/riscv/sigreturn/fp_restore.c
new file mode 100644
index 000000000000..1a56684c5f28
--- /dev/null
+++ b/tools/testing/selftests/riscv/sigreturn/fp_restore.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Check that rt_sigreturn() restores the FP registers from the signal
+ * frame when the copy of the frame sleeps on a page fault while the
+ * handler's FP state is dirty.  The frame is placed on a userfaultfd
+ * page that the handler drops, so the copy faults after f0-f15.
+ */
+#include <fcntl.h>
+#include <pthread.h>
+#include <signal.h>
+#include <stdint.h>
+#include <string.h>
+#include <sys/auxv.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <ucontext.h>
+#include <unistd.h>
+#include <asm/hwcap.h>
+#include <linux/userfaultfd.h>
+#include "kselftest_harness.h"
+
+#define PAGE		4096UL
+#define STACK_SIZE	65536UL
+/* the page boundary falls this far into the FP image, after f0-f15 */
+#define SPLIT		128
+
+#define F0_15	"0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15"
+/* caller-saved only, so the handler's epilogue does not reload them */
+#define HANDLER_REGS	"0, 1, 2, 3, 4, 5, 6, 7, 10, 11, 12, 13, 14, 15"
+
+static uint64_t pattern[16], handler_pattern[16];
+static void *saved;
+static int placed;
+
+static void handler(int sig, siginfo_t *si, void *context)
+{
+	ucontext_t *uc = context;
+	unsigned long split = (unsigned long)&uc->uc_mcontext.__fpregs + SPLIT;
+
+	placed = !(split & (PAGE - 1));
+	if (placed) {
+		/* rt_sigreturn() will fault on this page and wait for serve() */
+		memcpy(saved, (void *)split, PAGE);
+		if (madvise((void *)split, PAGE, MADV_DONTNEED))
+			_exit(1);
+	}
+	asm volatile(".irp i, " HANDLER_REGS "\n"
+		     "fld f\\i, \\i * 8(%[h])\n"
+		     ".endr\n"
+		     : : [h] "r" (handler_pattern)
+		     : "memory", "ft0", "ft1", "ft2", "ft3", "ft4", "ft5",
+		       "ft6", "ft7", "fa0", "fa1", "fa2", "fa3", "fa4", "fa5");
+}
+
+static void *serve(void *arg)
+{
+	struct uffdio_copy copy = { .src = (unsigned long)saved, .len = PAGE };
+	struct uffd_msg msg;
+	int fd = (long)arg;
+
+	while (read(fd, &msg, sizeof(msg)) == sizeof(msg)) {
+		copy.dst = msg.arg.pagefault.address & ~(PAGE - 1);
+		copy.copy = 0;
+		ioctl(fd, UFFDIO_COPY, &copy);
+	}
+	return NULL;
+}
+
+TEST(fp_restore_frame_fault)
+{
+	struct sigaction sa = {
+		.sa_sigaction = handler,
+		.sa_flags = SA_SIGINFO | SA_ONSTACK,
+	};
+	struct uffdio_api api = { .api = UFFD_API };
+	struct uffdio_register reg = { .mode = UFFDIO_REGISTER_MODE_MISSING };
+	stack_t ss = { .ss_size = STACK_SIZE };
+	long pid = getpid(), tid = syscall(__NR_gettid);
+	uint64_t out[16];
+	unsigned long fcsr, shift;
+	pthread_t thread;
+	int fd, i;
+
+	if (!(getauxval(AT_HWCAP) & COMPAT_HWCAP_ISA_D))
+		SKIP(return, "no D extension");
+
+	fd = syscall(__NR_userfaultfd, O_CLOEXEC);
+	if (fd < 0)
+		SKIP(return, "userfaultfd not available");
+	ASSERT_EQ(0, ioctl(fd, UFFDIO_API, &api));
+
+	for (i = 0; i < 16; i++) {
+		pattern[i] = 0x4010000000000000ULL + i;
+		handler_pattern[i] = 0xbff0000000000000ULL + i;
+	}
+
+	saved = mmap(NULL, PAGE, PROT_READ | PROT_WRITE,
+		     MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	ASSERT_NE(MAP_FAILED, saved);
+	ss.ss_sp = mmap(NULL, STACK_SIZE, PROT_READ | PROT_WRITE,
+			MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+	ASSERT_NE(MAP_FAILED, ss.ss_sp);
+	/* populate it, so only the page the handler drops faults */
+	memset(ss.ss_sp, 0, STACK_SIZE);
+	reg.range.start = (unsigned long)ss.ss_sp;
+	reg.range.len = STACK_SIZE;
+	ASSERT_EQ(0, ioctl(fd, UFFDIO_REGISTER, &reg));
+	ASSERT_EQ(0, pthread_create(&thread, NULL, serve, (void *)(long)fd));
+	ASSERT_EQ(0, sigaction(SIGUSR1, &sa, NULL));
+
+	/* lower the frame until the FP image crosses a page at SPLIT */
+	for (shift = 0; shift < PAGE && !placed; shift += 16) {
+		register long a0 asm("a0") = pid;
+		register long a1 asm("a1") = tid;
+		register long a2 asm("a2") = SIGUSR1;
+		register long a7 asm("a7") = __NR_tgkill;
+
+		ss.ss_size = STACK_SIZE - shift;
+		ASSERT_EQ(0, sigaltstack(&ss, NULL));
+		fcsr = 0x1f;
+		asm volatile(".irp i, " F0_15 "\n"
+			     "fld f\\i, \\i * 8(%[in])\n"
+			     ".endr\n"
+			     "csrw fcsr, %[fcsr]\n"
+			     "ecall\n"
+			     "csrr %[fcsr], fcsr\n"
+			     ".irp i, " F0_15 "\n"
+			     "fsd f\\i, \\i * 8(%[out])\n"
+			     ".endr\n"
+			     : "+r" (a0), [fcsr] "+r" (fcsr)
+			     : "r" (a1), "r" (a2), "r" (a7),
+			       [in] "r" (pattern), [out] "r" (out)
+			     : "memory", "ft0", "ft1", "ft2", "ft3", "ft4", "ft5",
+			       "ft6", "ft7", "fs0", "fs1", "fa0", "fa1", "fa2",
+			       "fa3", "fa4", "fa5");
+	}
+	ASSERT_TRUE(placed);
+
+	for (i = 0; i < 16; i++) {
+		EXPECT_EQ(pattern[i], out[i]) TH_LOG("f%d", i);
+	}
+	EXPECT_EQ(0x1fUL, fcsr);
+}
+
+TEST_HARNESS_MAIN
-- 
2.53.0