* [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()
2020-01-17 6:26 [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Sean Christopherson
@ 2020-01-17 6:26 ` Sean Christopherson
2020-01-17 18:31 ` Dave Hansen
2020-01-17 6:26 ` [PATCH 2/4] KVM: x86: Ensure guest's FPU state is loaded when accessing for emulation Sean Christopherson
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2020-01-17 6:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Dave Hansen, Thomas Gleixner
Handle TIF_NEED_FPU_LOAD similar to how fpu__copy() handles the flag
when duplicating FPU state to a new task struct. TIF_NEED_FPU_LOAD can
be set any time control is transferred out of KVM, be it voluntarily,
e.g. if I/O is triggered during a KVM call to get_user_pages, or
involuntarily, e.g. if softirq runs after an IRQ occurs. Therefore,
KVM must account for TIF_NEED_FPU_LOAD whenever it is (potentially)
accessing CPU FPU state.
Fixes: 5f409e20b7945 ("x86/fpu: Defer FPU state load until return to userspace")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/x86.c | 27 ++++++++++++++++++++++++---
1 file changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cf917139de6b..0c7211491f98 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -8476,8 +8476,20 @@ static void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
{
fpregs_lock();
- copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
- /* PKRU is separately restored in kvm_x86_ops->run. */
+ /*
+ * If userspace's FPU state is not resident in the CPU registers, just
+ * memcpy() from current, else save CPU state directly to user_fpu.
+ */
+ if (test_thread_flag(TIF_NEED_FPU_LOAD))
+ memcpy(&vcpu->arch.user_fpu->state, ¤t->thread.fpu.state,
+ fpu_kernel_xstate_size);
+ else
+ copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
+
+ /*
+ * Load guest's FPU state to the CPU registers. PKRU is separately
+ * loaded in kvm_x86_ops->run.
+ */
__copy_kernel_to_fpregs(&vcpu->arch.guest_fpu->state,
~XFEATURE_MASK_PKRU);
@@ -8492,7 +8504,16 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
{
fpregs_lock();
- copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
+ /*
+ * If guest's FPU state is not resident in the CPU registers, just
+ * memcpy() from current, else save CPU state directly to guest_fpu.
+ */
+ if (test_thread_flag(TIF_NEED_FPU_LOAD))
+ memcpy(&vcpu->arch.guest_fpu->state, ¤t->thread.fpu.state,
+ fpu_kernel_xstate_size);
+ else
+ copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
+
copy_kernel_to_fpregs(&vcpu->arch.user_fpu->state);
fpregs_mark_activate();
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()
2020-01-17 6:26 ` [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu() Sean Christopherson
@ 2020-01-17 18:31 ` Dave Hansen
2020-01-17 18:43 ` Sean Christopherson
0 siblings, 1 reply; 11+ messages in thread
From: Dave Hansen @ 2020-01-17 18:31 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, Derek Yerger, kernel, Thomas Lambertz,
Rik van Riel, Sebastian Andrzej Siewior, Borislav Petkov,
Thomas Gleixner
On 1/16/20 10:26 PM, Sean Christopherson wrote:
> Handle TIF_NEED_FPU_LOAD similar to how fpu__copy() handles the flag
> when duplicating FPU state to a new task struct. TIF_NEED_FPU_LOAD can
> be set any time control is transferred out of KVM, be it voluntarily,
> e.g. if I/O is triggered during a KVM call to get_user_pages, or
> involuntarily, e.g. if softirq runs after an IRQ occurs. Therefore,
> KVM must account for TIF_NEED_FPU_LOAD whenever it is (potentially)
> accessing CPU FPU state.
>
> Fixes: 5f409e20b7945 ("x86/fpu: Defer FPU state load until return to userspace")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> arch/x86/kvm/x86.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cf917139de6b..0c7211491f98 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8476,8 +8476,20 @@ static void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
> {
> fpregs_lock();
>
> - copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
> - /* PKRU is separately restored in kvm_x86_ops->run. */
> + /*
> + * If userspace's FPU state is not resident in the CPU registers, just
> + * memcpy() from current, else save CPU state directly to user_fpu.
> + */
> + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> + memcpy(&vcpu->arch.user_fpu->state, ¤t->thread.fpu.state,
> + fpu_kernel_xstate_size);
> + else
> + copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
> +
> + /*
> + * Load guest's FPU state to the CPU registers. PKRU is separately
> + * loaded in kvm_x86_ops->run.
> + */
> __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu->state,
> ~XFEATURE_MASK_PKRU);
Nit: it took me a minute to realize that there is both:
vcpu->arch.user_fpu
and
vcpu->arch.guest_fpu
It might help readability to have local variables for those, or at least
a comment to help differentiate the two.
> @@ -8492,7 +8504,16 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
> {
> fpregs_lock();
>
> - copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
> + /*
> + * If guest's FPU state is not resident in the CPU registers, just
> + * memcpy() from current, else save CPU state directly to guest_fpu.
> + */
> + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> + memcpy(&vcpu->arch.guest_fpu->state, ¤t->thread.fpu.state,
> + fpu_kernel_xstate_size);
> + else
> + copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
> +
> copy_kernel_to_fpregs(&vcpu->arch.user_fpu->state);
>
> fpregs_mark_activate();
This also makes me wonder if we want to have copy_fpregs_to_fpstate()
check for TIF_NEED_FPU_LOAD and complain if it's set.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()
2020-01-17 18:31 ` Dave Hansen
@ 2020-01-17 18:43 ` Sean Christopherson
0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-01-17 18:43 UTC (permalink / raw)
To: Dave Hansen
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Thomas Gleixner
On Fri, Jan 17, 2020 at 10:31:53AM -0800, Dave Hansen wrote:
> On 1/16/20 10:26 PM, Sean Christopherson wrote:
> > Handle TIF_NEED_FPU_LOAD similar to how fpu__copy() handles the flag
> > when duplicating FPU state to a new task struct. TIF_NEED_FPU_LOAD can
> > be set any time control is transferred out of KVM, be it voluntarily,
> > e.g. if I/O is triggered during a KVM call to get_user_pages, or
> > involuntarily, e.g. if softirq runs after an IRQ occurs. Therefore,
> > KVM must account for TIF_NEED_FPU_LOAD whenever it is (potentially)
> > accessing CPU FPU state.
> >
> > Fixes: 5f409e20b7945 ("x86/fpu: Defer FPU state load until return to userspace")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> > arch/x86/kvm/x86.c | 27 ++++++++++++++++++++++++---
> > 1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index cf917139de6b..0c7211491f98 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -8476,8 +8476,20 @@ static void kvm_load_guest_fpu(struct kvm_vcpu *vcpu)
> > {
> > fpregs_lock();
> >
> > - copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
> > - /* PKRU is separately restored in kvm_x86_ops->run. */
> > + /*
> > + * If userspace's FPU state is not resident in the CPU registers, just
> > + * memcpy() from current, else save CPU state directly to user_fpu.
> > + */
> > + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> > + memcpy(&vcpu->arch.user_fpu->state, ¤t->thread.fpu.state,
> > + fpu_kernel_xstate_size);
> > + else
> > + copy_fpregs_to_fpstate(vcpu->arch.user_fpu);
> > +
> > + /*
> > + * Load guest's FPU state to the CPU registers. PKRU is separately
> > + * loaded in kvm_x86_ops->run.
> > + */
> > __copy_kernel_to_fpregs(&vcpu->arch.guest_fpu->state,
> > ~XFEATURE_MASK_PKRU);
>
> Nit: it took me a minute to realize that there is both:
>
> vcpu->arch.user_fpu
> and
> vcpu->arch.guest_fpu
>
> It might help readability to have local variables for those, or at least
> a comment to help differentiate the two.
Or even better, add a helper to wrap the logic instead of copy+paste, e.g.:
static void kvm_save_current_fpu(struct fpu *fpu)
{
if (test_thread_flag(TIF_NEED_FPU_LOAD))
memcpy(&fpu->state, ¤t->thread.fpu.state,
fpu_kernel_xstate_size);
else
copy_fpregs_to_fpstate(fpu);
}
>
>
> > @@ -8492,7 +8504,16 @@ static void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
> > {
> > fpregs_lock();
> >
> > - copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
> > + /*
> > + * If guest's FPU state is not resident in the CPU registers, just
> > + * memcpy() from current, else save CPU state directly to guest_fpu.
> > + */
> > + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> > + memcpy(&vcpu->arch.guest_fpu->state, ¤t->thread.fpu.state,
> > + fpu_kernel_xstate_size);
> > + else
> > + copy_fpregs_to_fpstate(vcpu->arch.guest_fpu);
> > +
> > copy_kernel_to_fpregs(&vcpu->arch.user_fpu->state);
> >
> > fpregs_mark_activate();
>
> This also makes me wonder if we want to have copy_fpregs_to_fpstate()
> check for TIF_NEED_FPU_LOAD and complain if it's set.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] KVM: x86: Ensure guest's FPU state is loaded when accessing for emulation
2020-01-17 6:26 [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Sean Christopherson
2020-01-17 6:26 ` [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu() Sean Christopherson
@ 2020-01-17 6:26 ` Sean Christopherson
2020-01-17 6:26 ` [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest" Sean Christopherson
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-01-17 6:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Dave Hansen, Thomas Gleixner
Lock the FPU regs and reload the current thread's FPU state, which holds
the guest's FPU state, to the CPU registers if necessary prior to
accessing guest FPU state as part of emulation. kernel_fpu_begin() can
be called from softirq context, therefore KVM must ensure softirqs are
disabled (locking the FPU regs disables softirqs) when touching CPU FPU
state.
Note, for all intents and purposes this reverts commit 6ab0b9feb82a7
("x86,kvm: remove KVM emulator get_fpu / put_fpu"), but at the time it
was applied, removing get/put_fpu() was correct. The re-introduction
of {get,put}_fpu() is necessitated by the deferring of FPU state load.
Fixes: 5f409e20b7945 ("x86/fpu: Defer FPU state load until return to userspace")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/emulate.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 952d1a4f4d7e..2a5bed60ce50 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -22,6 +22,7 @@
#include "kvm_cache_regs.h"
#include <asm/kvm_emulate.h>
#include <linux/stringify.h>
+#include <asm/fpu/api.h>
#include <asm/debugreg.h>
#include <asm/nospec-branch.h>
@@ -1075,8 +1076,23 @@ static void fetch_register_operand(struct operand *op)
}
}
+static void emulator_get_fpu(void)
+{
+ fpregs_lock();
+
+ fpregs_assert_state_consistent();
+ if (test_thread_flag(TIF_NEED_FPU_LOAD))
+ switch_fpu_return();
+}
+
+static void emulator_put_fpu(void)
+{
+ fpregs_unlock();
+}
+
static void read_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data, int reg)
{
+ emulator_get_fpu();
switch (reg) {
case 0: asm("movdqa %%xmm0, %0" : "=m"(*data)); break;
case 1: asm("movdqa %%xmm1, %0" : "=m"(*data)); break;
@@ -1098,11 +1114,13 @@ static void read_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data, int reg)
#endif
default: BUG();
}
+ emulator_put_fpu();
}
static void write_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data,
int reg)
{
+ emulator_get_fpu();
switch (reg) {
case 0: asm("movdqa %0, %%xmm0" : : "m"(*data)); break;
case 1: asm("movdqa %0, %%xmm1" : : "m"(*data)); break;
@@ -1124,10 +1142,12 @@ static void write_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data,
#endif
default: BUG();
}
+ emulator_put_fpu();
}
static void read_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
{
+ emulator_get_fpu();
switch (reg) {
case 0: asm("movq %%mm0, %0" : "=m"(*data)); break;
case 1: asm("movq %%mm1, %0" : "=m"(*data)); break;
@@ -1139,10 +1159,12 @@ static void read_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
case 7: asm("movq %%mm7, %0" : "=m"(*data)); break;
default: BUG();
}
+ emulator_put_fpu();
}
static void write_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
{
+ emulator_get_fpu();
switch (reg) {
case 0: asm("movq %0, %%mm0" : : "m"(*data)); break;
case 1: asm("movq %0, %%mm1" : : "m"(*data)); break;
@@ -1154,6 +1176,7 @@ static void write_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
case 7: asm("movq %0, %%mm7" : : "m"(*data)); break;
default: BUG();
}
+ emulator_put_fpu();
}
static int em_fninit(struct x86_emulate_ctxt *ctxt)
@@ -1161,7 +1184,9 @@ static int em_fninit(struct x86_emulate_ctxt *ctxt)
if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
return emulate_nm(ctxt);
+ emulator_get_fpu();
asm volatile("fninit");
+ emulator_put_fpu();
return X86EMUL_CONTINUE;
}
@@ -1172,7 +1197,9 @@ static int em_fnstcw(struct x86_emulate_ctxt *ctxt)
if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
return emulate_nm(ctxt);
+ emulator_get_fpu();
asm volatile("fnstcw %0": "+m"(fcw));
+ emulator_put_fpu();
ctxt->dst.val = fcw;
@@ -1186,7 +1213,9 @@ static int em_fnstsw(struct x86_emulate_ctxt *ctxt)
if (ctxt->ops->get_cr(ctxt, 0) & (X86_CR0_TS | X86_CR0_EM))
return emulate_nm(ctxt);
+ emulator_get_fpu();
asm volatile("fnstsw %0": "+m"(fsw));
+ emulator_put_fpu();
ctxt->dst.val = fsw;
@@ -4092,8 +4121,12 @@ static int em_fxsave(struct x86_emulate_ctxt *ctxt)
if (rc != X86EMUL_CONTINUE)
return rc;
+ emulator_get_fpu();
+
rc = asm_safe("fxsave %[fx]", , [fx] "+m"(fx_state));
+ emulator_put_fpu();
+
if (rc != X86EMUL_CONTINUE)
return rc;
@@ -4136,6 +4169,8 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
if (rc != X86EMUL_CONTINUE)
return rc;
+ emulator_get_fpu();
+
if (size < __fxstate_size(16)) {
rc = fxregs_fixup(&fx_state, size);
if (rc != X86EMUL_CONTINUE)
@@ -4151,6 +4186,8 @@ static int em_fxrstor(struct x86_emulate_ctxt *ctxt)
rc = asm_safe("fxrstor %[fx]", : [fx] "m"(fx_state));
out:
+ emulator_put_fpu();
+
return rc;
}
@@ -5448,7 +5485,9 @@ static int flush_pending_x87_faults(struct x86_emulate_ctxt *ctxt)
{
int rc;
+ emulator_get_fpu();
rc = asm_safe("fwait");
+ emulator_put_fpu();
if (unlikely(rc != X86EMUL_CONTINUE))
return emulate_exception(ctxt, MF_VECTOR, 0, false);
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"
2020-01-17 6:26 [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Sean Christopherson
2020-01-17 6:26 ` [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu() Sean Christopherson
2020-01-17 6:26 ` [PATCH 2/4] KVM: x86: Ensure guest's FPU state is loaded when accessing for emulation Sean Christopherson
@ 2020-01-17 6:26 ` Sean Christopherson
2020-03-04 7:41 ` Liu, Jing2
2020-01-17 6:26 ` [PATCH 4/4] KVM: x86: Remove unused ctxt param from emulator's FPU accessors Sean Christopherson
2020-03-04 7:38 ` [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Liu, Jing2
4 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2020-01-17 6:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Dave Hansen, Thomas Gleixner
Reload the current thread's FPU state, which contains the guest's FPU
state, to the CPU registers if necessary during vcpu_enter_guest().
TIF_NEED_FPU_LOAD can be set any time control is transferred out of KVM,
e.g. if I/O is triggered during a KVM call to get_user_pages() or if a
softirq occurs while KVM is scheduled in.
Moving the handling of TIF_NEED_FPU_LOAD from vcpu_enter_guest() to
kvm_arch_vcpu_load(), effectively kvm_sched_in(), papered over a bug
where kvm_put_guest_fpu() failed to account for TIF_NEED_FPU_LOAD. The
easiest way to the kvm_put_guest_fpu() bug was to run with involuntary
preemption enable, thus handling TIF_NEED_FPU_LOAD during kvm_sched_in()
made the bug go away. But, removing the handling in vcpu_enter_guest()
exposed KVM to the rare case of a softirq triggering kernel_fpu_begin()
between vcpu_load() and vcpu_enter_guest().
Now that kvm_{load,put}_guest_fpu() correctly handle TIF_NEED_FPU_LOAD,
revert the commit to both restore the vcpu_enter_guest() behavior and
eliminate the superfluous switch_fpu_return() in kvm_arch_vcpu_load().
Note, leaving the handling in kvm_arch_vcpu_load() isn't wrong per se,
but it is unnecessary, and most critically, makes it extremely difficult
to find bugs such as the kvm_put_guest_fpu() issue due to shrinking the
window where a softirq can corrupt state.
A sample trace triggered by warning if TIF_NEED_FPU_LOAD is set while
vcpu state is loaded:
<IRQ>
gcmaes_crypt_by_sg.constprop.12+0x26e/0x660
? 0xffffffffc024547d
? __qdisc_run+0x83/0x510
? __dev_queue_xmit+0x45e/0x990
? ip_finish_output2+0x1a8/0x570
? fib4_rule_action+0x61/0x70
? fib4_rule_action+0x70/0x70
? fib_rules_lookup+0x13f/0x1c0
? helper_rfc4106_decrypt+0x82/0xa0
? crypto_aead_decrypt+0x40/0x70
? crypto_aead_decrypt+0x40/0x70
? crypto_aead_decrypt+0x40/0x70
? esp_output_tail+0x8f4/0xa5a [esp4]
? skb_ext_add+0xd3/0x170
? xfrm_input+0x7a6/0x12c0
? xfrm4_rcv_encap+0xae/0xd0
? xfrm4_transport_finish+0x200/0x200
? udp_queue_rcv_one_skb+0x1ba/0x460
? udp_unicast_rcv_skb.isra.63+0x72/0x90
? __udp4_lib_rcv+0x51b/0xb00
? ip_protocol_deliver_rcu+0xd2/0x1c0
? ip_local_deliver_finish+0x44/0x50
? ip_local_deliver+0xe0/0xf0
? ip_protocol_deliver_rcu+0x1c0/0x1c0
? ip_rcv+0xbc/0xd0
? ip_rcv_finish_core.isra.19+0x380/0x380
? __netif_receive_skb_one_core+0x7e/0x90
? netif_receive_skb_internal+0x3d/0xb0
? napi_gro_receive+0xed/0x150
? 0xffffffffc0243c77
? net_rx_action+0x149/0x3b0
? __do_softirq+0xe4/0x2f8
? handle_irq_event_percpu+0x6a/0x80
? irq_exit+0xe6/0xf0
? do_IRQ+0x7f/0xd0
? common_interrupt+0xf/0xf
</IRQ>
? irq_entries_start+0x20/0x660
? vmx_get_interrupt_shadow+0x2f0/0x710 [kvm_intel]
? kvm_set_msr_common+0xfc7/0x2380 [kvm]
? recalibrate_cpu_khz+0x10/0x10
? ktime_get+0x3a/0xa0
? kvm_arch_vcpu_ioctl_run+0x107/0x560 [kvm]
? kvm_init+0x6bf/0xd00 [kvm]
? __seccomp_filter+0x7a/0x680
? do_vfs_ioctl+0xa4/0x630
? security_file_ioctl+0x32/0x50
? ksys_ioctl+0x60/0x90
? __x64_sys_ioctl+0x16/0x20
? do_syscall_64+0x5f/0x1a0
? entry_SYSCALL_64_after_hwframe+0x44/0xa9
---[ end trace 9564a1ccad733a90 ]---
This reverts commit e751732486eb3f159089a64d1901992b1357e7cc.
Fixes: e751732486eb3 ("KVM: X86: Fix fpu state crash in kvm guest")
Reported-by: Derek Yerger <derek@djy.llc>
Reported-by: kernel@najdan.com
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Thomas Lambertz <mail@thomaslambertz.de>
Cc: Rik van Riel <riel@surriel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/x86.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0c7211491f98..ac290b7cc4d7 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3458,10 +3458,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
kvm_x86_ops->vcpu_load(vcpu, cpu);
- fpregs_assert_state_consistent();
- if (test_thread_flag(TIF_NEED_FPU_LOAD))
- switch_fpu_return();
-
/* Apply any externally detected TSC adjustments (due to suspend) */
if (unlikely(vcpu->arch.tsc_offset_adjustment)) {
adjust_tsc_offset_host(vcpu, vcpu->arch.tsc_offset_adjustment);
@@ -8198,8 +8194,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
trace_kvm_entry(vcpu->vcpu_id);
guest_enter_irqoff();
- /* The preempt notifier should have taken care of the FPU already. */
- WARN_ON_ONCE(test_thread_flag(TIF_NEED_FPU_LOAD));
+ fpregs_assert_state_consistent();
+ if (test_thread_flag(TIF_NEED_FPU_LOAD))
+ switch_fpu_return();
if (unlikely(vcpu->arch.switch_db_regs)) {
set_debugreg(0, 7);
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"
2020-01-17 6:26 ` [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest" Sean Christopherson
@ 2020-03-04 7:41 ` Liu, Jing2
2020-03-04 7:58 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: Liu, Jing2 @ 2020-03-04 7:41 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, Derek Yerger, kernel, Thomas Lambertz,
Rik van Riel, Sebastian Andrzej Siewior, Borislav Petkov,
Dave Hansen, Thomas Gleixner
On 1/17/2020 2:26 PM, Sean Christopherson wrote:
> @@ -8198,8 +8194,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> trace_kvm_entry(vcpu->vcpu_id);
> guest_enter_irqoff();
>
> - /* The preempt notifier should have taken care of the FPU already. */
> - WARN_ON_ONCE(test_thread_flag(TIF_NEED_FPU_LOAD));
> + fpregs_assert_state_consistent();
> + if (test_thread_flag(TIF_NEED_FPU_LOAD))
> + switch_fpu_return();
>
> if (unlikely(vcpu->arch.switch_db_regs)) {
> set_debugreg(0, 7);
Can kvm be preempt out again after this (before really enter to guest)?
Thanks,
Jing
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"
2020-03-04 7:41 ` Liu, Jing2
@ 2020-03-04 7:58 ` Paolo Bonzini
0 siblings, 0 replies; 11+ messages in thread
From: Paolo Bonzini @ 2020-03-04 7:58 UTC (permalink / raw)
To: Liu, Jing2, Sean Christopherson
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, Derek Yerger, kernel, Thomas Lambertz,
Rik van Riel, Sebastian Andrzej Siewior, Borislav Petkov,
Dave Hansen, Thomas Gleixner
On 04/03/20 08:41, Liu, Jing2 wrote:
>> trace_kvm_entry(vcpu->vcpu_id);
>> guest_enter_irqoff();
>> - /* The preempt notifier should have taken care of the FPU
>> already. */
>> - WARN_ON_ONCE(test_thread_flag(TIF_NEED_FPU_LOAD));
>> + fpregs_assert_state_consistent();
>> + if (test_thread_flag(TIF_NEED_FPU_LOAD))
>> + switch_fpu_return();
>> if (unlikely(vcpu->arch.switch_db_regs)) {
>> set_debugreg(0, 7);
>
> Can kvm be preempt out again after this (before really enter to guest)?
No, irqs are disabled here.
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] KVM: x86: Remove unused ctxt param from emulator's FPU accessors
2020-01-17 6:26 [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Sean Christopherson
` (2 preceding siblings ...)
2020-01-17 6:26 ` [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest" Sean Christopherson
@ 2020-01-17 6:26 ` Sean Christopherson
2020-03-04 7:38 ` [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Liu, Jing2
4 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-01-17 6:26 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Dave Hansen, Thomas Gleixner
Remove an unused struct x86_emulate_ctxt * param from low level helpers
used to access guest FPU state. The unused param was left behind by
commit 6ab0b9feb82a ("x86,kvm: remove KVM emulator get_fpu / put_fpu").
No functional change intended.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/emulate.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 2a5bed60ce50..3e3b3cd60cce 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1090,7 +1090,7 @@ static void emulator_put_fpu(void)
fpregs_unlock();
}
-static void read_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data, int reg)
+static void read_sse_reg(sse128_t *data, int reg)
{
emulator_get_fpu();
switch (reg) {
@@ -1117,8 +1117,7 @@ static void read_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data, int reg)
emulator_put_fpu();
}
-static void write_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data,
- int reg)
+static void write_sse_reg(sse128_t *data, int reg)
{
emulator_get_fpu();
switch (reg) {
@@ -1145,7 +1144,7 @@ static void write_sse_reg(struct x86_emulate_ctxt *ctxt, sse128_t *data,
emulator_put_fpu();
}
-static void read_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
+static void read_mmx_reg(u64 *data, int reg)
{
emulator_get_fpu();
switch (reg) {
@@ -1162,7 +1161,7 @@ static void read_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
emulator_put_fpu();
}
-static void write_mmx_reg(struct x86_emulate_ctxt *ctxt, u64 *data, int reg)
+static void write_mmx_reg(u64 *data, int reg)
{
emulator_get_fpu();
switch (reg) {
@@ -1234,7 +1233,7 @@ static void decode_register_operand(struct x86_emulate_ctxt *ctxt,
op->type = OP_XMM;
op->bytes = 16;
op->addr.xmm = reg;
- read_sse_reg(ctxt, &op->vec_val, reg);
+ read_sse_reg(&op->vec_val, reg);
return;
}
if (ctxt->d & Mmx) {
@@ -1285,7 +1284,7 @@ static int decode_modrm(struct x86_emulate_ctxt *ctxt,
op->type = OP_XMM;
op->bytes = 16;
op->addr.xmm = ctxt->modrm_rm;
- read_sse_reg(ctxt, &op->vec_val, ctxt->modrm_rm);
+ read_sse_reg(&op->vec_val, ctxt->modrm_rm);
return rc;
}
if (ctxt->d & Mmx) {
@@ -1862,10 +1861,10 @@ static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
op->bytes * op->count);
break;
case OP_XMM:
- write_sse_reg(ctxt, &op->vec_val, op->addr.xmm);
+ write_sse_reg(&op->vec_val, op->addr.xmm);
break;
case OP_MM:
- write_mmx_reg(ctxt, &op->mm_val, op->addr.mm);
+ write_mmx_reg(&op->mm_val, op->addr.mm);
break;
case OP_NONE:
/* no writeback */
@@ -5495,11 +5494,10 @@ static int flush_pending_x87_faults(struct x86_emulate_ctxt *ctxt)
return X86EMUL_CONTINUE;
}
-static void fetch_possible_mmx_operand(struct x86_emulate_ctxt *ctxt,
- struct operand *op)
+static void fetch_possible_mmx_operand(struct operand *op)
{
if (op->type == OP_MM)
- read_mmx_reg(ctxt, &op->mm_val, op->addr.mm);
+ read_mmx_reg(&op->mm_val, op->addr.mm);
}
static int fastop(struct x86_emulate_ctxt *ctxt, void (*fop)(struct fastop *))
@@ -5578,10 +5576,10 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
* Now that we know the fpu is exception safe, we can fetch
* operands from it.
*/
- fetch_possible_mmx_operand(ctxt, &ctxt->src);
- fetch_possible_mmx_operand(ctxt, &ctxt->src2);
+ fetch_possible_mmx_operand(&ctxt->src);
+ fetch_possible_mmx_operand(&ctxt->src2);
if (!(ctxt->d & Mov))
- fetch_possible_mmx_operand(ctxt, &ctxt->dst);
+ fetch_possible_mmx_operand(&ctxt->dst);
}
if (unlikely(emul_flags & X86EMUL_GUEST_MASK) && ctxt->intercept) {
--
2.24.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes
2020-01-17 6:26 [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Sean Christopherson
` (3 preceding siblings ...)
2020-01-17 6:26 ` [PATCH 4/4] KVM: x86: Remove unused ctxt param from emulator's FPU accessors Sean Christopherson
@ 2020-03-04 7:38 ` Liu, Jing2
2020-03-04 15:24 ` Sean Christopherson
4 siblings, 1 reply; 11+ messages in thread
From: Liu, Jing2 @ 2020-03-04 7:38 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, Derek Yerger, kernel, Thomas Lambertz,
Rik van Riel, Sebastian Andrzej Siewior, Borislav Petkov,
Dave Hansen, Thomas Gleixner
On 1/17/2020 2:26 PM, Sean Christopherson wrote:
> TIF_FPU_NEED_LOAD can be set any time
> control is transferred out of KVM, e.g. via IRQ->softirq, not just when
> KVM is preempted.
Hi Sean,
Is this just because kernel_fpu_begin() is called during softirq? I saw
the dump trace in 3/4 message, but didn't find out clue.
Could I ask where kernel_fpu_begin() is called? Or is this just a
"possible" thing?
Because I just want to make sure that, kvm can use this flag to cover
all preempt/softirq/(other?) cases?
Thanks,
Jing
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes
2020-03-04 7:38 ` [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes Liu, Jing2
@ 2020-03-04 15:24 ` Sean Christopherson
0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2020-03-04 15:24 UTC (permalink / raw)
To: Liu, Jing2
Cc: Paolo Bonzini, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Derek Yerger, kernel,
Thomas Lambertz, Rik van Riel, Sebastian Andrzej Siewior,
Borislav Petkov, Dave Hansen, Thomas Gleixner
On Wed, Mar 04, 2020 at 03:38:44PM +0800, Liu, Jing2 wrote:
>
> On 1/17/2020 2:26 PM, Sean Christopherson wrote:
> >TIF_FPU_NEED_LOAD can be set any time
> >control is transferred out of KVM, e.g. via IRQ->softirq, not just when
> >KVM is preempted.
>
> Hi Sean,
>
> Is this just because kernel_fpu_begin() is called during softirq? I saw the
> dump trace in 3/4 message, but didn't find out clue.
Yes, but "just" doing kernel_fpu_begin() swaps the task's (e.g. guest's in
this case) XSAVE/FPU state out of the CPU's registers.
> Could I ask where kernel_fpu_begin() is called? Or is this just a "possible"
> thing?
In the trace from patch 3, it's called by gcmaes_crypt_by_sg() to decrypt a
packet[*] during a receive action after the kernel was interruped by the
network device.
[*] I assume it's decrypting a packet, I'm not at all familiar with the
networking stack so it could be decrypting something else entirely.
> Because I just want to make sure that, kvm can use this flag to cover all
> preempt/softirq/(other?) cases?
Yes, TIF_FPU_NEED_LOAD is set any time its associated tasks's FPU state is
swapped out and needs to be reloaded before returning to userspace. For
KVM, "returning to userspace" also means entering the guest or accessing
guest state.
^ permalink raw reply [flat|nested] 11+ messages in thread