question about save_xstate_sig() - WHY DOES THIS WORK?

While working on a patch series to defer FPU state loading until kernel -> user space transition, and be more lazy with FPU state while in the kernel, I came across this code in save_xstate_sig().

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Specifically, save_user_xstate() may page fault and sleep. After returning from the page fault, there is no guarantee that the FPU state will be restored into the CPU, when the system is not running with eager fpu mode.

In that case, what prevents us from saving random FPU register state to the user's stack frame? Potentially state containing data from other programs...

Quoted 13 lines · Rik van Riel

While working on a patch series to defer FPU state loading until kernel -> user space transition, and be more lazy with FPU state while in the kernel, I came across this code in save_xstate_sig().

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Specifically, save_user_xstate() may page fault and sleep. After returning from the page fault, there is no guarantee that the FPU state will be restored into the CPU, when the system is not running with eager fpu mode.

Quoted 4 lines

In that case, what prevents us from saving random FPU register state to the user's stack frame? Potentially state containing data from other programs...

If the FPU state is not current, we'll have CR0.TS = 1 and the XSAVE will cause an #NM exception, which will cause the FPU state to be swapped in.

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Let me abuse this thread to ask more questions.

Peter, could you help?

Quoted 3 lines · Rik van Riel

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

As I already mentioned, at least math_error()->save_init_fpu() looks buggy. And unlazy_fpu() doesn't look right too.

Note that save_init_fpu() is calles after conditional_sti(), so unless I missed something the task can be preempted and we can actually hit WARN_ON_ONCE(!__thread_has_fpu()) if !use_eager_fpu() && .fpu_counter == 0.

Worse, the unconditional __save_init_fpu() is obviously wrong in this case.

I already have a patch which (like the patch from Rik) turns it into

static inline void save_init_fpu(struct task_struct *tsk) { preempt_disable(); if (__thread_has_fpu(tsk)) { if (use_eager_fpu()) { __save_fpu(tsk); } else { __save_init_fpu(tsk); __thread_fpu_end(tsk); } } preempt_enable(); }

Now the questions:

- This doesn't hurt, but does it really need __thread_fpu_end?

Perhaps this is because we do not check the error code returned by __save_init_fpu? although I am not sure I understand the comment above fpu_save_init correctly...

- What about do_bounds() ? Should not it use save_init_fpu() rather than fpu_save_init() ?

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

and note that in this case __thread_fpu_end() is wrong if use_eager_fpu, but fortunately the only possible caller of unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt __kernel_fpu_begin() from irq?

I mean, is it safe if __save_init_fpu() path is interrupted by another __save_init_fpu() + restore_fpu_checking() from __kernel_fpu_begin/end?

Quoted 4 lines · Oleg Nesterov

Now the questions:

- This doesn't hurt, but does it really need __thread_fpu_end?

Quoted 3 lines

Perhaps this is because we do not check the error code returned by __save_init_fpu? although I am not sure I understand the comment above fpu_save_init correctly...

Looking at the code some more, I do not see any call site of save_init_fpu() that actually needs or wants __thread_fpu_end(), with or without eager fpu mode.

It looks like we can get rid of that.

Quoted 2 lines

- What about do_bounds() ? Should not it use save_init_fpu() rather than fpu_save_init() ?

I suppose do_bounds() probably should save the fpu context while not preemptible,

but that may also mean moving conditional_sti()
until after save_init_fpu() or __save_init_fpu() has been called.

Quoted 10 lines

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

and note that in this case __thread_fpu_end() is wrong if use_eager_fpu, but fortunately the only possible caller of unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt __kernel_fpu_begin() from irq?

Quoted 3 lines

I mean, is it safe if __save_init_fpu() path is interrupted by another __save_init_fpu() + restore_fpu_checking() from __kernel_fpu_begin/end?

I got lost in the core dump code trying to figure out whether this is safe or broken. I'll need some more time to look through that code...

Quoted 7 lines · Rik van Riel

Quoted 4 lines · Oleg Nesterov

Now the questions:

- This doesn't hurt, but does it really need __thread_fpu_end?

Quoted 7 lines

Quoted 3 lines

Perhaps this is because we do not check the error code returned by __save_init_fpu? although I am not sure I understand the comment above fpu_save_init correctly...

