x86, fpu: shift drop_init_fpu() from save_xstate_sig() to handle_signal()

Al, Linus, could you take a look?

Looks simple, but I have to admit that every time I read FPU code I feel that I never read it before. And I never really understood it in details.

See the changelog, but in short drop_init_fpu() in save_xstate_sig() looks wrong. This assumes that we are going to call the handler and thus we need the new FPU state. But this is only true if setup_frame() won't fail after that. If it fails, we simply lose the FPU state.

Many thanks to Bean Anderson for the detailed report, let me quote it:

(1) A real-time signal is being delivered to a thread.
(2) There is not enough room to push the ucontext on the stack so a SIGSEGV is generated
(3) The segv handler is running on the sigaltstack.
(4) The ucontext received by the segv handler does not contain the FP registers. And on return from the segv handler, the existing FP registers appear to be zero'd out.

Oleg.

save_xstate_sig()->drop_init_fpu() doesn't look right. setup_rt_frame() can fail after that, in this case the next setup_rt_frame() triggered by SIGSEGV won't save fpu simply because the old state was lost. This obviously mean that fpu won't be restored after sys_rt_sigreturn() from SIGSEGV handler.

Shift drop_init_fpu() into !failed branch in handle_signal().

Test-case (needs -O2):

	#include <stdio.h>
	#include <signal.h>
	#include <unistd.h>
	#include <sys/syscall.h>
	#include <sys/mman.h>
	#include <pthread.h>
	#include <assert.h>

	volatile double D;

	void test(double d)
	{
		int pid = getpid();

		for (D = d; D == d; ) {
			/* sys_tkill(pid, SIGHUP); asm to avoid save/reload
			 * fp regs around "C" call */
			asm ("" : : "a"(200), "D"(pid), "S"(1));
			asm ("syscall" : : : "ax");
		}

		printf("ERR!!\n");
	}

	void sigh(int sig)
	{
	}

	char altstack[4096 * 10] __attribute__((aligned(4096)));

	void *tfunc(void *arg)
	{
		for (;;) {
			mprotect(altstack, sizeof(altstack), PROT_READ);
			mprotect(altstack, sizeof(altstack), PROT_READ|PROT_WRITE);
		}
	}

	int main(void)
	{
		stack_t st = {
			.ss_sp = altstack,
			.ss_size = sizeof(altstack),
			.ss_flags = SS_ONSTACK,
		};

		struct sigaction sa = {
			.sa_handler = sigh,
		};

		pthread_t pt;

		sigaction(SIGSEGV, &sa, NULL);
		sigaltstack(&st, NULL);
		sa.sa_flags = SA_ONSTACK;
		sigaction(SIGHUP, &sa, NULL);

		pthread_create(&pt, NULL, tfunc, NULL);

		test(123.456);
		return 0;
	}

---
 arch/x86/kernel/signal.c |    5 +++++
 arch/x86/kernel/xsave.c  |    2 --
 2 files changed, 5 insertions(+), 2 deletions(-)

index 2851d63..ed37a76 100644
@@ -675,6 +675,11 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 		 * handler too.
 		 */
 		regs->flags &= ~(X86_EFLAGS_DF|X86_EFLAGS_RF|X86_EFLAGS_TF);
+		/*
+		 * Ensure the signal handler starts with the new fpu state.
+		 */
+		if (used_math())
+			drop_init_fpu(current);
 	}
 	signal_setup_done(failed, ksig, test_thread_flag(TIF_SINGLESTEP));
 }

index a4b451c..74b34c2 100644
@@ -268,8 +268,6 @@ int save_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 	if (use_fxsr() && save_xstate_epilog(buf_fx, ia32_fxstate))
 		return -1;
 
-	drop_init_fpu(tsk);	/* trigger finit */
-
 	return 0;
 }
 

1.5.5.1

Add more cc's. Suresh, could you please review?

While at it, can't we cleanup the use_eager_fpu() logic? Just look at switch_fpu_prepare(),

	else if (!use_eager_fpu())
		 stts();

looks "obviously buggy" if use_eager_fpu() && !tsk_used_math(new). In this case the "new" task will use the unitialized fpu state if it starts to use fpu. Yes, in fact this is not a bug, this is only possible when we know that the task can't return to usermode (__restore_xstate_sig(), exit_thread()).

But still this doesn't look clean, see 5/5. And I think we can do a lot more cleanups on top of this change.

