| Message ID | 20240327193601.28903-2-palmer@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Clobber V state on system calls | expand |
LGTM. I suspect this hasn't manifested as a bug because a glibc routine with an inline syscall would need to be vectorized for this to be a potential problem. But the prophylaxis is a good idea. On Wed, Mar 27, 2024 at 2:37 PM Palmer Dabbelt <palmer@rivosinc.com> wrote: > > The Linux uABI clobbers all V state on syscalls (similar to SVE), but > the syscall inline asm macros don't enforce this. So just explicitly > clobber everything. > > Reported-by: Vineet Gupta <vineetg@rivosinc.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > --- > Vineet's been debugging a userspace hang, and it looks like it's > uncovered at least three issues: > > * Linux isn't properly tracking V state, which results in some > signal-based userpace return paths missing the V state save. This is > almost certainly a Linux bug, Charlie is looking at it. > * GCC only discards the V register state on function calls, despite the > ABI also mandating that the V CSR state is discarded. I'm not 100% on > this one as I don't really understand the vsetvl passes, but we were > talking about it on the GCC call yesterday and that's our best guess > right now. > * glibc doesn't mark the V state as clobbered by syscalls. > > I don't know if we can actually manifest incorrect behavior here and it > definately doesn't build (GCC doesn't support vxsat [1]). I'm sort of > just sending this as a placeholder, but I figured with all the other > chaos I should send it rather than risking forgetting about it. > > [1]: https://inbox.sourceware.org/gcc-patches/20240327195403.29732-2-palmer@rivosinc.com/ > --- > sysdeps/unix/sysv/linux/riscv/sysdep.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h > index ee015dfeb6..3e3971e321 100644 > --- a/sysdeps/unix/sysv/linux/riscv/sysdep.h > +++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h > @@ -354,7 +354,17 @@ > _sys_result; \ > }) > > +#ifdef __riscv_vector > +# define __SYSCALL_CLOBBERS "memory", "vl", "vtype", "vxrm", "vxsat", \ > + "v0", "v1", "v2", "v3", "v4", "v5", \ > + "v6", "v7", "v8", "v9", "v10", "v11", \ > + "v12", "v13", "v14", "v15", "v16", "v17", \ > + "v18", "v18", "v19", "v20", "v21", "v22", \ > + "v23", "v24", "v25", "v26", "v27", "v28", \ > + "v29", "v30", "v31" > +#else > # define __SYSCALL_CLOBBERS "memory" > +#endif > > extern long int __syscall_error (long int neg_errno); > > -- > 2.44.0 >
On Wed, 27 Mar 2024 14:48:45 PDT (-0700), Andrew Waterman wrote: > LGTM. I suspect this hasn't manifested as a bug because a glibc > routine with an inline syscall would need to be vectorized for this to > be a potential problem. But the prophylaxis is a good idea. IIUC we've also got another quirk where GCC discards all V register state on inline ASM blocks (but I think doesn't discard the V CSR state), so it'd be pretty unlikely we actually vectorize anything with the syscall macros. Getting a reproducer for those is next on the TODO list ;) > On Wed, Mar 27, 2024 at 2:37 PM Palmer Dabbelt <palmer@rivosinc.com> wrote: >> >> The Linux uABI clobbers all V state on syscalls (similar to SVE), but >> the syscall inline asm macros don't enforce this. So just explicitly >> clobber everything. >> >> Reported-by: Vineet Gupta <vineetg@rivosinc.com> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> >> --- >> Vineet's been debugging a userspace hang, and it looks like it's >> uncovered at least three issues: >> >> * Linux isn't properly tracking V state, which results in some >> signal-based userpace return paths missing the V state save. This is >> almost certainly a Linux bug, Charlie is looking at it. >> * GCC only discards the V register state on function calls, despite the >> ABI also mandating that the V CSR state is discarded. I'm not 100% on >> this one as I don't really understand the vsetvl passes, but we were >> talking about it on the GCC call yesterday and that's our best guess >> right now. >> * glibc doesn't mark the V state as clobbered by syscalls. >> >> I don't know if we can actually manifest incorrect behavior here and it >> definately doesn't build (GCC doesn't support vxsat [1]). I'm sort of >> just sending this as a placeholder, but I figured with all the other >> chaos I should send it rather than risking forgetting about it. >> >> [1]: https://inbox.sourceware.org/gcc-patches/20240327195403.29732-2-palmer@rivosinc.com/ >> --- >> sysdeps/unix/sysv/linux/riscv/sysdep.h | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h >> index ee015dfeb6..3e3971e321 100644 >> --- a/sysdeps/unix/sysv/linux/riscv/sysdep.h >> +++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h >> @@ -354,7 +354,17 @@ >> _sys_result; \ >> }) >> >> +#ifdef __riscv_vector >> +# define __SYSCALL_CLOBBERS "memory", "vl", "vtype", "vxrm", "vxsat", \ >> + "v0", "v1", "v2", "v3", "v4", "v5", \ >> + "v6", "v7", "v8", "v9", "v10", "v11", \ >> + "v12", "v13", "v14", "v15", "v16", "v17", \ >> + "v18", "v18", "v19", "v20", "v21", "v22", \ >> + "v23", "v24", "v25", "v26", "v27", "v28", \ >> + "v29", "v30", "v31" >> +#else >> # define __SYSCALL_CLOBBERS "memory" >> +#endif >> >> extern long int __syscall_error (long int neg_errno); >> >> -- >> 2.44.0 >>
On 3/27/24 14:53, Palmer Dabbelt wrote: > On Wed, 27 Mar 2024 14:48:45 PDT (-0700), Andrew Waterman wrote: >> LGTM. I suspect this hasn't manifested as a bug because a glibc >> routine with an inline syscall would need to be vectorized for this to >> be a potential problem. But the prophylaxis is a good idea. > IIUC we've also got another quirk where GCC discards all V register > state on inline ASM blocks If so, is this patch needed ? No, it doesn't unless V regs are in clobber list. > (but I think doesn't discard the V CSR > state), so it'd be pretty unlikely we actually vectorize anything with > the syscall macros. Getting a reproducer for those is next on the TODO > list 😉 If one specifies vtype as clobber, it is refreshed with a vsetvl. gcc seems to be doing the right thing ATM. I've posted a test which confirms the same [1] -Vineet [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/648499.html
diff --git a/sysdeps/unix/sysv/linux/riscv/sysdep.h b/sysdeps/unix/sysv/linux/riscv/sysdep.h index ee015dfeb6..3e3971e321 100644 --- a/sysdeps/unix/sysv/linux/riscv/sysdep.h +++ b/sysdeps/unix/sysv/linux/riscv/sysdep.h @@ -354,7 +354,17 @@ _sys_result; \ }) +#ifdef __riscv_vector +# define __SYSCALL_CLOBBERS "memory", "vl", "vtype", "vxrm", "vxsat", \ + "v0", "v1", "v2", "v3", "v4", "v5", \ + "v6", "v7", "v8", "v9", "v10", "v11", \ + "v12", "v13", "v14", "v15", "v16", "v17", \ + "v18", "v18", "v19", "v20", "v21", "v22", \ + "v23", "v24", "v25", "v26", "v27", "v28", \ + "v29", "v30", "v31" +#else # define __SYSCALL_CLOBBERS "memory" +#endif extern long int __syscall_error (long int neg_errno);
The Linux uABI clobbers all V state on syscalls (similar to SVE), but the syscall inline asm macros don't enforce this. So just explicitly clobber everything. Reported-by: Vineet Gupta <vineetg@rivosinc.com> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> --- Vineet's been debugging a userspace hang, and it looks like it's uncovered at least three issues: * Linux isn't properly tracking V state, which results in some signal-based userpace return paths missing the V state save. This is almost certainly a Linux bug, Charlie is looking at it. * GCC only discards the V register state on function calls, despite the ABI also mandating that the V CSR state is discarded. I'm not 100% on this one as I don't really understand the vsetvl passes, but we were talking about it on the GCC call yesterday and that's our best guess right now. * glibc doesn't mark the V state as clobbered by syscalls. I don't know if we can actually manifest incorrect behavior here and it definately doesn't build (GCC doesn't support vxsat [1]). I'm sort of just sending this as a placeholder, but I figured with all the other chaos I should send it rather than risking forgetting about it. [1]: https://inbox.sourceware.org/gcc-patches/20240327195403.29732-2-palmer@rivosinc.com/ --- sysdeps/unix/sysv/linux/riscv/sysdep.h | 10 ++++++++++ 1 file changed, 10 insertions(+)