Looking at the code some more, I do not see any call site of save_init_fpu() that actually needs or wants __thread_fpu_end(), with or without eager fpu mode.

Yes. But probably it is needed if __save_init_fpu() returns 0. But this is minor, __thread_fpu_end() doesn't hurt correctness-wise if !eager.

Quoted 1 line

It looks like we can get rid of that.

Agreed, but probably this needs a separate change.

Quoted 5 lines

Quoted 2 lines

- What about do_bounds() ? Should not it use save_init_fpu() rather than fpu_save_init() ?

I suppose do_bounds() probably should save the fpu context while not preemptible,

plus it also needs the __thread_has_fpu() check. Otherwise fpu_save_init() can save the wrong FPU state afaics.

Quoted 2 lines

but that may also mean moving conditional_sti() until after save_init_fpu() or __save_init_fpu() has been called.

Agreed, this can work too.

Quoted 9 lines

Quoted 9 lines

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

and note that in this case __thread_fpu_end() is wrong if use_eager_fpu, but fortunately the only possible caller of unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt
__kernel_fpu_begin() from irq?

Quoted 7 lines

Quoted 3 lines

I mean, is it safe if __save_init_fpu() path is interrupted by another __save_init_fpu() + restore_fpu_checking() from __kernel_fpu_begin/end?

I got lost in the core dump code trying to figure out whether this is safe or broken. I'll need some more time to look through that code...

It is called indirectly by regset code, see
xstateregs_get()->init_fpu().

The coredumping task can't return to user-mode and use FPU in this case, so this is not that bad. Still unlazy_fpu()->__thread_fpu_end() is wrong if eager.

And I forgot to mention, the "else" branch in unlazy_fpu() makes no sense. And note that save_init_fpu() and unlazy_fpu() is the same thing (if we fix/cleanup them).

Oh. I'll try to finish my cleanups and send them tomorrow. Unless you do this ;)

Quoted 9 lines · Oleg Nesterov

Quoted 9 lines

Quoted 9 lines

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

and note that in this case __thread_fpu_end() is wrong if use_eager_fpu, but fortunately the only possible caller of unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt
__kernel_fpu_begin() from irq?

It looks like it should be safe, as long as __save_init_fpu() knows that the task no longer has the FPU after __kernel_fpu_end(), so it does not try to save the kernel FPU state to the user's task->thread.fpu.state->xstate

The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end() needs to be kept from running during unlazy_fpu().

This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need to check whether preemption is disabled, and lock out __kernel_fpu_begin() when preemption is disabled.

It does not look like it currently does that...

Quoted 3 lines · Oleg Nesterov

Quoted 3 lines

Quoted 3 lines

- Is unlazy_fpu()->__save_init_fpu() safe wrt
__kernel_fpu_begin() from irq?

It looks like it should be safe, as long as __save_init_fpu() knows that the task no longer has the FPU after __kernel_fpu_end(), so it does not try to save the kernel FPU state to the user's task->thread.fpu.state->xstate

The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end() needs to be kept from running during unlazy_fpu().

This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need to check whether preemption is disabled, and lock out __kernel_fpu_begin() when preemption is disabled.

I can certainly merge unlazy_fpu() and save_init_fpu() into the same function, but I am not sure whether or not it should call __thread_fpu_end() - it looks like that would be desirable in some cases, but not in others...

Quoted 10 lines · Rik van Riel

Quoted 3 lines · Oleg Nesterov

Quoted 3 lines

Quoted 3 lines

- Is unlazy_fpu()->__save_init_fpu() safe wrt
__kernel_fpu_begin() from irq?

It looks like it should be safe, as long as __save_init_fpu() knows that the task no longer has the FPU after __kernel_fpu_end(), so it does not try to save the kernel FPU state to the user's task->thread.fpu.state->xstate

Not sure this is enough, but...

Quoted 2 lines

The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end() needs to be kept from running during unlazy_fpu().

Yes,

Quoted 3 lines

This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need to check whether preemption is disabled, and lock out __kernel_fpu_begin() when preemption is disabled.

But we already have kernel_fpu_disable/enable. unlazy_cpu() can use it to avoid the race ?

Quoted 4 lines

I can certainly merge unlazy_fpu() and save_init_fpu() into the same function, but I am not sure whether or not it should call __thread_fpu_end() - it looks like that would be desirable in some cases, but not in others...

