Re: kern/60539 (XSAVE changes break ucontext userspace API)

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Wed, 5 Aug 2026 15:36:03 +0000
> From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
> 
> 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])

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.

# 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=1.20#115

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 = (sizeof(struct sigframe_siginfo) -
+			    offsetof(struct sigframe_siginfo,
+				sf_uc.uc_mcontext.__fpregs)),
+		};
+
 		process_read_xsave(l, &xsavebuf, &xsavelen);
 		KASSERT(xsavebuf != NULL);
 		KASSERT(xsavelen <= XSAVE_MAX_BYTES);
+		CTASSERT(overlap <= XSAVE_MAX_BYTES);
 
 		KASSERT(!onstack || sp >= (char *)l->l_sigstk.ss_sp);
 		KASSERT(!onstack ||
 		    sp - (char *)l->l_sigstk.ss_sp >= xsavelen);
-		sp -= xsavelen;
+		KASSERT(!onstack ||
+		    sp - (char *)l->l_sigstk.ss_sp >= overlap);
+		sp -= MAX(xsavelen, overlap);
 
 		KASSERT(!onstack || sp >= (char *)l->l_sigstk.ss_sp);
 		KASSERT(!onstack ||
@@ -652,6 +661,10 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
 		KASSERT(!onstack || sp >= (char *)l->l_sigstk.ss_sp);
 		KASSERT(((uintptr_t)sp & (XSAVE_ALIGN - 1)) == 0);
 		user_xsave = (void *)sp;
+
+		CTASSERT((overlap % XSAVE_ALIGN) == 0);
+		CTASSERT((overlap & STACK_ALIGNBYTES) == 0);
+		sp += overlap;
 	}
 
 	/*
@@ -672,6 +685,17 @@ sendsig_siginfo(const ksiginfo_t *ksi, c
 	KASSERT(!onstack || (char *)fp >= (char *)l->l_sigstk.ss_sp);
 	KASSERT(((uintptr_t)fp & STACK_ALIGNBYTES) == 8);
 
+	/*
+	 * 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 == 0 ||
+	    (((uintptr_t)&fp->sf_uc.uc_mcontext.__fpregs & (XSAVE_ALIGN - 1))
+		== 0));
+	KASSERT(xsavebuf == 0 || (uintptr_t)user_xsave ==
+	    (uintptr_t)&fp->sf_uc.uc_mcontext.__fpregs);
+
 	memset(&frame, 0, sizeof(frame));
 	frame.sf_ra = (uint64_t)ps->sa_sigdesc[sig].sd_tramp;
 	frame.sf_si._info = 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);
 
 	/*
-	 * 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 ==
+		    (void *)user_xsave);
 		error = cpu_getmcontext_xsave(l, &frame.sf_uc.uc_mcontext,
 		    &frame.sf_uc.uc_flags, xsavebuf, xsavelen, user_xsave);
 		if (error != 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 = sizeof(mcp->__fpregs.__fxsave) };
 	int error;
 
+	CTASSERT(fxsavelen == 512);
+
 	KASSERT(*flags & _UC_FPU);
+	KASSERT(fxsavelen <= xsavelen);
+	KDASSERT(memcmp(&mcp->__fpregs.__fxsave, xsavebuf, fxsavelen) == 0);
 
 	/*
-	 * Copy out the XSAVE area.
+	 * Copy out the part of the XSAVE area that doesn't overlap.
 	 */
-	error = copyout(xsavebuf, user_xsave, xsavelen);
+	error = copyout((const char *)xsavebuf + fxsavelen,
+	    (char *)user_xsave + fxsavelen, xsavelen - fxsavelen);
 	if (error != 0)
 		return error;