x86, fpu: copy_process's FPU paths cleanups

Hello,

Who can review this? And where should I send FPU changes?

And it seems that nobody cares about 2 fixes I sent before. Linus, I understand that you won't take them into v3.17, but perhaps you can ack/nack them explicitly? It seems that nobody can do this.

Oleg.

 arch/x86/include/asm/fpu-internal.h |    2 +-
 arch/x86/kernel/process.c           |   16 +++++++++-------
 arch/x86/kernel/process_32.c        |    2 --
 arch/x86/kernel/process_64.c        |    1 -
 4 files changed, 10 insertions(+), 11 deletions(-)

__thread_fpu_begin() checks X86_FEATURE_EAGER_FPU by hand, we have a helper for that.

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

index b0537b9..4cbb40e 100644
@@ -344,7 +344,7 @@ static inline void __thread_fpu_end(struct task_struct *tsk)
 
 static inline void __thread_fpu_begin(struct task_struct *tsk)
 {
-	if (!static_cpu_has_safe(X86_FEATURE_EAGER_FPU))
+	if (!use_eager_fpu())
 		clts();
 	__thread_set_has_fpu(tsk);
 }

1.5.5.1

arch_dup_task_struct() copies thread.fpu if fpu_allocated(), this looks suboptimal and misleading. Say, a forking process could use FPU only once in a signal handler but now tsk_used_math(src) == F, in this case the child gets a copy of fpu->state for no reason. The child won't use the saved registers anyway even if it starts to use FPU, this can only avoid fpu_alloc() in do_device_not_available().

Change this code to check tsk_used_math(current) instead. We still need to clear fpu->has_fpu/state, we could do this memset(0) under fpu_allocated() check but I think this doesn't make sense. See also the next change.