I _think_ that we never actually want __thread_fpu_end(), although it doesn't really hurt if !eager. Probably ulazy/save should do

	if (!__save_init_fpu())
		__thread_fpu_end();

Quoted 24 lines · Rik van Riel

Quoted 9 lines · Oleg Nesterov

Quoted 9 lines

Quoted 9 lines

- Why unlazy_fpu() always does __save_init_fpu() even if use_eager_fpu?

and note that in this case __thread_fpu_end() is wrong if use_eager_fpu, but fortunately the only possible caller of unlazy_fpu() is coredump. fpu_copy() checks use_eager_fpu().

- Is unlazy_fpu()->__save_init_fpu() safe wrt
__kernel_fpu_begin() from irq?

It looks like it should be safe, as long as __save_init_fpu() knows that the task no longer has the FPU after __kernel_fpu_end(), so it does not try to save the kernel FPU state to the user's task->thread.fpu.state->xstate

The caveat here is that __kernel_fpu_begin()/__kernel_fpu_end() needs to be kept from running during unlazy_fpu().

This means interrupted_kernel_fpu_idle and/or irq_fpu_usable need to check whether preemption is disabled, and lock out __kernel_fpu_begin() when preemption is disabled.

Quoted 1 line

It does not look like it currently does that...

... and that won't work, because preempt_disable() is a noop without CONFIG_PREEMPT enabled. Sigh.

Not sure how to work around that, except by having __Kernel_fpu_end() always restore the task FPU state, if the task had the FPU when entering.

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Quoted 2 lines · Rik van Riel

Quoted 2 lines

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Lets (try to) fix unlazy_fpu/save_init_fpu at least.

Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

The functions save_init_fpu and unlazy_fpu do essentially the same thing:

Callers of init_fpu do want __thread_fpu_end, so move the call to __thread_fpu_end into init_fpu.

