Re: [PATCH 01/17] x86, fpu: wrap get_xsave_addr() to make it safer

[Oleg Nesterov <oleg@redhat.com>]

So far I do not understand this discussion ;) I didn't see the patches
and other emails...

On 03/24, Andy Lutomirski wrote:
>
> On Tue, Mar 24, 2015 at 5:12 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> >>
> >> So why are you unlazying it?
> >
> > Oleg actually suggested it.

perhaps you are talking about math_error() ? It uses save_init_fpu()
in Linus's tree, but this is wrong. It was changed in tip/x86/fpu
to use unlazy_fpu() and save_init_fpu() was killed. Plus, just in
case, unlazy_fpu() was changed too and now it is

	void unlazy_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();
	}

and in fact (I think) it needs another change later, __thread_fpu_end()
should depend on __save_init_fpu().

> >> IIUC, the xstae for current can be in one of three logical states:
> >>
> >> 1. Live in CPU regs.  The in-memory copy is garbage and the state is
> >> in CPU regs.
> >> 2. Lazy.  The in-memory copy and the CPU regs match.  Writing to
> >> either copy is illegal.
> >> 3. In memory only.  Writing to the in-memory copy is safe.
> >>
> >> IIUC, you want to read the xstate, do you're okay with #2 or #3.  This
> >> would be tsk_get_xsave_field_for_read in my terminology.
> >>
> >> If you want to write the xstate, you'd need to be in state #3, which
> >> would be tsk_get_xsave_field_for_write.
> >>
> >> IIUC, unlazy_fpu just moves from from state 2 to 3.
> >
> > I won't completely claim to understand what's going on with the FPU
> > code, but I think your analysis is a bit off.
> >
> > unlazy_fpu() does __save_init_fpu() which (among other things) calls
> > xsave to dump the CPU registers to memory.  That doesn't make any sense
> > to do if "The in-memory copy and the CPU regs match."
> >
> > IOW, unlazy_fpu() is called when the in-memory copy is garbage and takes
> > us to a state where we can look at the in-memory copy.

Not necessarily, but most probably I misunderstood... If __thread_has_fpu()
we don't and can't know whether in-memory copy and the CPU regs match, so
we simply write FPU state to memory.

> I think that __save_init_fpu (called by unlazy_fpu) does that, but
> __thread_fpu_end calls __thread_clear_has_fpu, which, in turn, zaps
> fpu_owner_task, which will force an unnecessary xrstor.  Or maybe not
> if we have further bugs.

Yes, see above. We do not really need unconditional __thread_fpu_end().

> Holy crap these functions are poorly named.

Yes... let me quote another email from me:

	Perhaps you can also find a beter name for __save_init_fpu/etc ;) The
	name clearly suggests that it does "save + init" while in fact it does
	"save and may be destroy FPU state". At least for the callers, the fact
	that "destroy" is actually "init" doesn't really matter.

	But lets not rename it right now. This can conflict with the fixes we
	need to do first.

So please do not rename it now ;) I had to switch to other tasks, but I hope
I will continue the FPU cleanups "soon".

> Also, what, if anything,
> guarantees that fpu_owner_task is set on entry to userspace?

Well, this probaly needs other cleanups. I am not sure __thread_set_has_fpu()
actually should set fpu_owner_task = current. But this is offtopic.

Again, I am not sure I understand your concern... but fpu_owner_task should
be set if __thread_has_fpu(). Otherwise we do not care.

But __thread_fpu_end() must _clear_ fpu_owner_task, this is what we do care.

Oleg.