use_eager_fpu() assumes that fpu_allocated() is always true, but a forking task (and thus its child) must always have PF_USED_MATH set, otherwise the child can either use FPU without used_math() (note that switch_fpu_prepare() doesn't do stts() in this case), or it will be killed by do_device_not_available()->BUG_ON(use_eager_fpu).

---
 arch/x86/kernel/process.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

index 08c5ded..60076b4 100644
@@ -64,14 +64,13 @@ EXPORT_SYMBOL_GPL(task_xstate_cachep);
  */
 int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
-	int ret;
-
 	*dst = *src;
-	if (fpu_allocated(&src->thread.fpu)) {
-		memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
-		ret = fpu_alloc(&dst->thread.fpu);
-		if (ret)
-			return ret;
+
+	memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
+	if (tsk_used_math(src)) {
+		int err = fpu_alloc(&dst->thread.fpu);
+		if (err)
+			return err;
 		fpu_copy(dst, src);
 	}
 	return 0;

1.5.5.1

Cosmetic, but I think thread.fpu_counter should be initialized in arch_dup_task_struct() too, along with other "fpu" variables. And probably it make sense to turn it into thread.fpu->counter.

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

index 36dfb1d..7474f56 100644
@@ -66,6 +66,7 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
 	*dst = *src;
 
+	dst->thread.fpu_counter = 0;
 	dst->thread.fpu.has_fpu = 0;
 	dst->thread.fpu.last_cpu = ~0;
 	dst->thread.fpu.state = NULL;

index 7bc86bb..c73b3c1 100644
@@ -152,7 +152,6 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 		childregs->orig_ax = -1;
 		childregs->cs = __KERNEL_CS | get_kernel_rpl();
 		childregs->flags = X86_EFLAGS_IF | X86_EFLAGS_FIXED;
-		p->thread.fpu_counter = 0;
 		p->thread.io_bitmap_ptr = NULL;
 		memset(p->thread.ptrace_bps, 0, sizeof(p->thread.ptrace_bps));
 		return 0;

@@ -165,7 +164,6 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.ip = (unsigned long) ret_from_fork;
 	task_user_gs(p) = get_user_gs(current_pt_regs());
 
-	p->thread.fpu_counter = 0;
 	p->thread.io_bitmap_ptr = NULL;
 	tsk = current;
 	err = -ENOMEM;

index ca5b02d..593257d 100644
@@ -163,7 +163,6 @@ int copy_thread(unsigned long clone_flags, unsigned long sp,
 	p->thread.sp = (unsigned long) childregs;
 	p->thread.usersp = me->thread.usersp;
 	set_tsk_thread_flag(p, TIF_FORK);
-	p->thread.fpu_counter = 0;
 	p->thread.io_bitmap_ptr = NULL;
 
 	savesegment(gs, p->thread.gsindex);

1.5.5.1

Cosmetic, but imho memset(&dst->thread.fpu, 0) is not good simply because it hides the (important) usage of ->has_fpu/etc from grep. Change this code to initialize the members explicitly.

And note that ->last_cpu = 0 looks simply wrong, this can confuse fpu_lazy_restore() if per_cpu(fpu_owner_task, 0) has already exited and copy_process() re-allocated the same task_struct. Fortunately this is not actually possible because child->fpu_counter == 0 and thus fpu_lazy_restore() will not be called, but still this is not clean/robust.

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

index 60076b4..36dfb1d 100644
@@ -66,7 +66,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 {
 	*dst = *src;
 
-	memset(&dst->thread.fpu, 0, sizeof(dst->thread.fpu));
+	dst->thread.fpu.has_fpu = 0;
+	dst->thread.fpu.last_cpu = ~0;
+	dst->thread.fpu.state = NULL;
 	if (tsk_used_math(src)) {
 		int err = fpu_alloc(&dst->thread.fpu);
 		if (err)

1.5.5.1

Oleg, this is unacceptable.

Last week was Kernel Summit and that was right on the heels of a merge window. We are backlogged like crazy and being rude doesn't help one iota.

Sent from my mobile phone.  Please pardon brevity and lack of formatting.

FWIW, I have nothing against this series (or, indeed, the last series with the exception of 2/5 that got replaced by just the preemption disable).

Although with the whole i387 state I always hope somebody else checks it too, and it's obviously not 3.17 material any more (possibly with the exception of the afore-mentioned preemption patch, although even that might be better off as 3.18 + stable)

Linus

Also, the previous set of patches was under active review by Linus with objections, we very much wanted to wait for all that to conclude one way or another.

Thanks,

Ingo

Me? Rude??

I really hope that this is the first (and last) email from me which can be interpreted this way ;)

Trust me this was not intentional, and I am sorry if it looked like it.

OK, thanks. (that last series needs more work, kernel_fpu_begin/end).

Of course! And I do not know whom else I should ask to review, I simply do not know who might be interested.

Yes, sure.

OK.

But this all is not that important, I started to spam you just because I tried to understand this code and came to conclusion it deserves some cleanups.

My real concern is the first fix. Because this bug was not foung by me, Bean actually hit this bug. I attached it below just in case. If I resend it I'll optimistically assume that you do not see any problem in this patch too.

Thanks,

Oleg.

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

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

Sorry for confusion. But this series and the previous fixes do not depend on that set of patches.

Oleg.

Let me first try to avoid another angry email.

Sure, I understand that it is easy to say "I do not understand your code, do not know how to test, please review".

But what else can I do if I believe it is buggy? IOW, please consider this as the questions, not the patches. Better yet, as a bug report. Feel free to ignore if you are busy or you are not interested enough.

But of course, I'll appreciate any comment very much.

And to remind, rightly or not I believe they need changes anyway.

Linus, according to git-log/blame you understand these paths better than anybody else. I hope you can take a look.

Not for inclusion of course, although I hope that 1-2 and perhaps 3 make sense anyway.

Oleg.

interrupted_kernel_fpu_idle() tries to detect if kernel_fpu_begin() is safe or not. In particulat it should obviously deny the nested kernel_fpu_begin() and this logic doesn't look clean.

If use_eager_fpu() == T we rely on a) __thread_has_fpu() check in interrupted_kernel_fpu_idle(), and b) on the fact that _begin() does __thread_clear_has_fpu().

Otherwise we demand that the interrupted task has no FPU if it is in kernel mode, this works becase __kernel_fpu_begin() does clts().

Add the per-cpu "bool in_kernel_fpu" variable, and change this code to check/set/clear it. This allows to do some cleanups (see the next changes) and fixes.

Note that the current code looks racy. Say, kernel_fpu_begin() right after math_state_restore()->__thread_fpu_begin() will overwrite the regs we are going to restore. This patch doesn't even try to fix this, it just adds the comment, but "in_kernel_fpu" can also be used to implement kernel_fpu_disable() / kernel_fpu_enable().

---
 arch/x86/include/asm/i387.h |    2 +-
 arch/x86/kernel/i387.c      |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

index ed8089d..5e275d3 100644
@@ -40,8 +40,8 @@ extern void __kernel_fpu_end(void);
 
 static inline void kernel_fpu_begin(void)
 {
-	WARN_ON_ONCE(!irq_fpu_usable());
 	preempt_disable();
+	WARN_ON_ONCE(!irq_fpu_usable());
 	__kernel_fpu_begin();
 }
 

index d5dd808..8fb8868 100644
@@ -19,6 +19,8 @@
 #include <asm/fpu-internal.h>
 #include <asm/user.h>
 
+static DEFINE_PER_CPU(bool, in_kernel_fpu);
+
 /*
  * Were we in an interrupt that interrupted kernel mode?
  *

@@ -33,6 +35,9 @@
  */
 static inline bool interrupted_kernel_fpu_idle(void)
 {
+	if (this_cpu_read(in_kernel_fpu))
+		return false;
+
 	if (use_eager_fpu())
 		return __thread_has_fpu(current);
 

@@ -73,6 +78,9 @@ void __kernel_fpu_begin(void)
 {
 	struct task_struct *me = current;
 
+	this_cpu_write(in_kernel_fpu, true);
+
+	/* FIXME: race with math_state_restore()-like code */
 	if (__thread_has_fpu(me)) {
 		__thread_clear_has_fpu(me);
 		__save_init_fpu(me);

@@ -99,6 +107,8 @@ void __kernel_fpu_end(void)
 	} else {
 		stts();
 	}
+
+	this_cpu_write(in_kernel_fpu, false);
 }
 EXPORT_SYMBOL(__kernel_fpu_end);
 

1.5.5.1

Now that we have in_kernel_fpu we can remove __thread_clear_has_fpu() in __kernel_fpu_begin(). And this allow to replace the asymmetrical and nontrivial use_eager_fpu+tsk_used_math check in kernel_fpu_end() with the same __thread_has_fpu().

The logic becomes really simple afaics: if _begin() does save() then _end() needs restore(), and this is controlled by __thread_has_fpu().

Also, with this patch __kernel_fpu_end() does restore_fpu_checking() and WARNs if it fails instead of math_state_restore(). I think this looks better because we no longer need __thread_fpu_begin(), and it would be better to report the failure in this case.

---
 arch/x86/kernel/i387.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

index 8fb8868..19dd36d 100644
@@ -82,9 +82,7 @@ void __kernel_fpu_begin(void)
 
 	/* FIXME: race with math_state_restore()-like code */
 	if (__thread_has_fpu(me)) {
-		__thread_clear_has_fpu(me);
 		__save_init_fpu(me);
-		/* We do 'stts()' in __kernel_fpu_end() */
 	} else if (!use_eager_fpu()) {
 		this_cpu_write(fpu_owner_task, NULL);
 		clts();

@@ -94,17 +92,12 @@ EXPORT_SYMBOL(__kernel_fpu_begin);
 
 void __kernel_fpu_end(void)
 {
-	if (use_eager_fpu()) {
-		/*
-		 * For eager fpu, most the time, tsk_used_math() is true.
-		 * Restore the user math as we are done with the kernel usage.
-		 * At few instances during thread exit, signal handling etc,
-		 * tsk_used_math() is false. Those few places will take proper
-		 * actions, so we don't need to restore the math here.
-		 */
-		if (likely(tsk_used_math(current)))
-			math_state_restore();
-	} else {
+	struct task_struct *me = current;
+
+	if (__thread_has_fpu(me)) {
+		if (WARN_ON(restore_fpu_checking(me)))
+			drop_init_fpu(me);
+	} else if (!use_eager_fpu()) {
 		stts();
 	}
 

1.5.5.1

According to the comment and the changelog in 5187b28f "x86: Allow FPU to be used at interrupt time even with eagerfpu", the __thread_has_fpu() check was added to avoid the nested kernel_fpu_begin(). Now that we have in_kernel_fpu we can remove this check and always return true.

__thread_has_fpu() can be false even if use_eager_fpu(), but this case doesn't differ from !use_eager_fpu() case except we should not worry about X86_CR0_TS, __kernel_fpu_begin/end will not touch this bit. And I still think that "use_eager_fpu && (!__thread_has_fpu || !used_math)" special cases should die, but this is off-topic right now.

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

index 19dd36d..9fb2899 100644
@@ -30,8 +30,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
  * be set (so that the clts/stts pair does nothing that is
  * visible in the interrupted kernel thread).
  *
- * Except for the eagerfpu case when we return 1 unless we've already
- * been eager and saved the state in kernel_fpu_begin().
+ * Except for the eagerfpu case when we return 1.
  */
 static inline bool interrupted_kernel_fpu_idle(void)
 {

@@ -39,7 +38,7 @@ static inline bool interrupted_kernel_fpu_idle(void)
 		return false;
 
 	if (use_eager_fpu())
-		return __thread_has_fpu(current);
+		return true;
 
 	return !__thread_has_fpu(current) &&
 		(read_cr0() & X86_CR0_TS);

1.5.5.1

ONCE AGAIN, THIS IS MORE THE QUESTION THAN THE PATCH.

interrupted_kernel_fpu_idle() does:

	if (use_eager_fpu())
		return true;

	return !__thread_has_fpu(current) &&
		(read_cr0() & X86_CR0_TS);

and it is absolutely not clear why these 2 cases differ so much.

To remind, the use_eager_fpu() case is buggy; __save_init_fpu() in __kernel_fpu_begin() can race with math_state_restore() which does __thread_fpu_begin() + restore_fpu_checking(). So we should fix this race anyway and we can't require __thread_has_fpu() == F likes the !use_eager_fpu() case does, in this case kernel_fpu_begin() will not work if it interrupts the idle thread (this will reintroduce the performance regression fixed by 5187b28f).

Probably math_state_restore() can use kernel_fpu_disable/end() which sets/clears in_kernel_fpu, or it can disable irqs. Doesn't matter, we should fix this bug anyway.

And if we fix this bug, why else !use_eager_fpu() case needs the much more strict check? Why we can't handle the __thread_has_fpu(current) case the same way?

The comment deleted by this change says:

and TS must be set so that the clts/stts pair does nothing

and can explain the difference, but I can not understand this (again, assuming that we fix the race(s) mentoined above).

Say, user_fpu_begin(). Yes, kernel_fpu_begin/end() can restore X86_CR0_TS. But this should be fine? A context switch before restore_user_xstate() can equally set it back? And device_not_available() should be fine even in kernel context?

I'll appreciate any comment.

---
 arch/x86/kernel/i387.c |   44 +-------------------------------------------
 1 files changed, 1 insertions(+), 43 deletions(-)

index 9fb2899..ef60f33 100644
@@ -22,54 +22,12 @@
 static DEFINE_PER_CPU(bool, in_kernel_fpu);
 
 /*
- * Were we in an interrupt that interrupted kernel mode?
- *
- * On others, we can do a kernel_fpu_begin/end() pair *ONLY* if that
- * pair does nothing at all: the thread must not have fpu (so
- * that we don't try to save the FPU state), and TS must
- * be set (so that the clts/stts pair does nothing that is
- * visible in the interrupted kernel thread).
- *
- * Except for the eagerfpu case when we return 1.
- */
-static inline bool interrupted_kernel_fpu_idle(void)
-{
-	if (this_cpu_read(in_kernel_fpu))
-		return false;
-
-	if (use_eager_fpu())
-		return true;
-
-	return !__thread_has_fpu(current) &&
-		(read_cr0() & X86_CR0_TS);
-}
-
-/*
- * Were we in user mode (or vm86 mode) when we were
- * interrupted?
- *
- * Doing kernel_fpu_begin/end() is ok if we are running
- * in an interrupt context from user mode - we'll just
- * save the FPU state as required.
- */
-static inline bool interrupted_user_mode(void)
-{
-	struct pt_regs *regs = get_irq_regs();
-	return regs && user_mode_vm(regs);
-}
-
-/*
  * Can we use the FPU in kernel mode with the
  * whole "kernel_fpu_begin/end()" sequence?
- *
- * It's always ok in process context (ie "not interrupt")
- * but it is sometimes ok even from an irq.
  */
 bool irq_fpu_usable(void)
 {
-	return !in_interrupt() ||
-		interrupted_user_mode() ||
-		interrupted_kernel_fpu_idle();
+	return !this_cpu_read(in_kernel_fpu);
 }
 EXPORT_SYMBOL(irq_fpu_usable);
 

1.5.5.1

These 4 patches also look good to me.

yes indeed, explicit calls to math_state_restore() in eager_fpu case has this race. I guess this is present from the commit 5187b28f.

thanks,
suresh

this patch I think needs more thought for sure. please see below.

No. The reason is that has_fpu state and cr0.TS can get out of sync.

Let's say you get an interrupt after clts() in __thread_fpu_begin() called as part of user_fpu_begin().

And because of this proposed change, irq_fpu_usable() returns true and an interrupt can end-up using fpu and after the return from interrupt we can have a state where cr0.TS is set but we end up resuming the execution from __thread_set_has_fpu(). Now after this point has_fpu is set but cr0.TS is set. And now any schedule() with this state (let's say immd after preemption_enable() at the end of user_fpu_begin()) is dangerous. We can get a dna fault in the middle of __switch_to() which can lead to subtle bugs.

not in some critical places like switch_to().

other than this patch, rest of the changes look ok to me. Can you please resend this patchset with the math_state_restore() race addressed aswell?

thanks,
suresh

Of course.

Thanks. Yes, I thought about this race, but I didn't realize that a DNA inside __switch_to() is dangerous.

Thanks a lot. OK, this is trivially fixable. I had this v2 in mind from the very beginning because I was not really sure that unconditional stts() in kernel_fpu_end() is safe (but yes, I didn't realize why). Just we need to save X86_CR0_TS in the same per-cpu in_kernel_fpu,

	static DEFINE_PER_CPU(int, in_kernel_fpu);

	void __kernel_fpu_begin(void)
	{
		struct task_struct *me = current;

		this_cpu_write(in_kernel_fpu, 1);

		if (__thread_has_fpu(me)) {
			__save_init_fpu(me);
		} else if (!use_eager_fpu()) {
			this_cpu_write(fpu_owner_task, NULL);
			if ((read_cr0() & X86_CR0_TS))
				this_cpu_write(in_kernel_fpu, 2);
				clts();
			}
		}
	}

	void __kernel_fpu_end(void)
	{
		struct task_struct *me = current;

		if (__thread_has_fpu(me)) {
			if (WARN_ON(restore_fpu_checking(me)))
				drop_init_fpu(me);
		} else if (!use_eager_fpu()) {
			if (this_cpu_read(in_kernel_fpu) == 2)
				stts();
		}

		this_cpu_write(in_kernel_fpu, 0);
	}

Or, perhaps better, we can turn user_fpu_begin()->preempt_disable() into kernel_fpu_disable().

Do you think this can work?

OK, will do, but probably next week.

Will you agree if I add kernel_fpu_disable/enable to fix this race? It can probably have more users (see above).

Thanks!

Oleg.

Sorry for noise, but just in case...

Of course, even if this can work, we should do this later. Let's start with the simple changes, then we will see if we can actually remove all other checks.

Oleg.