static inline void save_init_fpu(struct task_struct *tsk) {

arch/x86/include/asm/fpu-internal.h +0 -5

-	WARN_ON_ONCE(!__thread_has_fpu(tsk));
-
-	if (use_eager_fpu()) {
-		__save_fpu(tsk);
-		return;

It is not clear why the "else" branch clears ->fpu_counter, this makes no sense.

If use_eager_fpu() then this has no effect. Otherwise, if we actually wanted to prevent fpu preload after the context switch we would need to reset it unconditionally, even if __thread_has_fpu().

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 3 lines · Oleg Nesterov

Quoted 2 lines · Rik van Riel

Quoted 2 lines

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Quoted 4 lines

Lets (try to) fix unlazy_fpu/save_init_fpu at least.

Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will enable eagerfpu.

It's a bit of an indirect way to avoid the WARN_ON() and probably needs a better comment.

unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu(). Unconditional __thread_fpu_end() is only correct if we know that this thread can't return to user-mode and use FPU.

Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(), and init_fpu(current) can be only called by the coredumping thread via regset->get(). But it is exported to modules, and imo this should be fixed anyway.

And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy() and save_init_fpu() do.

- It seems that even !use_eager_fpu() case doesn't need the unconditional __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.

- It is still not clear to me if __save_init_fpu() can safely nest with another save + restore from __kernel_fpu_begin(). If not, we can use kernel_fpu_disable() to fix the race.

Signed-off-by Oleg Nesterov <[email protected]>

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

arch/x86/kernel/i387.c +6 -2

index c3b92c0..8e070a6 100644
@@ -120,8 +120,12 @@ void unlazy_fpu(struct task_struct *tsk)
 {
 	preempt_disable();
 	if (__thread_has_fpu(tsk)) {
-		__save_init_fpu(tsk);
-		__thread_fpu_end(tsk);
+		if (use_eager_fpu()) {
+			__save_fpu(tsk);
+		} else {
+			__save_init_fpu(tsk);
+			__thread_fpu_end(tsk);
+		}
 	}
 	preempt_enable();
 }

Quoted 7 lines · Dave Hansen

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Rik van Riel

Quoted 2 lines

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Lets (try to) fix unlazy_fpu/save_init_fpu at least.

Quoted 5 lines

Quoted 2 lines

Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will enable eagerfpu.

unless eagerfpu=off? but this doesn't matter.

Quoted 8 lines · Oleg Nesterov

unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu(). Unconditional __thread_fpu_end() is only correct if we know that this thread can't return to user-mode and use FPU.

Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(), and init_fpu(current) can be only called by the coredumping thread via regset->get(). But it is exported to modules, and imo this should be fixed anyway.

What about xfpregs_set?

It looks like that code copies an entire FPU state in from userspace, and expects the kernel to start using that new FPU state.

This is called from the ptrace code.

When we switch to the traced task, the __thread_fpu_end() that was called from init_fpu() ensures that switch_fpu_begin() will actually load the new FPU state from memory into the registers, and we will not take the fpu_lazy_restore() branch.

What am I missing?

Yes. But in this case tsk != current, and we ensure that __switch_to() was finished. wait_task_inactive().

Quoted 5 lines

When we switch to the traced task, the __thread_fpu_end() that was called from init_fpu() ensures that switch_fpu_begin() will actually load the new FPU state from memory into the registers, and we will not take the fpu_lazy_restore() branch.

No. in this case we rely on "tsk->thread.fpu.last_cpu = ~0" which disables fpu_lazy_restore().

unlazy_fpu()->__thread_fpu_end() doesn't look right if use_eager_fpu(). Unconditional __thread_fpu_end() is only correct if we know that this thread can't return to user-mode and use FPU.

Fortunately it has only 2 callers. fpu_copy() checks use_eager_fpu(), and init_fpu(current) can be only called by the coredumping thread via regset->get(). But it is exported to modules, and imo this should be fixed anyway.

And if we check use_eager_fpu() we can use __save_fpu() like fpu_copy() and save_init_fpu() do.

- It seems that even !use_eager_fpu() case doesn't need the unconditional __thread_fpu_end(), we only need it if __save_init_fpu() returns 0.

- It is still not clear to me if __save_init_fpu() can safely nest with another save + restore from __kernel_fpu_begin(). If not, we can use kernel_fpu_disable() to fix the race.

Signed-off-by Oleg Nesterov <[email protected]>

math_error() calls save_init_fpu() after conditional_sti(), this means that the caller can be preempted. If !use_eager_fpu() we can hit the WARN_ON_ONCE(!__thread_has_fpu(tsk)) and/or save the wrong FPU state.

Change math_error() to use unlazy_fpu() and kill save_init_fpu().

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 14 lines · Oleg Nesterov

Quoted 12 lines · Dave Hansen

Quoted 7 lines · Oleg Nesterov

Quoted 2 lines · Rik van Riel

Quoted 2 lines

Not only is this broken with my new code, but it looks like it may be broken with the current code, too...

Lets (try to) fix unlazy_fpu/save_init_fpu at least.

Dave, fpu_save_init() in do_bounds() and task_get_bounds_dir() looks wrong too, shouldn't it use unlazy_fpu() ? See the changelog in 3/3.

IIRC, the 'cpu_has_xsaveopt' on the CPUs that support will MPX will enable eagerfpu.

Quoted 1 line

unless eagerfpu=off? but this doesn't matter.

Yeah, that's true. That would also explain why I haven't run in to this at all in testing.

Ugh, fpu_save_init() says it isn't preempt safe anyway, so we shouldn't be using it.

I'll send a fix.

How about the trivial patch below (on top of this series) ?

arch/x86/kernel/traps.c +1 -1

@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);
 	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);
 	if (!bndcsr)

arch/x86/mm/mpx.c +1 -1

@@ -352,7 +352,7 @@ static __user void *task_get_bounds_dir(
 	 * The bounds directory pointer is stored in a register
 	 * only accessible if we first do an xsave.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	bndcsr = get_xsave_addr(&tsk->thread.fpu.state->xsave, XSTATE_BNDCSR);
 	if (!bndcsr)
 		return MPX_INVALID_BOUNDS_DIR;

arch/x86/kernel/traps.c +1 -1

@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);

	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Quoted 6 lines · Oleg Nesterov

--- x/arch/x86/kernel/traps.c +++ x/arch/x86/kernel/traps.c @@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r * It is not directly accessible, though, so we need to * do an xsave and then pull it out of the xsave buffer. */ -
fpu_save_init(&tsk->thread.fpu); + unlazy_fpu(tsk); xsave_buf =
&(tsk->thread.fpu.state->xsave);

