Re: [PATCH 2/3] x86, fpu: unlazy_fpu: don't do __thread_fpu_end() if use_eager_fpu()

[Rik van Riel <riel@redhat.com>]

On 01/29/2015 04:08 PM, Oleg Nesterov wrote:
> 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.

I believe that xstateregs_set and xfpregs_set still
need __thread_fpu_end().

What am I missing?

> 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 <oleg@redhat.com>
> ---
>  arch/x86/kernel/i387.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index c3b92c0..8e070a6 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -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();
>  }
>