Re: [patch] x86: fix taking DNA during 64bit sigreturn

Linus Torvalds
2007-11-12T19:09:54Z

On Sun, 11 Nov 2007, Siddha, Suresh B wrote:
>
> restore sigcontext is taking a DNA exception while restoring FP context from
> the user stack, during the sigreturn. Appended patch fixes it by doing clts()
> if the app doesn't touch FP during the signal handler execution. This will
> stop generating a DNA, during the fxrstor in the sigreturn.
>
> This improves 64-bit lat_sig numbers by ~30% on my core2 platform.

The sad part here is that the 32-bit kernel doesn't seem to have this
problem at all, and has it fixed thanks to just having cleaner
abstractions (eg the "unlazy_fpu()" function will just automatically clear
TS_USEDFPU as part of __save_init_fpu()).

In comparison, the x86-64 math handling is a total friggin mess. Maybe
it's not *beautiful* in the 32-bit path either, but most of the complexity
in the 32-bit stuff is due to just the complexity of interactions. In the
64-bit code, the complexity seems to come largely from just being crap,
crap, crap.

The *real* fix for this is almost certainly to just get rid of the 64-bit
code entirely, and use the 32-bit code as the base for one single unified
setup. The 32-bit code should be largely a superset of the 64-bit code
anyway, since it has to handle more cases, and does it more cleanly.

Oh, well. Your patch looks good for now.

Btw: the 64-bit code looks like it might have preemption issues too.
Another bug that seems to be fixed in the 32-bit codepath.

Anybody willing to try to do that unification around the 32-bit code? The
biggest issue with the 64-bit code is to avoid the extra rex prefix for
"fxsave", which means that the fxsave/fxrstor thing is done with

"rex64/fxsave %P2(%1)"
: "m" (tsk->thread.i387.fxsave)
: "cdaSDb" (tsk),
"i" (offsetof(__typeof__(*tsk), thread.i387.fxsave)));

which isn't exactly pretty, but the memory address generation works fine
in 32-bit code too, and the rex override is easily done with

#ifdef CONFIG_X86_64
#define REX64 "rex64/"
#else
#define REX64 ""
#endif

and then you just use

REX64 "fxsave"

in the asm statements instead.

So I'll apply this patch for 2.6.24, but considering that it really looks
like x86-64 is preempt-broken too, if somebody does a larger patch that
does the above merge and tests it, I would be open to apply that too. It
does seem to be a real bug (having a scheduling event in the signal path
between the actual fxsave and the clearing of the bits would *seem* to
possibly screw up the FP state!)

But maybe I'm missing some reason why it doesn't matter. The 32-bit code
was fixed back in 2003 (commit 5bff44fc272b948a85e893a007d01b9dfb3ad04f
"Be a lot more careful about TS_USEDFPU and preemption" in the historical
tree with a comment like this:

We had some races where we tested (or set) TS_USEDFPU together
with sequences that depended on the setting (like clearing or
setting the TS flag in %cr0) and we could be preempted in between,
which screws up the FPU state, since preemption will itself change
USEDFPU and the TS flag.

and maybe it's just the 32-bit tree being unnecessarily careful. It's long
enough ago that I don't remember the details, but I have a dim memory of
there actually being some state corruption involved.

Linus