But! I know absolutely nothing about i387, and I do not know how can I test these changes. In any case these patches should be ignored without authoritative acks. However the kernel seems to boot/run fine with the additional debugging patch below.

Oleg.

 arch/x86/include/asm/fpu-internal.h |   10 +++---
 arch/x86/kernel/i387.c              |    6 ++--
 arch/x86/kernel/process.c           |    3 +-
 arch/x86/kernel/xsave.c             |   55 +++++++++++++++++++---------------
 4 files changed, 41 insertions(+), 33 deletions(-)

-------------------------------------------------------------------------------

index 593257d..511e824 100644
@@ -283,8 +283,18 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	unsigned fsindex, gsindex;
 	fpu_switch_t fpu;
 
+	if (use_eager_fpu()) {
+		BUG_ON(!__thread_has_fpu(prev_p));
+		BUG_ON(!tsk_used_math(prev_p));
+		BUG_ON(!tsk_used_math(next_p));
+	}
+
 	fpu = switch_fpu_prepare(prev_p, next_p, cpu);
 
+	if (use_eager_fpu()) {
+		BUG_ON(!__thread_has_fpu(next_p));
+	}
+
 	/*
 	 * Reload esp0, LDT and the page table pointer:
 	 */

index 857ba40..a74e873 100644
@@ -1923,7 +1923,7 @@ extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t *ut,
  * child is not running and in turn not changing child->flags
  * at the same time the parent does it.
  */
-#define clear_stopped_child_used_math(child) do { (child)->flags &= ~PF_USED_MATH; } while (0)
+#define clear_stopped_child_used_math(child) do { BUG_ON(use_eager_fpu()); (child)->flags &= ~PF_USED_MATH; } while (0)
 #define set_stopped_child_used_math(child) do { (child)->flags |= PF_USED_MATH; } while (0)
 #define clear_used_math() clear_stopped_child_used_math(current)
 #define set_used_math() set_stopped_child_used_math(current)

Preparation. sanitize_restored_xstate() and convert_to_fxsr() do not really need task_struct, change them to accept task->thread.fpu.state instead.

---
 arch/x86/include/asm/fpu-internal.h |    2 +-
 arch/x86/kernel/i387.c              |    6 +++---
 arch/x86/kernel/xsave.c             |   13 ++++++-------
 3 files changed, 10 insertions(+), 11 deletions(-)

index 37506df..ae6bd35 100644
@@ -46,7 +46,7 @@ DECLARE_PER_CPU(struct task_struct *, fpu_owner_task);
 
 extern void convert_from_fxsr(struct user_i387_ia32_struct *env,
 			      struct task_struct *tsk);
-extern void convert_to_fxsr(struct task_struct *tsk,
+extern void convert_to_fxsr(union thread_xstate *xstate,
 			    const struct user_i387_ia32_struct *env);
 
 extern user_regset_active_fn fpregs_active, xfpregs_active;

index d5dd808..6de1916 100644
@@ -489,11 +489,11 @@ convert_from_fxsr(struct user_i387_ia32_struct *env, struct task_struct *tsk)
 		memcpy(&to[i], &from[i], sizeof(to[0]));
 }
 
-void convert_to_fxsr(struct task_struct *tsk,
+void convert_to_fxsr(union thread_xstate *xstate,
 		     const struct user_i387_ia32_struct *env)
 
 {
-	struct i387_fxsave_struct *fxsave = &tsk->thread.fpu.state->fxsave;
+	struct i387_fxsave_struct *fxsave = &xstate->fxsave;
 	struct _fpreg *from = (struct _fpreg *) &env->st_space[0];
 	struct _fpxreg *to = (struct _fpxreg *) &fxsave->st_space[0];
 	int i;

@@ -574,7 +574,7 @@ int fpregs_set(struct task_struct *target, const struct user_regset *regset,
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &env, 0, -1);
 	if (!ret)
-		convert_to_fxsr(target, &env);
+		convert_to_fxsr(target->thread.fpu.state, &env);
 
 	/*
 	 * update the header bit in the xsave header, indicating the

index 74b34c2..74d4129 100644
@@ -272,11 +272,11 @@ int save_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 }
 
 static inline void
-sanitize_restored_xstate(struct task_struct *tsk,
+sanitize_restored_xstate(union thread_xstate *xstate,
 			 struct user_i387_ia32_struct *ia32_env,
 			 u64 xstate_bv, int fx_only)
 {
-	struct xsave_struct *xsave = &tsk->thread.fpu.state->xsave;
+	struct xsave_struct *xsave = &xstate->xsave;
 	struct xsave_hdr_struct *xsave_hdr = &xsave->xsave_hdr;
 
 	if (use_xsave()) {

@@ -299,8 +299,7 @@ sanitize_restored_xstate(struct task_struct *tsk,
 		 * reasons.
 		 */
 		xsave->i387.mxcsr &= mxcsr_feature_mask;
-
-		convert_to_fxsr(tsk, ia32_env);
+		convert_to_fxsr(xstate, ia32_env);
 	}
 }
 

