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 <[email protected]>
To: [email protected], [email protected], [email protected],
	[email protected], [email protected]
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 <[email protected]>
 >=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 <[email protected]>
 # 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--