NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: kern/60539 (XSAVE changes break ucontext userspace API)
So, the good news is:
1. There is a one-character change that should make this work on all
versions of NetBSD, for programs that don't use AVX (which wouldn't
work before, and which it looks like this program doesn't do
anyway) or don't try to write back registers to be restored on
signal handler return:
-# define XMM_sig(p,i) (((struct fxsave64*)(p)->uc_mcontext.__fpregs)->fx_xmm[i])
+# define XMM_sig(p,i) (((struct fxsave64*)&(p)->uc_mcontext.__fpregs)->fx_xmm[i])
This change is needed because the mcontext_t member __fpregs
changed from being an array to being a union:
-typedef char __fpregset_t[512] __aligned(8);
+typedef union {
+ char __fxsave[512] __aligned(8);
+ struct {
...
+ } __xsave;
+} __fpregset_t;
And although the change I made to __fpregset_t isn't quite
API-compatible (sorry), you just can take a pointer to the array
itself with & (instead taking a pointer to the first element of the
array, which is what you get without the &) and convert it to a
pointer to the struct fxsave64 object. So the change to XMM_sig
will work on older versions of NetBSD just as well as >=11.
2. Existing binaries that don't use AVX registers are unaffected by
any ABI consequences of the XSAVE change, and it looks like that
covers the code you quoted.
The bad news -- other than the API breakage requiring randomly
sprinkling ampersands like you're in a fight with the Rust borrow
checker -- is that if the program _does_ use AVX, then writing back to
the x87/SSE (st(i), xmmN) registers in the FXSAVE part of the
ucontext_t will have no effect when the signal handler returns.
Signal handlers can still _read_ x87/SSE registers out of this
location -- it's just that _writing_ to them won't change the content
that gets restored into the registers on signal handler return.
The reason is that the kernel will lay out two separate copies of the
FXSAVE area (x87/SSE state), which I did in order to make it easy to
verify that the XSAVE patch (to save and restore registers _beyond_
x87/SSE: AVX, AVX-512, AMX, whatever) only has an effect on processes
that actually require the extra area, when they require it.
The layout on the stack on signal delivery looks something like this,
with higher addresses first:
+-----------------------+ \
| Hi16_ZMM | |
| --------------------- | |
| ZMM_Hi256 | |
| --------------------- | > XSAVE
| YMM_Hi128 | |
| --------------------- | |
| FXSAVE in XSAVE | |
+-----------------------+ /
| (opt. padding bytes) |
+-----------------------+ \
| FXSAVE in ucontext_t | |
| --------------------- | > ucontext_t
| rest of ucontext_t | |
+-----------------------+ /
| siginfo_t |
+-----------------------+
| return address |
+-----------------------+ <--- rsp on signal handler entry
I considered making the XSAVE and ucontext_t areas overlap, like this:
/ +-----------------------+
| | Hi16_ZMM |
| | --------------------- |
| | ZMM_Hi256 |
XSAVE < | --------------------- |
| | YMM_Hi128 |
| *-----------------------* \
| | FXSAVE in ucontext_t | |
\ *-----------------------* > ucontext_t
| rest of ucontext_t | |
+-----------------------+ /
| siginfo_t |
+-----------------------+
| return address |
+-----------------------+ <--- rsp on signal handler entry
However, when the signal handler returns to the return address we
provided, it enters the signal trampoline, which calls setcontext(2):
72 NENTRY(__sigtramp_siginfo_2)
73 movq %r15,%rdi
74 movq $SYS_setcontext, %rax
75 syscall
https://nxr.netbsd.org/xref/src/lib/libc/arch/x86_64/sys/__sigtramp2.S?r=1.9
And the first thing setcontext(2) does is copyin sizeof(ucontext_t)
bytes -- that is, _only_ the ucontext_t, _not including_ the rest of
the XSAVE area, because at this point the machine-independent
setcontext(2) logic has no way to know that the downstream
machine-dependent cpu_setmcontext logic is hungry for more bytes:
341 int
342 sys_setcontext(struct lwp *l, const struct sys_setcontext_args *uap,
343 register_t *retval)
344 {
345 /* {
346 syscallarg(const ucontext_t *) ucp;
347 } */
348 struct proc *p = l->l_proc;
349 ucontext_t uc;
350 int error;
351
352 error = copyin(SCARG(uap, ucp), &uc, sizeof (uc));
...
358 error = setucontext(l, &uc);
https://nxr.netbsd.org/xref/src/sys/kern/sys_sig.c?r=1.62#341
And by the time control reaches cpu_setmcontext, it doesn't even know
where it came from in userland, so it doesn't know where to copyin the
rest of the XSAVE area from. I could in principle have added more MD
hooks to sys_setcontext to tell conditionally copy in larger chunks
but that seemed like a very risky change to pull up to 11 in order to
fix a long-term embarrassing x86-only bug.
Instead, I just put the XSAVE area in a separate space on the stack
(again, to keep the stack manipulation logic in sendsig_siginfo very
easy to audit), and carved out 16 bytes of the 512-byte FXSAVE area
that are architecturally unused (bytes 511:416 are not `reserved' like
bytes 463:416 but `not use[d]' by the architecture according to the
Intel manual, and `Reserved, IGN' rather than (e.g.) `Reserved, MBZ'
according to the AMD manual), to store a pointer to and length of the
XSAVE area.
That way, the x86 cpu_setmcontext can find it to copy it in (after
validating the length) in order to restore the state:
2264 if ((flags & _UC_XSAVE) != 0) {
2265 const struct xsave_header *user_xsave =
2266 (void *)(uintptr_t)mcp->__fpregs.__xsave.__xsaveptr;
2267
2268 xsavelen = mcp->__fpregs.__xsave.__xsavelen;
2269 error = process_verify_xsavelen(l, xsavelen);
2270 if (error != 0)
2271 goto out;
2272 xsavebuf = kmem_alloc(xsavelen, KM_SLEEP);
2273 error = copyin(user_xsave, xsavebuf, xsavelen);
2274 if (error != 0)
2275 goto out;
2276 error = process_verify_xsave(l, xsavebuf, xsavelen);
2277 if (error != 0)
2278 goto out;
2279 }
https://nxr.netbsd.org/xref/src/sys/arch/amd64/amd64/machdep.c?r=1.380#2255
(That's why I changed the mcontext_t __fpregs member to be a union: so
I could add named __xsaveptr/__xsavelen members. I don't know why it
was ever an array, anyway; that didn't facilitate access to the CPU
state the way that, say, a struct with st0/st1/.../st7/xmm0/xmm1/...
members would have.)
So, if you want access to state beyond x87/SSE -- the high 128-bit
halves of the AVX/AVX2 YMM registers, the high 256-bit halves of the
AVX512 ZMM registers or the high 16 ZMM registers, the obsolete BND
registers, the future AMX/ACE tile data, &c. -- you will need to fish
it out of uc->uc_mcontext.__fpregs.__xsaveptr/__xsavelen. And
avoiding the overlap meant the new logic in amd64/machdep.c -- and
amd64/netbsd32_machdep.c and i386/machdep.c -- would be nice and
simple and very clearly conditional on `if (process_needs_xsave_p())'.
That meant, however, that there would be two locations where the
FXSAVE area could live: one inside the ucontext_t, and one in the
separate XSAVE area.
`Not to worry,' I thought to myself, `because we can just keep copying
out the ucontext_t FXSAVE area so existing debuggers can still read
the state even if they don't know about the separate XSAVE area, and
it'll all be hunky-dory, right?'
Wrong! FPU trap handlers (and debuggers) can substitute values in
computations, so they may also need to write back registers. And with
two FXSAVE areas, which one does the kernel restore from? Well, I
picked the XSAVE one, not the ucontext_t one, again to keep it simple.
So, perhaps we should combine the approaches:
1. use the __xsaveptr/__xsavelen so that the MD cpu_setmcontext knows
where to find the XSAVE area,
AND
2. make the XSAVE area overlap the ucontext_t so the FXSAVE sections
of the two coincide, with some careful attention to stack pointer
arithmetic and alignment in sendsig_siginfo.
This way:
- Programs that hordes of engineers around the world have rushed to
adapt to NetBSD 11.0 by using __xsaveptr/__xsavelen will still work
in NetBSD 11.1 and NetBSD 12, i.e., we don't break compatibility
_again_ in quick succession.
- Programs whose inexcusibly lazy maintainers are way behind the times
and STILL TODAY haven't done a thing to adapt to NetBSD 11.0 will
also continue to work even if they newly use AVX when they didn't
before, i.e., we restore `compatibility' for programs that never
worked right in the first place on NetBSD.
And for NetBSD>11.0, they will be able to just cast __fpregs to an
XSAVE area for access to >=AVX state, without having to go through
the __xsaveptr/__xsavelen business altogether.
- Programs that don't use extended CPU state beyond x87/SSE remain
unaffected.
- We can pull up the changes -- with the array-to-union change under
#ifdef _KERNEL -- to netbsd-10 and netbsd-9 to make AVX register
access work reliably in the face of signal handlers without breaking
the API or affecting `compatibility' of programs using AVX registers
that never worked right in the first place on NetBSD.
Home |
Main Index |
Thread Index |
Old Index