[PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes

[PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes

[Sean Christopherson]

Three bug fixes related to deferring FPU loading to return to userspace,
or in this case, deferring to entering a KVM guest.  And a cleanup patch I
couldn't resist throwing on top.

The crux of the matter is that 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.  The previous attempt to fix the underlying bug(s)
by handling TIF_FPU_NEED_LOAD during kvm_sched_in() only made the bug(s)
harder to hit, i.e. it resolved the preemption case but only shrunk the
window where a softirq could corrupt state.

Paolo, patch 01 will conflict with commit 95145c25a78c ("KVM: x86: Add a
WARN on TIF_NEED_FPU_LOAD in kvm_load_guest_fpu()") that is sitting in
kvm/queue.  The kvm/queue commit should simply be dropped.

Patch 01 fixes the original underlying bug, which is that KVM doesn't
handle TIF_FPU_NEED_LOAD when swapping between guest and userspace FPU
state.

Patch 02 fixes (unconfirmed) issues similar to the reported bug where KVM
doesn't ensure CPU FPU state is fresh when accessing it during emulation.
This patch is smoke tested only (via kvm-unit-tests' emulator tests).

Patch 03 reverts the preemption bug fix, which simultaneously restores the
handling of TIF_FPU_NEED_LOAD in vcpu_enter_guest() to fix the reported
bugs[1][2] and removes the now-unnecessary preemption workaround.

Alternatively, the handling of TIF_NEED_FPU_LOAD in the kvm_sched_in()
path could be left in place, i.e it doesn't cause immediate damage, but
as stated before, restoring FPU state after KVM is preempted only makes
it harder to find the actual bugs.  Case in point, I originally split
the revert into two separate patches (so that removing the kvm_sched_in()
call wouldn't be tagged for stable), but that only hid the underlying
kvm_put_guest_fpu() bug until I fully reverted the commit.

Patch 04 removes an unused emulator context param from several of the
functions that are fixed in patch 02.  The param was left behind during
a previous KVM FPU state rework.

Tested via Thomas Lambertz's mprime reproducer[3], which detected issues
with buggy KVMs on my system in under a minute.  Ran clean for ~30 minutes
on each of the first two patches and several hours with all three patches
applied.

[1] https://lore.kernel.org/kvm/1e525b08-6204-3238-5d56-513f82f1d7fb@djy.llc
[2] https://lore.kernel.org/kvm/bug-206215-28872@https.bugzilla.kernel.org%2F
[3] https://lore.kernel.org/lkml/217248af-e980-9cb0-ff0d-9773413b9d38@thomaslambertz.de

Sean Christopherson (4):
  KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()
  KVM: x86: Ensure guest's FPU state is loaded when accessing for
    emulation
  KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"
  KVM: x86: Remove unused ctxt param from emulator's FPU accessors

 arch/x86/kvm/emulate.c | 67 ++++++++++++++++++++++++++++++++----------
 arch/x86/kvm/x86.c     | 36 +++++++++++++++++------
 2 files changed, 79 insertions(+), 24 deletions(-)

-- 
2.24.1

[PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()

[Sean Christopherson]

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, &current->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, &current->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

Re: [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()

[Dave Hansen]

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, &current->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, &current->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.

Re: [PATCH 1/4] KVM: x86: Handle TIF_NEED_FPU_LOAD in kvm_{load,put}_guest_fpu()

[Sean Christopherson]

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, &current->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, &current->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, &current->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.

[PATCH 2/4] KVM: x86: Ensure guest's FPU state is loaded when accessing for emulation

[Sean Christopherson]

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

[PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"

[Sean Christopherson]

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

Re: [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"

[Liu, Jing2]

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

Re: [PATCH 3/4] KVM: x86: Revert "KVM: X86: Fix fpu state crash in kvm guest"

[Paolo Bonzini]

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

[PATCH 4/4] KVM: x86: Remove unused ctxt param from emulator's FPU accessors

[Sean Christopherson]

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

Re: [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes

[Liu, Jing2]

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

Re: [PATCH 0/4] KVM: x86: TIF_NEED_FPU_LOAD bug fixes

[Sean Christopherson]

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.