[PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Oleg Nesterov]

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

[PATCH 1/4] x86, fpu: change __thread_fpu_begin() to use use_eager_fpu()

[Oleg Nesterov]

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

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

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

[PATCH 2/4] x86, fpu: copy_process: avoid fpu_alloc/copy if !used_math()

[Oleg Nesterov]

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

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

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 08c5ded..60076b4 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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

[PATCH 3/4] x86, fpu: copy_process: sanitize fpu->last_cpu initialization

[Oleg Nesterov]

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.

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

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 60076b4..36dfb1d 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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

[PATCH 4/4] x86, fpu: shift "fpu_counter = 0" from copy_thread() to arch_dup_task_struct()

[Oleg Nesterov]

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.

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

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 36dfb1d..7474f56 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -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;
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index 7bc86bb..c73b3c1 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -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;
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index ca5b02d..593257d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[H. Peter Anvin]

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.

On August 27, 2014 11:51:38 AM PDT, Oleg Nesterov <oleg@redhat.com> wrote:
>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(-)

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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Linus Torvalds]

On Wed, Aug 27, 2014 at 11:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> Who can review this? And where should I send FPU changes?

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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

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

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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Oleg Nesterov]

On 08/27, H. Peter Anvin wrote:
>
> 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.

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.

> On August 27, 2014 11:51:38 AM PDT, Oleg Nesterov <oleg@redhat.com> wrote:
> >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(-)
> 
> -- 
> Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Oleg Nesterov]

On 08/27, Linus Torvalds wrote:
>
> On Wed, Aug 27, 2014 at 11:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Who can review this? And where should I send FPU changes?
>
> 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).

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

> Although with the whole i387 state I always hope somebody else checks
> it too,

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

> and it's obviously not 3.17 material any more

Yes, sure.

> (possibly with
> the exception of the afore-mentioned preemption patch, although even
> that might be better off as 3.18 + stable)

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

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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Oleg Nesterov]

On 08/28, Ingo Molnar wrote:
>
> * H. Peter Anvin <hpa@zytor.com> wrote:
>
> > 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.
>
> 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.

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

Oleg.

[PATCH 0/4] x86, fpu: kernel_fpu_begin/end cleanups

[Oleg Nesterov]

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.

On 08/28, Oleg Nesterov wrote:
>
> On 08/27, Linus Torvalds wrote:
> >
> > On Wed, Aug 27, 2014 at 11:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > Who can review this? And where should I send FPU changes?
> >
> > 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).
>
> OK, thanks. (that last series needs more work, kernel_fpu_begin/end).

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.

[PATCH 1/4] x86, fpu: introduce per-cpu "bool in_kernel_fpu"

[Oleg Nesterov]

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

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 arch/x86/include/asm/i387.h |    2 +-
 arch/x86/kernel/i387.c      |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
index ed8089d..5e275d3 100644
--- a/arch/x86/include/asm/i387.h
+++ b/arch/x86/include/asm/i387.h
@@ -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();
 }
 
diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index d5dd808..8fb8868 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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

[PATCH 2/4] x86, fpu: don't abuse ->has_fpu in __kernel_fpu_begin/end

[Oleg Nesterov]

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.

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

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 8fb8868..19dd36d 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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

[PATCH 3/4] x86, fpu: irq_fpu_usable: always return true if use_eager_fpu()

[Oleg Nesterov]

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.

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

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 19dd36d..9fb2899 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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

[PATCH 4/4] x86, fpu: irq_fpu_usable: kill all checks except !in_kernel_fpu

[Oleg Nesterov]

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

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index 9fb2899..ef60f33 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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

Re: [PATCH 0/4] x86, fpu: copy_process's FPU paths cleanups

[Suresh Siddha]

On Wed, Aug 27, 2014 at 11:51 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> 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(-)

These 4 patches also look good to me.

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

Re: [PATCH 1/4] x86, fpu: introduce per-cpu "bool in_kernel_fpu"

[Suresh Siddha]

On Fri, Aug 29, 2014 at 11:16 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> 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,

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


> it just adds the comment, but "in_kernel_fpu" can also be used to
> implement kernel_fpu_disable() / kernel_fpu_enable().
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  arch/x86/include/asm/i387.h |    2 +-
>  arch/x86/kernel/i387.c      |   10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/arch/x86/include/asm/i387.h b/arch/x86/include/asm/i387.h
> index ed8089d..5e275d3 100644
> --- a/arch/x86/include/asm/i387.h
> +++ b/arch/x86/include/asm/i387.h
> @@ -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();
>  }
>
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index d5dd808..8fb8868 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -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
>
>

Re: [PATCH 4/4] x86, fpu: irq_fpu_usable: kill all checks except !in_kernel_fpu

[Suresh Siddha]

On Fri, Aug 29, 2014 at 11:17 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> ONCE AGAIN, THIS IS MORE THE QUESTION THAN THE PATCH.

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

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

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.


> A context switch before restore_user_xstate()
> can equally set it back?
> And device_not_available() should be fine even
> in kernel context?

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

>
> I'll appreciate any comment.
> ---
>  arch/x86/kernel/i387.c |   44 +-------------------------------------------
>  1 files changed, 1 insertions(+), 43 deletions(-)
>
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 9fb2899..ef60f33 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -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
>
>

Re: [PATCH 4/4] x86, fpu: irq_fpu_usable: kill all checks except !in_kernel_fpu

[Oleg Nesterov]

On 09/02, Suresh Siddha wrote:
>
> On Fri, Aug 29, 2014 at 11:17 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > ONCE AGAIN, THIS IS MORE THE QUESTION THAN THE PATCH.
>
> this patch I think needs more thought for sure. please see below.

Of course.

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

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?

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

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.

Re: [PATCH 4/4] x86, fpu: irq_fpu_usable: kill all checks except !in_kernel_fpu

[Oleg Nesterov]

Sorry for noise, but just in case...

On 09/02, Oleg Nesterov wrote:
>
> Do you think this can work?

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.