Messages in this thread | | | From | Björn Töpel <> | Subject | Re: [PATCH v2] riscv: Discard vector state on syscalls | Date | Wed, 28 Jun 2023 12:35:43 +0200 |
| |
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?
| |