linux-kernel - Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150203190805.GA8455@redhat.com>
Date:	Tue, 3 Feb 2015 20:08:05 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	Rik van Riel <riel@...hat.com>
Cc:	dave.hansen@...ux.intel.com, sbsiddha@...il.com,
	luto@...capital.net, tglx@...utronix.de, mingo@...hat.com,
	hpa@...or.com, fenghua.yu@...el.com, x86@...nel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] x86,fpu: use disable_task_lazy_fpu_restore helper

On 02/02, Rik van Riel wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/02/2015 02:21 PM, Oleg Nesterov wrote:
> > I'll try to read this patch tomorrow. Too late for me.
> >
> > I think it is fine, but
> >
> > On 02/02, riel@...hat.com 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.
>
> 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.

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/