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

[Oleg Nesterov <oleg@redhat.com>]

On 02/16, Borislav Petkov wrote:
>
> On Fri, Feb 06, 2015 at 03:01:59PM -0500, riel@redhat.com wrote:
> > From: Oleg Nesterov <oleg@redhat.com>
> >
> > 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.
>
> I can follow so far.
>
> > - 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.
>
> Well, my primitive understanding would say no, not safely, for the
> simple reason that we have only one XSAVE state area per thread.
> However, __kernel_fpu_begin() is called with preemption disabled so ...
> I guess I'm still not seeing the race.

This is not about preemption. But let me first say that I do not know
how the FPU hardware actually works, and I do not understand the FPU
asm code at all.

Let's look at this code

	if (__thread_has_fpu(tsk)) {
		__save_init_fpu(tsk);	// interrupt -> kernel_fpu_begin()
		__thread_fpu_end(tsk);
	}

Suppose that kernel_fpu_begin() from interrupt races with __save_init_fpu()
in progress. Is this safe? I do not know.

My concern is that (I think) __save_init_fpu() can save the FPU state _and_
modify it (say, it can reset some register to default value). This means that
the nested __save_init_fpu() from __kernel_fpu_begin() can save the modified
register again to current->thread.fpu.

If my understanding is wrong, then why switch_fpu_prepare() clears .last_cpu
if __save_init_fpu() returns 0 (which iiuc means that CPU's state does not
match the saved state) ?

Plus I have other (more vague) concerns...

> Btw, what is kernel_fpu_disable()? Can't find it here.

It's already in Linus's tree, see

14e153ef75eecae8fd0738ffb42120f4962a00cd x86, fpu: Introduce per-cpu in_kernel_fpu state
33a3ebdc077fd85f1bf4d4586eea579b297461ae x86, fpu: Don't abuse has_fpu in __kernel_fpu_begin/end()
7575637ab293861a799f3bbafe0d8c597389f4e9 x86, fpu: Fix math_state_restore() race with kernel_fpu_begin()

And, Borislav, thanks for looking at this!

Oleg.