Re: [PATCH v2] riscv: Discard vector state on syscalls

[Björn Töpel]

Andy Chiu <andy.chiu@xxxxxxxxxx> writes:

> On Wed, Jun 28, 2023 at 6:35 PM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>>
>> Andy Chiu <andy.chiu@xxxxxxxxxx> writes:
>>
>> > On Tue, Jun 27, 2023 at 12:57 AM Björn Töpel <bjorn@xxxxxxxxxx> wrote:
>> >>
>> >> From: Björn Töpel <bjorn@xxxxxxxxxxxx>
>> >>
>> >> 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@xxxxxxxxxxxxxxxxx/ # [1]
>> >> Suggested-by: Darius Rad <darius@xxxxxxxxxxxx>
>> >> Suggested-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
>> >> Suggested-by: Rémi Denis-Courmont <remi@xxxxxxxxxx>
>> >> Signed-off-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
>> >> ---
>> >> 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.

It's not a racy, but you're correct that setting the state to Initial,
will cause issues. When get/set_regs is called, the tracee will be
stopped, and a schedule() has been done.

Tracee: syscall-->(datap stale; change dirty->initial)-->stopped (datap
still stale). Tracer will get stale data.

> 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.

Special handling for ptrace is a bit overkill -- at least now. I'll spin
a v3, where discard simply sets the state to dirty.

Thanks for finding this!


Björn