@@ -375,7 +374,7 @@ int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 		 * thread's fpu state, reconstruct fxstate from the fsave
 		 * header. Sanitize the copied state etc.
 		 */
-		struct xsave_struct *xsave = &tsk->thread.fpu.state->xsave;
+		union thread_xstate *xstate = tsk->thread.fpu.state;
 		struct user_i387_ia32_struct env;
 		int err = 0;
 

@@ -389,11 +388,11 @@ int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		drop_fpu(tsk);
 
-		if (__copy_from_user(xsave, buf_fx, state_size) ||
+		if (__copy_from_user(&xstate->xsave, buf_fx, state_size) ||
 		    __copy_from_user(&env, buf, sizeof(env))) {
 			err = -1;
 		} else {
-			sanitize_restored_xstate(tsk, &env, xstate_bv, fx_only);
+			sanitize_restored_xstate(xstate, &env, xstate_bv, fx_only);
 			set_used_math();
 		}
 

1.5.5.1

__restore_xstate_sig() calls math_state_restore() with preemption enabled, not good. But this is minor, the main problem is that this drop_fpu/set_used_math/math_state_restore sequence creates the nasty "use_eager_fpu() && !used_math()" special case which complicates other FPU paths.

Change __restore_xstate_sig() to switch to swapper's fpu state, copy the user state to the thread's fpu state, and switch fpu->state back after sanitize_restored_xstate().

Without use_eager_fpu() fpu->state is null in between but this is fine because in this case we rely on clear_used_math()/set_used_math(), so this doesn't differ from !fpu_allocated() case.

Note: with or without this patch, perhaps it makes sense to send SEGV if __copy_from_user() fails.

---
 arch/x86/kernel/xsave.c |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)

index 74d4129..51be404 100644
@@ -325,6 +325,22 @@ static inline int restore_user_xstate(void __user *buf, u64 xbv, int fx_only)
 		return frstor_user(buf);
 }
 