...

Quoted 1 line

bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Hmm, if the the thread was not using the FPU, and this fails to save anything in to the xsave_buf, what will bndcsr point to?

Quoted 9 lines · Oleg Nesterov

arch/x86/kernel/traps.c +1 -1

@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);

...

Quoted 1 line

	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Hmm, if the the thread was not using the FPU, and this fails to save anything in to the xsave_buf, what will bndcsr point to? It _looks_ to me like it will just point to uninitialized data since the xsave never happened.

Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against the xsave->xstate_bv?

Quoted 14 lines · Dave Hansen

Quoted 9 lines · Oleg Nesterov

arch/x86/kernel/traps.c +1 -1

@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);

...

Quoted 1 line

	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Quoted 7 lines

Hmm, if the the thread was not using the FPU, and this fails to save anything in to the xsave_buf, what will bndcsr point to? It _looks_ to me like it will just point to uninitialized data since the xsave never happened.

Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against the xsave->xstate_bv?

Can't really comment, but let me clarify what I meant just in case...

If it was not using FPU then I guess do_bounds() can't happen. However, it can be preempted after conditional_sti(). fpu_save_init() is obviously wrong unless __thread_has_fpu() == T, and this can be false if !eagerfpu or if we add TIF_LOAD_FPU (defer FPU restore until return to userspace).

Quoted 24 lines · Oleg Nesterov

Quoted 14 lines · Dave Hansen

Quoted 9 lines · Oleg Nesterov

arch/x86/kernel/traps.c +1 -1

