linux-kernel - Re: [PATCH 1/3] x86, fpu: __kernel_fpu_begin() should clear fpu_owner_task even if use_eager

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: <20150220181343.GG19378@pd.tnic>
Date:	Fri, 20 Feb 2015 19:13:43 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Rik van Riel <riel@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Suresh Siddha <sbsiddha@...il.com>,
	linux-kernel@...r.kernel.org, mingo@...hat.com, hpa@...or.com,
	matt.fleming@...el.com, bp@...e.de, pbonzini@...hat.com,
	tglx@...utronix.de, luto@...capital.net
Subject: Re: [PATCH 1/3] x86, fpu: __kernel_fpu_begin() should clear
 fpu_owner_task even if use_eager_fpu()

On Mon, Jan 19, 2015 at 07:51:32PM +0100, Oleg Nesterov wrote:
> __kernel_fpu_begin() does nothing if !__thread_has_fpu() && use_eager_fpu(),
> perhaps it assumes that this case is simply impossible. This is certainly
> not possible if in_interrupt() == T; interrupted_user_mode() should have
> FPU, and interrupted_kernel_fpu_idle() should fail if !__thread_has_fpu().
>
> However, even if use_eager_fpu() == T a task can do drop_fpu(), then switch
> to another thread which becomes fpu_owner_task, then resume and call some
> function which does kernel_fpu_begin(). Say, an exiting task does a lot of
> things after exit_thread(), it is not safe to assume that it can't use FPU
> in these paths.

Yap, that makes sense. Applied.

> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  arch/x86/kernel/i387.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
> index 81049ff..26f0e80 100644
> --- a/arch/x86/kernel/i387.c
> +++ b/arch/x86/kernel/i387.c
> @@ -93,9 +93,10 @@ void __kernel_fpu_begin(void)
>  
>  	if (__thread_has_fpu(me)) {
>  		__save_init_fpu(me);
> -	} else if (!use_eager_fpu()) {
> +	} else {
>  		this_cpu_write(fpu_owner_task, NULL);
> -		clts();
> +		if (!use_eager_fpu())
> +			clts();
> 	}

Some git archeology:

304bceda6a18 ("x86, fpu: use non-lazy fpu restore for processors supporting xsave")

added that different handling on use_eager_fpu() boxes.
interrupted_kernel_fpu_idle() failed then on those machines though and when it
was switched to

	if (use_eager_fpu())
		return __thread_has_fpu(current);

in

5187b28ff082 ("x86: Allow FPU to be used at interrupt time even with eagerfpu")

it forgot to correct the "else if" in __kernel_fpu_begin().

Here's the relevant hunk from 304bceda6a18:

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index ab6a2e8028ae..528557470ddb 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -22,7 +22,15 @@
 /*
  * Were we in an interrupt that interrupted kernel mode?
  *
- * We can do a kernel_fpu_begin/end() pair *ONLY* if that
+ * For now, on xsave platforms we will return interrupted
+ * kernel FPU as not-idle. TBD: As we use non-lazy FPU restore
+ * for xsave platforms, ideally we can change the return value
+ * to something like __thread_has_fpu(current). But we need to
+ * be careful of doing __thread_clear_has_fpu() before saving
+ * the FPU etc for supporting nested uses etc. For now, take
+ * the simple route!
+ *
+ * On others, we can do a kernel_fpu_begin/end() pair *ONLY* if that
  * pair does nothing at all: the thread must not have fpu (so
  * that we don't try to save the FPU state), and TS must
  * be set (so that the clts/stts pair does nothing that is
@@ -30,6 +38,9 @@
  */
 static inline bool interrupted_kernel_fpu_idle(void)
 {
+       if (use_xsave())
+               return 0;
+
        return !__thread_has_fpu(current) &&
                (read_cr0() & X86_CR0_TS);
 }
@@ -73,7 +84,7 @@ void kernel_fpu_begin(void)
                __save_init_fpu(me);
                __thread_clear_has_fpu(me);
                /* We do 'stts()' in kernel_fpu_end() */
-       } else {
+       } else if (!use_xsave()) {
                this_cpu_write(fpu_owner_task, NULL);
                clts();
        }

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