+static void switch_fpu_xstate(struct task_struct *tsk, union thread_xstate *xstate)
+{
+	preempt_disable();
+	__drop_fpu(tsk);
+	tsk->thread.fpu_counter = 0;
+	tsk->thread.fpu.state = xstate;
+	/* use_eager_fpu() => xstate != NULL */
+	if (use_eager_fpu())
+		math_state_restore();
+	else if (xstate)
+		set_used_math();
+	else
+		clear_used_math();
+	preempt_enable();
+}
+
 int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 {
 	int ia32_fxstate = (buf != buf_fx);

@@ -377,28 +393,20 @@ int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 		union thread_xstate *xstate = tsk->thread.fpu.state;
 		struct user_i387_ia32_struct env;
 		int err = 0;
-
 		/*
-		 * Drop the current fpu which clears used_math(). This ensures
-		 * that any context-switch during the copy of the new state,
-		 * avoids the intermediate state from getting restored/saved.
-		 * Thus avoiding the new restored state from getting corrupted.
-		 * We will be ready to restore/save the state only after
-		 * set_used_math() is again set.
+		 * Ensure that that any context-switch during the copy of
+		 * the new state, avoids the intermediate state from getting
+		 * restored/saved.
 		 */
-		drop_fpu(tsk);
-
+		switch_fpu_xstate(tsk, init_task.thread.fpu.state);
 		if (__copy_from_user(&xstate->xsave, buf_fx, state_size) ||
 		    __copy_from_user(&env, buf, sizeof(env))) {
+		    	fpu_finit(&tsk->thread.fpu);
 			err = -1;
 		} else {
 			sanitize_restored_xstate(xstate, &env, xstate_bv, fx_only);
-			set_used_math();
 		}
-
-		if (use_eager_fpu())
-			math_state_restore();
-
+		switch_fpu_xstate(tsk, xstate);
 		return err;
 	} else {
 		/*

1.5.5.1

I really dislike this one.

If I read it right, you now do *two* math_state_restore calls for each FPU signal state restore. That's potentially quite expensive.

Also, you can actually end up with multiple threads pointing to the same math state in init_task.thread.fpu.state, right? Why is that any better than just having the save state temporarily contain garbage?

The other patches look sane, this one I really don't like. You may have good reasons for it, but it's disgusting.

Linus

(Damn, really fix this time, sorry for resend)

And the patch is buggy, fpu_finit(&tsk->thread.fpu) if __copy_from_user() fails is obviously wrong, but this is fixable.

Yes, this adds one restore_fpu_checking().

But only if a 32bit task does this. And only if use_eager_fpu(), and in this case we do this on every context switch unconditionally.

So personally I think it is not that bad. And this allows to do more cleanups (if this can actually work of course). But I can't really judge.

Yes. I think this should be fine, but let me remind that I do not understand i387.

I think this should be safe, because this thread and/or swapper/0 can do nothing with with fpu->state, and they should not use fpu. So I hope that, say, __save_init_fpu() and restore_fpu_checking() can race with each other using the same fpu->state without any problem. kernel_fpu_begin() looks fine to, fpu_save_init() should not hurt.

But again, again, this is only my speculation.

I do not know if restore_fpu_checking(garbage) is safe without sanitize_restored_xstate(). Can't this, say, trigger an exception?

But there is another reason. Any preemption will overwrite ->xsave, and I think this is the main reason why we should be careful.

5/5 (and other potential cleanups) depends on this change.

So do you still think this change is really bad? Or perhaps it is just technically wrong?

We can probably do fault_in_pages() + __copy_from_user_inatomic(), but this will complicate the code more... Something like

	__copy_from_user(&env);	

	while (!fatal_signal_pending() && !fault_in_pages_readable(buf_fx)) {
			return -1;

		preempt_disable();
		if (!__copy_from_user_in_atomic(buf_fx)) {
			sanitize_restored_xstate(...);
			math_state_restore();
			done = true;
		}
		preempt_disable();
		if (done)
			break;
	}

not sure this looks better.

Other ideas or should I simply forget about these cleanups?

OK. Given that this patch at least needs more discussion, let me send another simple fix first. This code calls math_state_restore() without preempt_disable() and afaics this is very wrong and can lead to FPU corruption: if this task gets a preemption after __thread_fpu_begin(), __save_init_fpu() will overwrite the registers we are going to restore.

Btw, do you see any problem with another "shift drop_init_fpu() from save_xstate_sig() to handle_signal()" fix I sent? I think that Bean Anderson is right, this should be fixed.

Oleg.

.. but if that's the case, then what was wrong with the old code that just copied the state over the unused space from the user space buffer?

You can't have it both ways. Either the old code was fine (because it doesn't use the buffer while it is in flux), or the new code is broken (because it uses the shared buffer). Your choice.No?

Linus

Confused... Just in case, I think that you mean current code, and ignoring the lack of preempt_disable() around math_state_restore() it is correct.

I'd like to change it only because this code is the main source of the nasty special case, used_math() and/or __thread_has_fpu(current) can be false even if use_eager_fpu().

But it is not unused? Although I probably misunderstood you from the very beginning.

OK, what I meant that without switch_fpu_xstate(init_task.fpu.state) or another hack we can't avoid drop_fpu() which leads to this special case.

Currently __copy_from_user(&xstate->xsave) copies the new registers right into this thread's fpu->state. If this thread is preempted before math_state_restore(), the context switch (__save_init_fpu) will overwrite the same buffer, the result of __copy_from_user() can be simply lost (entirely or not).

With this patch we can safely do __copy_from_user(xstate), this buffer is not used until the 2nd switch_fpu_xstate().

It uses the shared buffer, yes. But in this case (I think! please correct me!), when this thread uses the swapper's fpu->state, schedule() -> fpu_xsave() into this shared buffer should be fine because it should write the same content?

Oleg.

Well, if you think it is correct (apart from missing preemption), then I disagree *violently* with your "clean it up by restoring things twice" model.

The signal handling overhead of floating point restore is not small, and it's not theoretical. It's actually one of the main things that make signal handling slow. I haven't tested with the new optimized fxrstor, but it was one of the main annoyances with the lmbench numbers back when we used to track those closely (and which were one of the only tests to check signal handling performance).

Of course, these days people try to avoid signals (because they are slow), but still..

Linus

OK.

Again, this only if 32bit && use_eager_fpu(), and use_eager_fpu() adds this overhead to every context switch.

But you convinced me anyway. If this hack doesn't look "obviously safe and correct", lets forget it.

I'll try to play with copy_from_user_in_atomic(), if nothing else just to complete the discussion and see how the code can look in this case.

Thanks!

Oleg.

I'm afraid you all already hate me, but let me continue to spam your mailboxes.

OK, to complete the discussion, the code looks simple unless I missed something,

	if (ia32_fxstate) {
		/*
		 * For 32-bit frames with fxstate, copy the user state to the
		 * thread's fpu state, reconstruct fxstate from the fsave
		 * header. Sanitize the copied state etc.
		 */
		struct xsave_struct *xsave = &tsk->thread.fpu.state->xsave;
		struct user_i387_ia32_struct env;
		bool done;

		if (__copy_from_user(&env, buf, sizeof(env))
			return -1;

		for (done = false; !done; ) {
			if (fatal_signal_pending(current) ||
			    fault_in_pages_readable(buf_fx, state_size))
				return -1;

			preempt_disable();
			pagefault_disable(); /* not really needed */
			done = !__copy_from_user_inatomic(xsave, buf_fx, state_size);
			pagefault_enable();
			if (likely(done)) {
				sanitize_restored_xstate(tsk, &env, xstate_bv, fx_only);
				if (__thread_has_fpu(tsk))
					math_state_restore();
			} else {
				fpu_finit(&tsk->thread.fpu);
			}
			preempt_enable();
		}

		return 0;
	}

and in some sense it is even simpler because we do not care about use_eager_fpu() or set/clear_used_math().

Does it look better than switch_fpu_xstate() hack?

However, this code can race with kernel_fpu_begin() if use_eager_fpu(). I _think_ that kernel_fpu_begin/end and irq_fpu_usable() need cleanups too and in any case. Will try to do, but I am not sure.

And to spam you even more, I'll send you a couple of other, more simple cleanups.

Oleg.

fix Suresh's email...

And the patch is buggy, fpu_finit(&tsk->thread.fpu) if __copy_from_user() fails is obviously wrong, but this is fixable.

Yes, this adds one restore_fpu_checking().

But only if a 32bit task does this. And only if use_eager_fpu(), and in this case we do this on every context switch unconditionally.

So personally I think it is not that bad. And this allows to do more cleanups (if this can actually work of course). But I can't really judge.

Yes. I think this should be fine, but let me remind that I do not understand i387.

I think this should be safe, because this thread and/or swapper/0 can do nothing with with fpu->state, and they should not use fpu. So I hope that, say, __save_init_fpu() and restore_fpu_checking() can race with each other using the same fpu->state without any problem. kernel_fpu_begin() looks fine to, fpu_save_init() should not hurt.

But again, again, this is only my speculation.

I do not know if restore_fpu_checking(garbage) is safe without sanitize_restored_xstate(). Can't this, say, trigger an exception?

But there is another reason. Any preemption will overwrite ->xsave, and I think this is the main reason why we should be careful.

5/5 (and other potential cleanups) depends on this change.

So do you still think this change is really bad? Or perhaps it is just technically wrong?

We can probably do fault_in_pages() + __copy_from_user_inatomic(), but this will complicate the code more... Something like

	__copy_from_user(&env);	

	while (!fatal_signal_pending() && !fault_in_pages_readable(buf_fx)) {
			return -1;

		preempt_disable();
		if (!__copy_from_user_in_atomic(buf_fx)) {
			sanitize_restored_xstate(...);
			math_state_restore();
			done = true;
		}
		preempt_disable();
		if (done)
			break;
	}

not sure this looks better.

Other ideas or should I simply forget about these cleanups?

OK. Given that this patch at least needs more discussion, let me send another simple fix first. This code calls math_state_restore() without preempt_disable() and afaics this is very wrong and can lead to FPU corruption: if this task gets a preemption after __thread_fpu_begin(), __save_init_fpu() will overwrite the registers we are going to restore.

Btw, do you see any problem with another "shift drop_init_fpu() from save_xstate_sig() to handle_signal()" fix I sent? I think that Bean Anderson is right, this should be fixed.

Oleg.

Make exit_thread()->drop_fpu() depend on !use_eager_fpu(). This removes another source of "use_eager_fpu() && !used_math()" special case.

---
 arch/x86/kernel/process.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index 5df9447..7474f56 100644
@@ -120,7 +120,8 @@ void exit_thread(void)
 		kfree(bp);
 	}
 
