Messages in this thread | | | From | Andy Chiu <> | Date | Thu, 29 Jun 2023 01:03:19 +0800 | Subject | Re: [PATCH v2] riscv: Discard vector state on syscalls |
| |
On Wed, Jun 28, 2023 at 6:35 PM Björn Töpel <bjorn@kernel.org> wrote: > > Andy Chiu <andy.chiu@sifive.com> writes: > > > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@kernel.org> wrote: > >> > >> From: Björn Töpel <bjorn@rivosinc.com> > >> > >> The RISC-V vector specification states: > >> Executing a system call causes all caller-saved vector registers > >> (v0-v31, vl, vtype) and vstart to become unspecified. > >> > >> The vector registers are set to all 1s, vill is set (invalid), and the > >> vector status is set to Initial. > >> > >> That way we can prevent userspace from accidentally relying on the > >> stated save. > >> > >> Rémi pointed out [1] that writing to the registers might be > >> superfluous, and setting vill is sufficient. > >> > >> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1] > >> Suggested-by: Darius Rad <darius@bluespec.com> > >> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com> > >> Suggested-by: Rémi Denis-Courmont <remi@remlab.net> > >> Signed-off-by: Björn Töpel <bjorn@rivosinc.com> > >> --- > >> v1->v2: > >> Proper register restore for initial state (Andy) > >> Set registers to 1s, and not 0s (Darius) > >> --- > >> arch/riscv/include/asm/vector.h | 42 ++++++++++++++++++++++++++++++--- > >> arch/riscv/kernel/traps.c | 2 ++ > >> 2 files changed, 41 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h > >> index 04c0b07bf6cd..93d702d9988c 100644 > >> --- a/arch/riscv/include/asm/vector.h > >> +++ b/arch/riscv/include/asm/vector.h > >> @@ -139,14 +139,49 @@ static inline void riscv_v_vstate_save(struct task_struct *task, > >> } > >> } > >> > >> +static inline void __riscv_v_vstate_discard(void) > >> +{ > >> + unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1); > >> + > >> + riscv_v_enable(); > >> + asm volatile ( > >> + ".option push\n\t" > >> + ".option arch, +v\n\t" > >> + "vsetvli %0, x0, e8, m8, ta, ma\n\t" > >> + "vmv.v.i v0, -1\n\t" > >> + "vmv.v.i v8, -1\n\t" > >> + "vmv.v.i v16, -1\n\t" > >> + "vmv.v.i v24, -1\n\t" > >> + "vsetvl %0, x0, %1\n\t" > >> + ".option pop\n\t" > >> + : "=&r" (vl) : "r" (vtype_inval) : "memory"); > >> + riscv_v_disable(); > >> +} > >> + > >> +static inline void riscv_v_vstate_discard(struct pt_regs *regs) > >> +{ > >> + if (!riscv_v_vstate_query(regs)) > >> + return; > >> + > >> + __riscv_v_vstate_discard(); > >> + riscv_v_vstate_on(regs); > >> +} > >> + > >> static inline void riscv_v_vstate_restore(struct task_struct *task, > >> struct pt_regs *regs) > >> { > >> - if ((regs->status & SR_VS) != SR_VS_OFF) { > >> - struct __riscv_v_ext_state *vstate = &task->thread.vstate; > >> - > >> + struct __riscv_v_ext_state *vstate = &task->thread.vstate; > >> + unsigned long status = regs->status & SR_VS; > >> + > >> + switch (status) { > >> + case SR_VS_INITIAL: > >> + __riscv_v_vstate_discard(); > >> + break; > >> + case SR_VS_CLEAN: > >> + case SR_VS_DIRTY: > >> __riscv_v_vstate_restore(vstate, vstate->datap); > >> __riscv_v_vstate_clean(regs); > >> + break; > >> } > >> } > >> > >> @@ -178,6 +213,7 @@ static inline bool riscv_v_vstate_ctrl_user_allowed(void) { return false; } > >> #define __switch_to_vector(__prev, __next) do {} while (0) > >> #define riscv_v_vstate_off(regs) do {} while (0) > >> #define riscv_v_vstate_on(regs) do {} while (0) > >> +#define riscv_v_vstate_discard(regs) do {} while (0) > >> > >> #endif /* CONFIG_RISCV_ISA_V */ > >> > >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > >> index 5158961ea977..5ff63a784a6d 100644 > >> --- a/arch/riscv/kernel/traps.c > >> +++ b/arch/riscv/kernel/traps.c > >> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > >> regs->epc += 4; > >> regs->orig_a0 = regs->a0; > >> > >> + riscv_v_vstate_discard(regs); > >> + > >> syscall = syscall_enter_from_user_mode(regs, syscall); > >> > >> if (syscall < NR_syscalls) > >> > >> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3 > >> -- > >> 2.39.2 > >> > > > > Hi, the above part looks good to me. In the context of kernel-mode > > vector, it would also be good to just discard V-context at the syscall > > entry. So the kernel can freely use Vector if needed. I will rebase my > > work on top of yours. > > Ok! > > > Another part that just came into my mind is the one for ptrace. Do we > > need to disallow, or immediately return all -1 if the tracee process > > is in the syscall path? It seems that we are likely to get stale > > values on datap if a tracee is being traced during a syscall. > > Hmm, could you elaborate a bit on when the tracer would get stale regs?
Yep, consider that our tracer process attaches to a tracee with PTRACE_SYSCALL. Then, the tracee will let the tracer to inspect it whenever it makes a syscall. The tracer wants to inspect V-registers at these PTRACE_SYSCALL stops. Assume the tracee context switches out before being inspected (Sadly I didn't find this part in the code, so maybe I was wrong). Now, we set all V-regs to -1 and VS to 'On' entering a syscall. However, -1 will not be saved into datap, which the tracer copies from, because riscv_v_vstate_save() only saves whenever VS is 'Dirty'. We intentionally want this because it saves unnecessary context saves. As a result, what we will get with REGSET_V will not reflect the latest one, and what we set will get lost since VS='ON' restores V to -1.
Since we are planning to discard V registers on syscall, does it make sense to also make ptrace aware of this? Or, just leave it as-it because reading/writing V register at syscall is not meaningful already.
Thanks, Andy
| |