@@ -313,7 +313,7 @@ dotraplinkage void do_bounds(struct pt_r
 	 * It is not directly accessible, though, so we need to
 	 * do an xsave and then pull it out of the xsave buffer.
 	 */
-	fpu_save_init(&tsk->thread.fpu);
+	unlazy_fpu(tsk);
 	xsave_buf = &(tsk->thread.fpu.state->xsave);

...

Quoted 1 line

	bndcsr = get_xsave_addr(xsave_buf, XSTATE_BNDCSR);

Quoted 7 lines

Hmm, if the the thread was not using the FPU, and this fails to save anything in to the xsave_buf, what will bndcsr point to? It _looks_ to me like it will just point to uninitialized data since the xsave never happened.

Fenghua, shouldn't get_xsave_addr() be checking the xstate bit against the xsave->xstate_bv?

Quoted 6 lines

Can't really comment, but let me clarify what I meant just in case...

If it was not using FPU then I guess do_bounds() can't happen. However, it can be preempted after conditional_sti(). fpu_save_init() is obviously wrong unless __thread_has_fpu() == T, and this can be false if !eagerfpu or if we add TIF_LOAD_FPU (defer FPU restore until return to userspace).

Forgot to mention... and if we use unlazy_fpu() we should not worry about preemption/__thread_has_fpu, we can rely on __save_init_fpu() in __switch_to() and/or __thread_has_fpu() check in unlazy_fpu().

But I am afraid I misunderstood your concerns, sorry for noise in this case.

Quoted 6 lines · On 01/30/2015 09:49 AM, [email protected] wrote:

(quoted diff) +0 -0

@@ -440,8 +440,9 @@ static inline fpu_switch_t

switch_fpu_prepare(struct task_struct *old, struct ta new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
(!__save_init_fpu(old)) - cpu = ~0; - old->thread.fpu.last_cpu
= cpu; + task_disable_lazy_fpu_restore(old); + else +
old->thread.fpu.last_cpu = cpu;

What is the 'else' doing here? It seems a bit disconnected from the other parts of the patch.

Quoted 6 lines · On 01/30/2015 09:49 AM, [email protected] wrote:

(quoted diff) +0 -0

@@ -440,8 +440,9 @@ static inline fpu_switch_t

switch_fpu_prepare(struct task_struct *old, struct ta new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
(!__save_init_fpu(old)) - cpu = ~0; - old->thread.fpu.last_cpu
= cpu; + task_disable_lazy_fpu_restore(old); + else +
old->thread.fpu.last_cpu = cpu;

What is the 'else' doing here? It seems a bit disconnected from the other parts of the patch.

Quoted 2 lines

Quoted 2 lines

Hmm, if the the thread was not using the FPU, and this fails to save anything in to the xsave_buf, what will bndcsr point to?

If the thread was not using the FPU, can we reach the bound range exception?

I believe the MPX feature uses information in the xstate.

From: Rik van Riel <[email protected]>

fpu.preload only gets set if new->thread.fpu_counter is already larger than 5. Incrementing it further does absolutely nothing. Remove those lines.

arch/x86/include/asm/fpu-internal.h +0 -2

@@ -447,7 +447,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 
 		/* Don't change CR0.TS if we just switch! */
 		if (fpu.preload) {
-			new->thread.fpu_counter++;
 			__thread_set_has_fpu(new);
 			prefetch(new->thread.fpu.state);
 		} else if (!use_eager_fpu())

@@ -456,7 +455,6 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 		old->thread.fpu_counter = 0;
 		task_disable_lazy_fpu_restore(old);
 		if (fpu.preload) {
-			new->thread.fpu_counter++;
 			if (fpu_lazy_restore(new, cpu))
 				fpu.preload = 0;
 			else

Signature · 1 line

1.9.3

Quoted 6 lines · On 02/02, [email protected] wrote:

From: Rik van Riel <[email protected]>

fpu.preload only gets set if new->thread.fpu_counter is already larger than 5. Incrementing it further does absolutely nothing. Remove those lines.

I _think_ that we increment it further on purpose. Note that fpu_counter is "char", so it seems that we want no more than 256 automatic preloads.

So I am not sure about this change. At least the changelog doesn't look right.

From: Rik van Riel <[email protected]>

With Oleg's patch "x86, fpu: don't abuse FPU in kernel threads if use_eager_fpu()", kernel threads no longer have an FPU state even on systems with use_eager_fpu()

That in turn means that a task may still have its FPU state loaded in the FPU registers, if the task only got interrupted by kernel threads from when it went to sleep, to when it woke up again.

In that case, there is no need to restore the FPU state for this task, since it is still in the registers.

The kernel can simply use the same logic to determine this as is used for !use_eager_fpu() systems.

arch/x86/include/asm/fpu-internal.h +1 -2

@@ -457,7 +457,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 		task_disable_lazy_fpu_restore(old);
 		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.9.3

Quoted 12 lines · On 02/02, [email protected] wrote:

From: Rik van Riel <[email protected]>

With Oleg's patch "x86, fpu: don't abuse FPU in kernel threads if use_eager_fpu()", kernel threads no longer have an FPU state even on systems with use_eager_fpu()

That in turn means that a task may still have its FPU state loaded in the FPU registers, if the task only got interrupted by kernel threads from when it went to sleep, to when it woke up again.

Quoted 5 lines

In that case, there is no need to restore the FPU state for this task, since it is still in the registers.

The kernel can simply use the same logic to determine this as is used for !use_eager_fpu() systems.

Yes, agreed, I was going to do this too.

And in fact this change make sense even without "don't abuse FPU in kernel threads", I think.

But in theory it depends on another change, "__kernel_fpu_begin() should clear fpu_owner_task even if use_eager_fpu()".

And that series was ignored ;)

I think this patch is fine.

This also fixes the lazy FPU restore disabling in drop_fpu, which only really works when !use_eager_fpu().
...

arch/x86/include/asm/fpu-internal.h +1 -1

 	preempt_enable();

@@ -440,7 +440,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta
 					     new->thread.fpu_counter > 5);
 	if (__thread_has_fpu(old)) {
 		if (!__save_init_fpu(old))
-			old->thread.fpu.last_cpu = ~0;
+			task_disable_lazy_fpu_restore(old);
 		else
 			old->thread.fpu.last_cpu = cpu;
 		old->thread.fpu.has_fpu = 0;	/* But leave fpu_owner_task! */

I'll try to read this patch tomorrow. Too late for me.

I think it is fine, but

Quoted 11 lines · On 02/02, [email protected] wrote:

This also fixes the lazy FPU restore disabling in drop_fpu, which only really works when !use_eager_fpu(). ...

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk) * Forget coprocessor state.. */ preempt_disable(); - tsk->thread.fpu_counter = 0; + task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); clear_used_math();

perhaps this makes sense anyway, but I am not sure if the changelog is right.

Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect after this.

Quoted 10 lines

preempt_enable(); @@ -440,7 +440,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta new->thread.fpu_counter > 5); if (__thread_has_fpu(old)) { if
(!__save_init_fpu(old)) - old->thread.fpu.last_cpu = ~0; +
task_disable_lazy_fpu_restore(old); else old->thread.fpu.last_cpu
= cpu; old->thread.fpu.has_fpu = 0; /* But leave fpu_owner_task!
*/ @@ -454,7 +454,7 @@ static inline fpu_switch_t switch_fpu_prepare(struct task_struct *old, struct ta stts(); }
else { old->thread.fpu_counter = 0; - old->thread.fpu.last_cpu =
~0; + task_disable_lazy_fpu_restore(old);

I am also wondering if we can remove this task_disable_lazy_fpu_restore... I mean, perhaps we should shift this into __thread_fpu_end() path. But this is almost off-topic and in any case this patch should not do this.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 20 lines · Oleg Nesterov

I'll try to read this patch tomorrow. Too late for me.

I think it is fine, but

Quoted 11 lines · On 02/02, [email protected] wrote:

This also fixes the lazy FPU restore disabling in drop_fpu, which only really works when !use_eager_fpu(). ...

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk) * Forget coprocessor state.. */ preempt_disable(); - tsk->thread.fpu_counter = 0; + task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); clear_used_math();