-	drop_fpu(me);
+	if (!use_eager_fpu())
+		drop_fpu(me);
 }
 
 void flush_thread(void)

1.5.5.1

This is not strictly needed, eager_fpu_init() is called before the very first schedule() on this CPU, but let's remove the last unconditional clear_used_math().

eager_fpu_init() does init_fpu() every time, but this is only needed on boot CPU, otherwise this idle thread already has a valid fpu state copied by arch_dup_task_struct().

And this allows to remove clear_used_math(). swapper/0 doesn't have this flag when start_kernel() calls us, other idle threads correctly have this flag set.

---
 arch/x86/kernel/xsave.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 51be404..33f4ebe 100644
@@ -601,19 +601,20 @@ void xsave_init(void)
 	this_func();
 }
 
-static inline void __init eager_fpu_init_bp(void)
+static void __init eager_fpu_init_bp(void)
 {
 	current->thread.fpu.state =
 	    alloc_bootmem_align(xstate_size, __alignof__(struct xsave_struct));
 	if (!init_xstate_buf)
 		setup_init_fpu_buf();
+
+	init_fpu(current);
 }
 
 void eager_fpu_init(void)
 {
 	static __refdata void (*boot_func)(void) = eager_fpu_init_bp;
 
-	clear_used_math();
 	current_thread_info()->status = 0;
 
 	if (eagerfpu == ENABLE)

@@ -633,7 +634,6 @@ void eager_fpu_init(void)
 	 * This is same as math_state_restore(). But use_xsave() is
 	 * not yet patched to use math_state_restore().
 	 */
-	init_fpu(current);
 	__thread_fpu_begin(current);
 	if (cpu_has_xsave)
 		xrstor_state(init_xstate_buf, -1);

1.5.5.1

switch_fpu_prepare() calls use_eager_fpu() 3 times, and every call is confusing.

1st: "tsk_used_math(new) && use_eager_fpu()" is pointless, eagerfpu
means that used_math() is also true.

2nd: !fpu.preload && use_eager_fpu() is not possible, we do not need
to check use_eager_fpu() before use_eager_fpu(). IOW, !preload alway means !use_eager_fpu(), so this check only adds more confusion.

3rd: If __thread_has_fpu(old) == F then use_eager_fpu() is false too,
no need to check it before fpu_lazy_restore(). IOW, the "else" branch is simply impossible if use_eager_fpu().

---
 arch/x86/include/asm/fpu-internal.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index ae6bd35..7b95a87 100644
@@ -424,8 +424,8 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 	 * If the task has used the math, pre-load the FPU on xsave processors
 	 * or if the past 5 consecutive context-switches used math.
 	 */
-	fpu.preload = tsk_used_math(new) && (use_eager_fpu() ||
-					     new->thread.fpu_counter > 5);
+	fpu.preload = use_eager_fpu() ||
+			(tsk_used_math(new) && new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))
 			cpu = ~0;

