[PATCH 0/1] x86, fpu: shift drop_init_fpu() from save_xstate_sig() to handle_signal()

[PATCH 0/1] x86, fpu: shift drop_init_fpu() from save_xstate_sig() to handle_signal()

[Oleg Nesterov]

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.

[PATCH 1/1] x86, fpu: shift drop_init_fpu() from save_xstate_sig() to handle_signal()

[Oleg Nesterov]

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;
	}

Reported-by: Bean Anderson <bean@azulsystems.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/signal.c |    5 +++++
 arch/x86/kernel/xsave.c  |    2 --
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 2851d63..ed37a76 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -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));
 }
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index a4b451c..74b34c2 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -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

[PATCH 0/5] x86, fpu: make use_eager_fpu() more eager

[Oleg Nesterov]

On 08/22, Oleg Nesterov wrote:
>
> 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.

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(-)

-------------------------------------------------------------------------------
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 593257d..511e824 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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:
 	 */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 857ba40..a74e873 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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)

[PATCH 1/5] x86, fpu: change sanitize_restored_xstate() and convert_to_fxsr() to accept thread_xstate

[Oleg Nesterov]

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

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 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(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 37506df..ae6bd35 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -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;
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index d5dd808..6de1916 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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
diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 74b34c2..74d4129 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -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

[PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

__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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/xsave.c |   36 ++++++++++++++++++++++--------------
 1 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 74d4129..51be404 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -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

[PATCH 3/5] x86, fpu: don't drop_fpu() in exit_thread() if use_eager_fpu()

[Oleg Nesterov]

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

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/process.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 5df9447..7474f56 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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

[PATCH 4/5] x86, fpu: shift init_fpu() from eager_fpu_init() to eager_fpu_init_bp()

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/kernel/xsave.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 51be404..33f4ebe 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -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

[PATCH 5/5] x86, fpu: sanitize the usage of use_eager_fpu() in switch_fpu_prepare()

[Oleg Nesterov]

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().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/fpu-internal.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index ae6bd35..7b95a87 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -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

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Linus Torvalds]

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

On Sun, Aug 24, 2014 at 12:47 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> __restore_xstate_sig() calls math_state_restore() with preemption
> enabled, not good. But this is minor, [...]

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

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.

On 08/24, Linus Torvalds wrote:
>
> 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.

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.

> Also, you can actually end up with multiple threads pointing to the
> same math state in init_task.thread.fpu.state, right?

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.

> Why is that any
> better than just having the save state temporarily contain garbage?

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.

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

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.

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

> fix Suresh's email...

(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.

On 08/24, Linus Torvalds wrote:
>
> 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.

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.

> Also, you can actually end up with multiple threads pointing to the
> same math state in init_task.thread.fpu.state, right?

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.

> Why is that any
> better than just having the save state temporarily contain garbage?

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.

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

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.

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Linus Torvalds]

On Mon, Aug 25, 2014 at 7:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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.

.. 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

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

On 08/25, Linus Torvalds wrote:
>
> On Mon, Aug 25, 2014 at 7:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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.
>
> .. but if that's the case, then what was wrong with the old code

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().

> that
> just copied the state over the unused space from the user space
> buffer?

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().

> 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?

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.

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Linus Torvalds]

On Mon, Aug 25, 2014 at 10:09 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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().

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

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

On 08/25, Linus Torvalds wrote:
>
> On Mon, Aug 25, 2014 at 10:09 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > 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().
>
> 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.

OK.

> The signal handling overhead of floating point restore is not small,
> and it's not theoretical.

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.

[PATCH] x86, fpu: __restore_xstate_sig()->math_state_restore() needs preempt_disable()

[Oleg Nesterov]

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.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/xsave.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
index 453343c..c52eb9c 100644
--- a/arch/x86/kernel/xsave.c
+++ b/arch/x86/kernel/xsave.c
@@ -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

Re: [PATCH 2/5] x86, fpu: don't drop_fpu() in __restore_xstate_sig() if use_eager_fpu()

[Oleg Nesterov]

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

On 08/25, Oleg Nesterov wrote:
>
> 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.

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.

Re: [PATCH] x86, fpu: __restore_xstate_sig()->math_state_restore() needs preempt_disable()

[Suresh Siddha]

On Mon, Aug 25, 2014 at 11:08 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  arch/x86/kernel/xsave.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/kernel/xsave.c b/arch/x86/kernel/xsave.c
> index 453343c..c52eb9c 100644
> --- a/arch/x86/kernel/xsave.c
> +++ b/arch/x86/kernel/xsave.c
> @@ -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 {
>

oops. looks good to me.

Reviewed-by: Suresh Siddha <sbsiddha@gmail.com>