linux-kernel - Re: [RFC PATCH 10/10] x86/fpu: defer FPU state load until return to userspace

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: <D5D1FB08-9CE8-498B-9636-C0390153814A@amacapital.net>
Date:   Thu, 20 Sep 2018 20:45:56 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>
Subject: Re: [RFC PATCH 10/10] x86/fpu: defer FPU state load until return to userspace

> On Sep 19, 2018, at 10:05 AM, Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> 
> On 2018-09-12 08:47:19 [-0700], Andy Lutomirski wrote:
>>> --- a/arch/x86/kernel/fpu/core.c
>>> +++ b/arch/x86/kernel/fpu/core.c
>>> @@ -101,14 +101,14 @@ void __kernel_fpu_begin(void)
>>> 
>>>   kernel_fpu_disable();
>>> 
>>> -    if (fpu->initialized) {
>>> +    __cpu_invalidate_fpregs_state();
>>> +
>>> +    if (!test_and_set_thread_flag(TIF_LOAD_FPU)) {
>> 
>> Since the already-TIF_LOAD_FPU path is supposed to be fast here, use test_thread_flag() instead. test_and_set operations do unconditional RMW operations and are always full barriers, so they’re slow.
> okay.
> 
>> Also, on top of this patch, there should be lots of cleanups available. In particular, all the fpu state accessors could probably be reworked to take TIF_LOAD_FPU into account, which would simplify the callers and maybe even the mess of variables tracking whether the state is in regs.
> 
> Do you refer to the fpu.initilized check or something else?
> 

I mean the fpu.initialized variable entirely. AFAIK, its only use is for kernel threads — setting it to false lets us switch to a kernel thread and back without saving and restoring. But TIF_LOAD_FPU should be able to replace it: when we have FPU regs loaded and we switch to *any* thread, kernel or otherwise, we can set TIF_LOAD_FPU and leave the old regs loaded.  So we don’t need the special case for kernel threads.

Which reminds me: if you haven’t already done so, can you add a helper to sanity check the current context?  It should check that the combination of owner_ctx, last_cpu, and TIF_LOAD_FPU is sane. For example, if owner_ctx or last_cpu is says the cpu regs are invalid for current but TIF_LOAD_FPU is clear, it should warn.  I think that at least switch_fpu_finish should call it.  Arguably switch_fpu_prepare should too, at the beginning.