@@ -437,14 +437,14 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 			new->thread.fpu_counter++;
 			__thread_set_has_fpu(new);
 			prefetch(new->thread.fpu.state);
-		} else if (!use_eager_fpu())
+		} else
 			stts();
 	} else {
 		old->thread.fpu_counter = 0;
 		old->thread.fpu.last_cpu = ~0;
 		if (fpu.preload) {
 			new->thread.fpu_counter++;
-			if (!use_eager_fpu() && fpu_lazy_restore(new, cpu))
+			if (fpu_lazy_restore(new, cpu))
 				fpu.preload = 0;
 			else
 				prefetch(new->thread.fpu.state);

1.5.5.1

Add preempt_disable() + preempt_enable() around math_state_restore() in __restore_xstate_sig(). Otherwise __switch_to() after __thread_fpu_begin() can overwrite fpu->state we are going to restore.

---
 arch/x86/kernel/xsave.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

index 453343c..c52eb9c 100644
@@ -397,8 +397,11 @@ int __restore_xstate_sig(void __user *buf, void __user *buf_fx, int size)
 			set_used_math();
 		}
 
-		if (use_eager_fpu())
+		if (use_eager_fpu()) {
+			preempt_disable();
 			math_state_restore();
+			preempt_enable();
+		}
 
 		return err;
 	} else {

1.5.5.1