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)
The following reply was made to PR kern/60539; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: nia%sent.at@localhost, gnats-bugs%netbsd.org@localhost, lib-bug-people%netbsd.org@localhost,
netbsd-bugs%netbsd.org@localhost, gnats-admin%netbsd.org@localhost
Cc:
Subject: Re: kern/60539 (XSAVE changes break ucontext userspace API)
Date: Thu, 6 Aug 2026 20:17:31 +0000
This is a multi-part message in MIME format.
--=_LF0R1FFWgpNWTy7tDsnl9sFJlFnD3FDt
Content-Transfer-Encoding: quoted-printable
> Date: Wed, 5 Aug 2026 15:36:03 +0000
> From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
>=20
> So, the good news is:
>=20
> 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:
>=20
> -# 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)->f=
x_xmm[i])
I should add, for completeness, that if you do want to write the
registers back in signal handlers in programs that use AVX
instructions, you'll have to check _UC_XSAVE to see whether there's a
possibly separate XSAVE area:
# define XMM_sig(p,i) \
(((struct fxsave64 *)((p)->uc_flags & _UC_XSAVE \
? (void *)(p)->uc_mcontext.__fpregs.__xsave.__xsaveptr \
: &(p)->uc_mcontext.__fpregs))->fx_xmm[i])
The member ucontext_t::uc_mcontext.__fpregs.__xsave.__xsaveptr is the
address of the start of an XSAVE area having
ucontext_t::uc_mcontext.__fpregs.__xsave.__xsavelen bytes, in the
OS-independent x86 XSAVE format (possibly XSAVE, possibly XSAVEOPT,
possibly XSAVEC -- it's the user's responsibility to figure that out
from the XSAVE header's XSTATE_BV/XCOMP_BV bits).
I thought of a scenario where this could (in principle) cause trouble,
but I still think it's rather unlikely:
1. Some micro-optimized library provides optional AVX implementations
of subroutines like memcpy, ChaCha encryption, or whatever, based
on runtime CPUID and XCR0 detection.
2. Despite being intended for micro-optimization, this library AVX
code _does not_ do VZEROUPPER when done, so the AVX registers
remain in use indefinitely, and hence NetBSD thinks the thread
requires XSAVE and not just FXSAVE.
3. The rest of the program carefully installs a SIGFPE handler _only_
while it is running known-safe x87/SSE instruction sequences, such
as those generated by a JIT compiler.
Parts (1) and (3) are likely to happen. But part (2) strikes me as
unlikely, because the whole point of this is micro-optimization, and
if you forget to do VZEROUPPER, that incurs a performance penalty on
all non-AVX SSE instructions.
I'm still open to thoughts on:
- Are there applications that the XSAVE ABI change really breaks?
- Should we try to dig ourselves deeper^W^W^Wfind a way to handle
mcontext_t on signal delivery differently?
The attached (untested) patch makes the XSAVE area overlap the
mcontext_t so there's only one FXSAVE area, on amd64. But it can't be
done on i386 because it would conlict with mcontext_t::_mc_tlsbase.
So I'm on the fence about whether to bother at all. Mostly the
benefit is probably just reducing the copyouts on signal delivery by
512 bytes.
--=_LF0R1FFWgpNWTy7tDsnl9sFJlFnD3FDt
Content-Type: text/plain; charset="ISO-8859-1"; name="pr60539-xsavemcontextcompat"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr60539-xsavemcontextcompat.patch"
# HG changeset patch
# User Taylor R Campbell <riastradh%NetBSD.org@localhost>
# Date 1786044700 0
# Thu Aug 06 19:31:40 2026 +0000
# Branch trunk
# Node ID 54069feb2f11ce0b39191566ccb7956862e5c21f
# Parent b125bad3265e13dfd5a772b3676744fca85ce488
# EXP-Topic riastradh-pr60539-xsavemcontextcompat
WIP: amd64: Make XSAVE area overlap mcontext_t on signal delivery.
This way:
1. We copy out 512 bytes fewer.
2. There is only one place where the x87/SSE registers are written to
-- and, more importantly, where they might be read from on return
from signal.
3. Existing applications that write to the mcontext_t's FXSAVE area
to change x87/SSE register content in threads that are using
extended CPU state like the AVX registers will still work (though
I suspect there are few, if any, such applications).
4. Newly adjusted applications that use 11.0's XSAVE area pointer
embedded in a padding cabinet in disused lavatory with a sign on
it saying `BEWARE OF LEOPARD --Intel' in the mcontext_t's FXSAVE
area will still work.
Unfortunately, we can't do the same for i386, because mcontext_t has
extra stuff in it after the FXSAVE area:
115 typedef struct {
116 __gregset_t __gregs;
117 __fpregset_t __fpregs;
118 __greg_t _mc_tlsbase;
119 } mcontext_t;
https://nxr.netbsd.org/xref/src/sys/arch/i386/include/mcontext.h?r=3D1.20#1=
15
PR kern/60539: XSAVE changes break ucontext userspace API
diff -r b125bad3265e -r 54069feb2f11 sys/arch/amd64/amd64/machdep.c
--- a/sys/arch/amd64/amd64/machdep.c Mon Aug 03 19:24:48 2026 +0000
+++ b/sys/arch/amd64/amd64/machdep.c Thu Aug 06 19:31:40 2026 +0000
@@ -635,14 +635,23 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
* FXSAVE area.
*/
if (process_xsave_needed_p(l)) {
+ enum {
+ overlap =3D (sizeof(struct sigframe_siginfo) -
+ offsetof(struct sigframe_siginfo,
+ sf_uc.uc_mcontext.__fpregs)),
+ };
+
process_read_xsave(l, &xsavebuf, &xsavelen);
KASSERT(xsavebuf !=3D NULL);
KASSERT(xsavelen <=3D XSAVE_MAX_BYTES);
+ CTASSERT(overlap <=3D XSAVE_MAX_BYTES);
=20
KASSERT(!onstack || sp >=3D (char *)l->l_sigstk.ss_sp);
KASSERT(!onstack ||
sp - (char *)l->l_sigstk.ss_sp >=3D xsavelen);
- sp -=3D xsavelen;
+ KASSERT(!onstack ||
+ sp - (char *)l->l_sigstk.ss_sp >=3D overlap);
+ sp -=3D MAX(xsavelen, overlap);
=20
KASSERT(!onstack || sp >=3D (char *)l->l_sigstk.ss_sp);
KASSERT(!onstack ||
@@ -652,6 +661,10 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
KASSERT(!onstack || sp >=3D (char *)l->l_sigstk.ss_sp);
KASSERT(((uintptr_t)sp & (XSAVE_ALIGN - 1)) =3D=3D 0);
user_xsave =3D (void *)sp;
+
+ CTASSERT((overlap % XSAVE_ALIGN) =3D=3D 0);
+ CTASSERT((overlap & STACK_ALIGNBYTES) =3D=3D 0);
+ sp +=3D overlap;
}
=20
/*
@@ -672,6 +685,17 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
KASSERT(!onstack || (char *)fp >=3D (char *)l->l_sigstk.ss_sp);
KASSERT(((uintptr_t)fp & STACK_ALIGNBYTES) =3D=3D 8);
=20
+ /*
+ * If we have to use XSAVE, the FXSAVE area of the ucontext_t
+ * on the user's stack must line up with the FXSAVE subarea of
+ * the XSAVE area on the user's stack.
+ */
+ KASSERT(xsavebuf =3D=3D 0 ||
+ (((uintptr_t)&fp->sf_uc.uc_mcontext.__fpregs & (XSAVE_ALIGN - 1))
+ =3D=3D 0));
+ KASSERT(xsavebuf =3D=3D 0 || (uintptr_t)user_xsave =3D=3D
+ (uintptr_t)&fp->sf_uc.uc_mcontext.__fpregs);
+
memset(&frame, 0, sizeof(frame));
frame.sf_ra =3D (uint64_t)ps->sa_sigdesc[sig].sd_tramp;
frame.sf_si._info =3D ksi->ksi_info;
@@ -686,10 +710,12 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
cpu_getmcontext(l, &frame.sf_uc.uc_mcontext, &frame.sf_uc.uc_flags);
=20
/*
- * If we have to use XSAVE, copy out that area separately --
- * and be ready to bail if it failed.
+ * If we have to use XSAVE, copy out the part of it past the
+ * FXSAVE area separately -- and be ready to bail if it failed.
*/
if (xsavebuf) {
+ KASSERT((void *)&fp->sf_uc.uc_mcontext.__fpregs =3D=3D
+ (void *)user_xsave);
error =3D cpu_getmcontext_xsave(l, &frame.sf_uc.uc_mcontext,
&frame.sf_uc.uc_flags, xsavebuf, xsavelen, user_xsave);
if (error !=3D 0)
@@ -2200,9 +2226,15 @@ cpu_getmcontext(struct lwp *l, mcontext_
/*
* cpu_getmcontext_xsave(l, mcp, flags, xsavebuf, xsavelen, user_xsave)
*
- * Copy out xsavebuf[0..xsavelen) to user_xsave, set mcp to point
- * there, and set _UC_XSAVE in flags. Caller must have already
- * used cpu_getmcontext to initialize mcp's FXSAVE area.
+ * Copy out xsavebuf[512..xsavelen) to user_xsave[512..xsavelen),
+ * set mcp to point at it, and set _UC_XSAVE in *flags. Caller:
+ *
+ * - must have already initialized the FXSAVE area of mcontext_t,
+ * - must have already set _UC_FPU in *flags,
+ * - must have arranged user_xsave[0..512) to overlap with the
+ * FXSAVE area of mcontext_t, and
+ * - must subsequently copy out the mcontext_t updated with a
+ * pointer/length to the XSAVE area.
*
* May fail if the copyout fails.
*/
@@ -2211,14 +2243,20 @@ cpu_getmcontext_xsave(struct lwp *l, mco
const struct xsave_header *xsavebuf, size_t xsavelen,
struct xsave_header *user_xsave)
{
+ enum { fxsavelen =3D sizeof(mcp->__fpregs.__fxsave) };
int error;
=20
+ CTASSERT(fxsavelen =3D=3D 512);
+
KASSERT(*flags & _UC_FPU);
+ KASSERT(fxsavelen <=3D xsavelen);
+ KDASSERT(memcmp(&mcp->__fpregs.__fxsave, xsavebuf, fxsavelen) =3D=3D 0);
=20
/*
- * Copy out the XSAVE area.
+ * Copy out the part of the XSAVE area that doesn't overlap.
*/
- error =3D copyout(xsavebuf, user_xsave, xsavelen);
+ error =3D copyout((const char *)xsavebuf + fxsavelen,
+ (char *)user_xsave + fxsavelen, xsavelen - fxsavelen);
if (error !=3D 0)
return error;
=20
--=_LF0R1FFWgpNWTy7tDsnl9sFJlFnD3FDt--
Home |
Main Index |
Thread Index |
Old Index