perhaps this makes sense anyway, but I am not sure if the changelog is right.

Quoted 2 lines

Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect after this.

There are several code paths, including signal handler stack setup and teardown, where we first clear PF_USED_MATH, but later on set it again, after setting up a new math state for the task.

We need to ensure we always use that new math state, and never lazily re-use what is still in the FPU registers.

Quoted 27 lines · Rik van Riel

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Quoted 20 lines · Oleg Nesterov

I'll try to read this patch tomorrow. Too late for me.

I think it is fine, but

Quoted 11 lines · On 02/02, [email protected] wrote:

This also fixes the lazy FPU restore disabling in drop_fpu, which only really works when !use_eager_fpu(). ...

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk) * Forget coprocessor state.. */ preempt_disable(); - tsk->thread.fpu_counter = 0; + task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); clear_used_math();

perhaps this makes sense anyway, but I am not sure if the changelog is right.

Quoted 2 lines

Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect after this.

Quoted 6 lines

There are several code paths, including signal handler stack setup and teardown, where we first clear PF_USED_MATH, but later on set it again, after setting up a new math state for the task.

We need to ensure we always use that new math state, and never lazily re-use what is still in the FPU registers.

Still can't understand....

I can only see only on example of re-setting PF_USED_MATH if eager_fpu, __restore_xstate_sig() does drop_fpu() + math_state_restore(). And this case looks fine in this sense...

Although let me repeat that imho math_state_restore() and all current users (including flush_thread() added by "don't abuse FPU in kernel threads") need cleanups in any case. I was going to (try to) do this if/when that series is applied. In particular, in this case math_state_restore() will wrongly return with irqs disabled or I am totally confused.

And set_used_math() if copy_from_user() fails doesn't look right, but this is another story.

If I missed something, perhaps you can update the changelog to explain how exactly it fixes drop_fpu? Otherwise, imo this patch should not change it. As we already discussed, we can probably cleanup this disable_lazy_fpu logic, but this needs another change/discussion.

I'll try to read this patch tomorrow. Too late for me.

I think it is fine, but

Quoted 11 lines · On 02/02, [email protected] wrote:

This also fixes the lazy FPU restore disabling in drop_fpu, which only really works when !use_eager_fpu(). ...

--- a/arch/x86/include/asm/fpu-internal.h +++ b/arch/x86/include/asm/fpu-internal.h @@ -396,7 +396,7 @@ static inline void drop_fpu(struct task_struct *tsk) * Forget coprocessor state.. */ preempt_disable(); - tsk->thread.fpu_counter = 0; + task_disable_lazy_fpu_restore(tsk); __drop_fpu(tsk); clear_used_math();

perhaps this makes sense anyway, but I am not sure if the changelog is right.

Note that we clear PF_USED_MATH, last_fpu/has_fpu have no effect after this.

Quoted 2 lines

So I am not sure about this change. At least the
changelog doesn't look right.

You are right, lets drop this patch 6/6.

Do the other five look right?

Is there consensus on the first 5 patches?

Is there consensus on the first 5 patches?