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

[Borislav Petkov]

On Tue, Feb 17, 2015 at 11:47:30AM +0100, Oleg Nesterov wrote:
> On 02/16, Borislav Petkov wrote:
> >
> > On Fri, Feb 06, 2015 at 03:01:59PM -0500, riel@xxxxxxxxxx wrote:
> > > From: Oleg Nesterov <oleg@xxxxxxxxxx>
> > >
> > > 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.

Same here :-P

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

I guess this was the reason for

	WARN_ON_ONCE(!irq_fpu_usable());

in kernel_fpu_begin(). And even that solution is not sufficient. Check
out this thread:

https://lkml.kernel.org/r/tip-baa916f39b50ad91661534652110df40396acda0@xxxxxxxxxxxxxx

for similar issues with the current handling of FPU state.

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

Oh ok, I see what you mean.

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

Right, this should be the reason why fpu_save_init() signals intact FPU
state with its retval.

Although I don't know what that XSTATE_FP bit checking is supposed to
mean as manuals say XCR0[0] is always 1. And nothing says that XSAVE
would ever clear it...

But the FNSAVE case, for example and IMHO, says that after FWAIT which
would raise unmasked FPU exceptions and thus clear FPU control register
bits...

Long story short, yeah, we better prevent concurrent __save_init_fpu()
from happening...

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

Same here. :-\

Our FPU code is nuts and misses a lot of comments.

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

Ah, and I was working on a plain 3.19, that's why I didn't see it. I'll rebase
your patches then.

> And, Borislav, thanks for looking at this!

Sure. Thanks for the patience and explaining stuff to me! :)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/