linux-kernel - Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush

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: <20150315185048.GB29134@pd.tnic>
Date:	Sun, 15 Mar 2015 19:50:48 +0100
From:	Borislav Petkov <bp@...e.de>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Dave Hansen <dave.hansen@...el.com>,
	Ingo Molnar <mingo@...nel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Pekka Riikonen <priikone@....fi>,
	Rik van Riel <riel@...hat.com>,
	Suresh Siddha <sbsiddha@...il.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"Yu, Fenghua" <fenghua.yu@...el.com>,
	Quentin Casasnovas <quentin.casasnovas@...cle.com>
Subject: Re: [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in
 flush_thread()

On Sun, Mar 15, 2015 at 07:16:43PM +0100, Oleg Nesterov wrote:
> Of course, drop_init_fpu() is fine if restore_fpu_checking() fails.
> 
> Did you mean this from the very beginning? In this case I agree of course.
> 
> Because I misinterpreted your initial comment:
> 
> 	One example where drop_init_fpu() seems to make sense is
> 	__kernel_fpu_end(): kernel is done with FPU and current was using the
> 	FPU prior so let's restore it for the eagerfpu case.
> 	
> as if you suggest to use it _instead_ of restore_fpu_checking().

Nah, not "instead" - I didn't express myself precisely enough. I was
trying to think out loud and look for an example where drop_init_fpu()
would make sense.

In most of the places it is used, it is in the error path of restoring
the FPU state, i.e. we were unable to restore for some reason, let's
reinit instead of just drop only, in the eager case.

And your patch correctly removed it from flush_thread() where it didn't
make any sense except to cause CPUs to get needlessly warmer.

Anyway, we're on the same page and that was a good exercise :-)

Thanks Oleg!

Btw, we probably should start documenting stuff like that so that we
don't have to re-fault all that info 6 months/a year from now when we
have to touch that code again. Hmm, how about something like this:

---
diff --git a/arch/x86/include/asm/fpu-internal.h b/arch/x86/include/asm/fpu-internal.h
index 2d4adff428ac..996f20a31f0a 100644
--- a/arch/x86/include/asm/fpu-internal.h
+++ b/arch/x86/include/asm/fpu-internal.h
@@ -406,6 +406,17 @@ static inline void restore_init_xstate(void)
 		fxrstor_checking(&init_xstate_buf->i387);
 }
 
+/*
+ * In addition to "forgetting" FPU state for @tsk, we restore the
+ * default FPU state in the eager case. Note, this is not needed in the
+ * non-eager case because there we will set CR0.TS and fault and setup
+ * an FPU state lazily.
+ *
+ * We restore the default FPU state in the eager case here as a means of
+ * addressing the failure of restoring the FPU state which @tsk points
+ * to and we still need some state to use so we use the default, clean
+ * one.
+ */
 static inline void drop_init_fpu(struct task_struct *tsk)
 {
 	if (!use_eager_fpu())
---

?

-- 
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/