This patch series suggests fixes for several corner cases in the RISC-V vector ptrace implementation:
- follow gdbserver expectations and return ENODATA instead of EINVAL if vector extension is supported but not yet activated for a traced process
- force vector context save on the next context switch after ptrace call that modified vector CSRs, to avoid reading stale values by the next ptrace calls
- force vector context save on the first context switch after vector context initialization, to avoid reading zero vlenb by an early attached debugger
For detailed description see the appropriate commit messages. A new test is added into the tools/testing/selftests/riscv/vector to verify the fixes. Each fix is accompanied by its own test case.
Initial version [1] of this series included only the last fix for zero vlenb.
[1] https://lore.kernel.org/linux-riscv/20250821173957.563472-1-geomatsi@gmail.c...
Ilya Mamay (1): riscv: ptrace: return ENODATA for inactive vector extension
Sergey Matyukevich (5): selftests: riscv: test ptrace vector interface selftests: riscv: set invalid vtype using ptrace riscv: vector: allow to force vector context save selftests: riscv: verify initial vector state with ptrace riscv: vector: initialize vlenb on the first context switch
arch/riscv/include/asm/thread_info.h | 2 + arch/riscv/include/asm/vector.h | 3 + arch/riscv/kernel/process.c | 2 + arch/riscv/kernel/ptrace.c | 15 +- arch/riscv/kernel/vector.c | 4 + .../testing/selftests/riscv/vector/.gitignore | 1 + tools/testing/selftests/riscv/vector/Makefile | 5 +- .../testing/selftests/riscv/vector/v_ptrace.c | 302 ++++++++++++++++++ 8 files changed, 331 insertions(+), 3 deletions(-) create mode 100644 tools/testing/selftests/riscv/vector/v_ptrace.c
base-commit: c746c3b5169831d7fb032a1051d8b45592ae8d78
Add a test case to check ptrace behavior in the case when vector extension is supported by the system, but vector context is not yet enabled for the traced process.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/.gitignore | 1 + tools/testing/selftests/riscv/vector/Makefile | 5 +- .../testing/selftests/riscv/vector/v_ptrace.c | 87 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/riscv/vector/v_ptrace.c
diff --git a/tools/testing/selftests/riscv/vector/.gitignore b/tools/testing/selftests/riscv/vector/.gitignore index 7d9c87cd0649..d21c03c3ee0e 100644 --- a/tools/testing/selftests/riscv/vector/.gitignore +++ b/tools/testing/selftests/riscv/vector/.gitignore @@ -2,3 +2,4 @@ vstate_exec_nolibc vstate_prctl v_initval v_exec_initval_nolibc +v_ptrace diff --git a/tools/testing/selftests/riscv/vector/Makefile b/tools/testing/selftests/riscv/vector/Makefile index 6f7497f4e7b3..c14ad127e7fb 100644 --- a/tools/testing/selftests/riscv/vector/Makefile +++ b/tools/testing/selftests/riscv/vector/Makefile @@ -2,7 +2,7 @@ # Copyright (C) 2021 ARM Limited # Originally tools/testing/arm64/abi/Makefile
-TEST_GEN_PROGS := v_initval vstate_prctl +TEST_GEN_PROGS := v_initval vstate_prctl v_ptrace TEST_GEN_PROGS_EXTENDED := vstate_exec_nolibc v_exec_initval_nolibc
include ../../lib.mk @@ -26,3 +26,6 @@ $(OUTPUT)/v_initval: v_initval.c $(OUTPUT)/sys_hwprobe.o $(OUTPUT)/v_helpers.o $(OUTPUT)/v_exec_initval_nolibc: v_exec_initval_nolibc.c $(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \ -Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc + +$(OUTPUT)/v_ptrace: v_ptrace.c $(OUTPUT)/sys_hwprobe.o $(OUTPUT)/v_helpers.o + $(CC) -static -o$@ $(CFLAGS) $(LDFLAGS) $^ diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c new file mode 100644 index 000000000000..6a8d56a5c4f4 --- /dev/null +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <sys/ptrace.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/wait.h> +#include <sys/uio.h> +#include <unistd.h> +#include <errno.h> + +#include <linux/ptrace.h> +#include <linux/elf.h> + +#include "../../kselftest_harness.h" +#include "v_helpers.h" + +volatile unsigned long chld_lock; + +TEST(ptrace_rvv_not_enabled) +{ + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm volatile ("ebreak" : : : ); + } else { + struct __riscv_v_regset_state *regset_data; + unsigned long vlenb; + size_t regset_size; + struct iovec iov; + int status; + int ret; + + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + ASSERT_GT(vlenb, 0) + TH_LOG("vlenb is not valid: %lu\n", vlenb); + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* try to read vector registers from the tracee */ + + regset_size = sizeof(*regset_data) + vlenb * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + /* V extension is available, but not yet enabled for the tracee */ + + errno = 0; + ret = ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov); + ASSERT_EQ(ENODATA, errno); + ASSERT_EQ(-1, ret); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + +TEST_HARNESS_MAIN
From: Ilya Mamay mmamayka01@gmail.com
Currently, ptrace returns EINVAL when the vector extension is supported but not yet activated for the traced process. This error code is inappropriate since all the ptrace arguments are valid.
Debug tools like gdbserver expect ENODATA when the requested register set is not active, e.g. see [1]. This expectation seems to be more appropriate, so modify the vector ptrace implementation to return: - EINVAL when V extension is not supported - ENODATA when V extension is supported but not active
[1] https://github.com/bminor/binutils-gdb/blob/637f25e88675fa47e47f9cc5e2cf3738...
Signed-off-by: Ilya Mamay mmamayka01@gmail.com --- arch/riscv/kernel/ptrace.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 8e86305831ea..906cf1197edc 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -95,9 +95,12 @@ static int riscv_vr_get(struct task_struct *target, struct __riscv_v_ext_state *vstate = &target->thread.vstate; struct __riscv_v_regset_state ptrace_vstate;
- if (!riscv_v_vstate_query(task_pt_regs(target))) + if (!has_vector()) return -EINVAL;
+ if (!riscv_v_vstate_query(task_pt_regs(target))) + return -ENODATA; + /* * Ensure the vector registers have been saved to the memory before * copying them to membuf. @@ -130,9 +133,12 @@ static int riscv_vr_set(struct task_struct *target, struct __riscv_v_ext_state *vstate = &target->thread.vstate; struct __riscv_v_regset_state ptrace_vstate;
- if (!riscv_v_vstate_query(task_pt_regs(target))) + if (!has_vector()) return -EINVAL;
+ if (!riscv_v_vstate_query(task_pt_regs(target))) + return -ENODATA; + /* Copy rest of the vstate except datap */ ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &ptrace_vstate, 0, sizeof(struct __riscv_v_regset_state));
Add a test case that attempts to set invalid vtype value using ptrace and verifies that the 'vill' bit is set as required by the RISC-V Vector specification v1.0, Section 3.4.1.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/v_ptrace.c | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+)
diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c index 6a8d56a5c4f4..ccda8a4dc49b 100644 --- a/tools/testing/selftests/riscv/vector/v_ptrace.c +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -84,4 +84,116 @@ TEST(ptrace_rvv_not_enabled) } }
+TEST(ptrace_rvv_invalid_vtype) +{ + static volatile unsigned long vtype; + unsigned long vlenb; + unsigned long reg; + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + if (vlenb > 16) + SKIP(return, "This test does not support VLEN > 128"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm(".option arch, +v\n"); + asm(".option arch, +c\n"); + asm volatile("vsetvli x0, x0, e8, m8, tu, mu\n"); + + while (1) { + asm volatile ("c.ebreak"); + asm volatile("csrr %[vtype], vtype" : [vtype] "=r"(vtype) : :); + asm volatile ("c.ebreak"); + } + } else { + struct __riscv_v_regset_state *regset_data; + struct user_regs_struct regs; + size_t regset_size; + struct iovec iov; + int status; + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for the 1st c.ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vector csr regs using ptrace GETREGSET */ + + regset_size = sizeof(*regset_data) + vlenb * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* set invalid vtype 0x1d = (5 | 3 << 3): + * - LMUL: 1/8 + * - SEW: 64 + * - invalid configuration for VLENB <= 128 + */ + regset_data->vtype = 0x1d; + ASSERT_EQ(0, ptrace(PTRACE_SETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* skip 1st c.ebreak, then resume and wait for the 2nd c.ebreak */ + + iov.iov_base = ®s; + iov.iov_len = sizeof(regs); + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, &iov)); + regs.pc += 2; + ASSERT_EQ(0, ptrace(PTRACE_SETREGSET, pid, NT_PRSTATUS, &iov)); + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vtype using ptrace GETREGSET */ + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* read tracee vtype ptrace PEEKDATA */ + + errno = 0; + reg = ptrace(PTRACE_PEEKDATA, pid, &vtype, NULL); + ASSERT_FALSE((errno != 0) && (reg == -1)); + + /* verify that V state is illegal */ + + EXPECT_EQ(reg, regset_data->vtype); + EXPECT_EQ(1UL, (regset_data->vtype >> (__riscv_xlen - 1))); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + TEST_HARNESS_MAIN
When ptrace updates vector CSR registers for a traced process, the changes may not be immediately visible to the next ptrace operations due to vector context switch optimizations.
The function 'riscv_v_vstate_save' saves context only if mstatus.VS is 'dirty'. However mstatus.VS of the traced process context may remain 'clean' between two breakpoints, if no vector instructions were executed between those two breakpoints. In this case the vector context will not be saved at the second breakpoint. As a result, the second ptrace may read stale vector CSR values.
Fix this by introducing a TIF flag that forces vector context save on the next context switch, regardless of mstatus.VS state. Set this flag on ptrace oprations that modify vector CSR registers.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- arch/riscv/include/asm/thread_info.h | 2 ++ arch/riscv/include/asm/vector.h | 3 +++ arch/riscv/kernel/process.c | 2 ++ arch/riscv/kernel/ptrace.c | 5 +++++ 4 files changed, 12 insertions(+)
diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h index 836d80dd2921..e05e9aa89c43 100644 --- a/arch/riscv/include/asm/thread_info.h +++ b/arch/riscv/include/asm/thread_info.h @@ -118,7 +118,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
#define TIF_32BIT 16 /* compat-mode 32bit process */ #define TIF_RISCV_V_DEFER_RESTORE 17 /* restore Vector before returing to user */ +#define TIF_RISCV_V_FORCE_SAVE 13 /* force Vector context save */
#define _TIF_RISCV_V_DEFER_RESTORE BIT(TIF_RISCV_V_DEFER_RESTORE) +#define _TIF_RISCV_V_FORCE_SAVE BIT(TIF_RISCV_V_FORCE_SAVE)
#endif /* _ASM_RISCV_THREAD_INFO_H */ diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h index b61786d43c20..d3770e13da93 100644 --- a/arch/riscv/include/asm/vector.h +++ b/arch/riscv/include/asm/vector.h @@ -370,6 +370,9 @@ static inline void __switch_to_vector(struct task_struct *prev, { struct pt_regs *regs;
+ if (test_and_clear_tsk_thread_flag(prev, TIF_RISCV_V_FORCE_SAVE)) + __riscv_v_vstate_dirty(task_pt_regs(prev)); + if (riscv_preempt_v_started(prev)) { if (riscv_v_is_on()) { WARN_ON(prev->thread.riscv_v_flags & RISCV_V_CTX_DEPTH_MASK); diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 31a392993cb4..47959c55cefb 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -183,6 +183,7 @@ void flush_thread(void) kfree(current->thread.vstate.datap); memset(¤t->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(current, TIF_RISCV_V_DEFER_RESTORE); + clear_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE); #endif #ifdef CONFIG_RISCV_ISA_SUPM if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM)) @@ -205,6 +206,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) memset(&dst->thread.vstate, 0, sizeof(struct __riscv_v_ext_state)); memset(&dst->thread.kernel_vstate, 0, sizeof(struct __riscv_v_ext_state)); clear_tsk_thread_flag(dst, TIF_RISCV_V_DEFER_RESTORE); + clear_tsk_thread_flag(dst, TIF_RISCV_V_FORCE_SAVE);
return 0; } diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c index 906cf1197edc..569f756bef23 100644 --- a/arch/riscv/kernel/ptrace.c +++ b/arch/riscv/kernel/ptrace.c @@ -148,6 +148,11 @@ static int riscv_vr_set(struct task_struct *target, if (vstate->vlenb != ptrace_vstate.vlenb) return -EINVAL;
+ if (vstate->vtype != ptrace_vstate.vtype || + vstate->vcsr != ptrace_vstate.vcsr || + vstate->vl != ptrace_vstate.vl) + set_tsk_thread_flag(target, TIF_RISCV_V_FORCE_SAVE); + vstate->vstart = ptrace_vstate.vstart; vstate->vl = ptrace_vstate.vl; vstate->vtype = ptrace_vstate.vtype;
Add a test case that attaches to a traced process immediately after its first vector instructions to verify the initial vector context state.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- .../testing/selftests/riscv/vector/v_ptrace.c | 103 ++++++++++++++++++ 1 file changed, 103 insertions(+)
diff --git a/tools/testing/selftests/riscv/vector/v_ptrace.c b/tools/testing/selftests/riscv/vector/v_ptrace.c index ccda8a4dc49b..f452e04629ea 100644 --- a/tools/testing/selftests/riscv/vector/v_ptrace.c +++ b/tools/testing/selftests/riscv/vector/v_ptrace.c @@ -196,4 +196,107 @@ TEST(ptrace_rvv_invalid_vtype) } }
+TEST(ptrace_rvv_early_access) +{ + static volatile unsigned long vstart; + static volatile unsigned long vtype; + static volatile unsigned long vlenb; + static volatile unsigned long vcsr; + static volatile unsigned long vl; + pid_t pid; + + if (!is_vector_supported()) + SKIP(return, "Vector not supported"); + + chld_lock = 1; + + pid = fork(); + + ASSERT_LE(0, pid) + TH_LOG("fork: %m"); + + if (pid == 0) { + while (chld_lock == 1) + asm volatile("" : : "g"(chld_lock) : "memory"); + + asm volatile("csrr %[vstart], vstart" : [vstart] "=r"(vstart)); + asm volatile("csrr %[vl], vl" : [vl] "=r"(vl)); + asm volatile("csrr %[vtype], vtype" : [vtype] "=r"(vtype)); + asm volatile("csrr %[vcsr], vcsr" : [vcsr] "=r"(vcsr)); + asm volatile("csrr %[vlenb], vlenb" : [vlenb] "=r"(vlenb)); + + asm volatile ("ebreak" : : : ); + } else { + struct __riscv_v_regset_state *regset_data; + unsigned long vstart_csr; + unsigned long vl_csr; + unsigned long vtype_csr; + unsigned long vcsr_csr; + unsigned long vlenb_csr; + size_t regset_size; + struct iovec iov; + int status; + + /* attach */ + + ASSERT_EQ(0, ptrace(PTRACE_ATTACH, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* unlock */ + + ASSERT_EQ(0, ptrace(PTRACE_POKEDATA, pid, &chld_lock, 0)); + + /* resume and wait for ebreak */ + + ASSERT_EQ(0, ptrace(PTRACE_CONT, pid, NULL, NULL)); + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFSTOPPED(status)); + + /* read tracee vector csr regs using ptrace PEEKDATA */ + + errno = 0; + vstart_csr = ptrace(PTRACE_PEEKDATA, pid, &vstart, NULL); + ASSERT_FALSE((errno != 0) && (vstart_csr == -1)); + + errno = 0; + vl_csr = ptrace(PTRACE_PEEKDATA, pid, &vl, NULL); + ASSERT_FALSE((errno != 0) && (vl_csr == -1)); + + errno = 0; + vtype_csr = ptrace(PTRACE_PEEKDATA, pid, &vtype, NULL); + ASSERT_FALSE((errno != 0) && (vtype_csr == -1)); + + errno = 0; + vcsr_csr = ptrace(PTRACE_PEEKDATA, pid, &vcsr, NULL); + ASSERT_FALSE((errno != 0) && (vcsr_csr == -1)); + + errno = 0; + vlenb_csr = ptrace(PTRACE_PEEKDATA, pid, &vlenb, NULL); + ASSERT_FALSE((errno != 0) && (vlenb_csr == -1)); + + /* read tracee csr regs using ptrace GETREGSET */ + + regset_size = sizeof(*regset_data) + vlenb_csr * 32; + regset_data = calloc(1, regset_size); + + iov.iov_base = regset_data; + iov.iov_len = regset_size; + + ASSERT_EQ(0, ptrace(PTRACE_GETREGSET, pid, NT_RISCV_VECTOR, &iov)); + + /* compare */ + + EXPECT_EQ(vstart_csr, regset_data->vstart); + EXPECT_EQ(vtype_csr, regset_data->vtype); + EXPECT_EQ(vlenb_csr, regset_data->vlenb); + EXPECT_EQ(vcsr_csr, regset_data->vcsr); + EXPECT_EQ(vl_csr, regset_data->vl); + + /* cleanup */ + + ASSERT_EQ(0, kill(pid, SIGKILL)); + } +} + TEST_HARNESS_MAIN
The vstate in thread_struct is zeroed when the vector context is initialized. That includes read-only register vlenb, which holds the vector register length in bytes. This zeroed state persists until mstatus.VS becomes 'dirty' and a context switch saves the actual hardware values.
This can expose the zero vlenb value to the user-space in early debug scenarios, e.g. when ptrace attaches to a traced process early, before any vector instruction except the first one was executed.
Fix this by forcing the vector context save on the first context switch.
Signed-off-by: Sergey Matyukevich geomatsi@gmail.com --- arch/riscv/kernel/vector.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c index 901e67adf576..3dd22a71aa18 100644 --- a/arch/riscv/kernel/vector.c +++ b/arch/riscv/kernel/vector.c @@ -120,6 +120,7 @@ static int riscv_v_thread_zalloc(struct kmem_cache *cache,
ctx->datap = datap; memset(ctx, 0, offsetof(struct __riscv_v_ext_state, datap)); + return 0; }
@@ -216,8 +217,11 @@ bool riscv_v_first_use_handler(struct pt_regs *regs) force_sig(SIGBUS); return true; } + riscv_v_vstate_on(regs); riscv_v_vstate_set_restore(current, regs); + set_tsk_thread_flag(current, TIF_RISCV_V_FORCE_SAVE); + return true; }
linux-kselftest-mirror@lists.linaro.org
-
Sergey Matyukevich