diff mbox series

KVM: x86: SVM: Call x86_spec_ctrl_set_guest/host() with interrupts disabled

Message ID alpine.DEB.2.21.1807202307400.1694@nanos.tec.linutronix.de (mailing list archive)
State New, archived
Headers show
Series KVM: x86: SVM: Call x86_spec_ctrl_set_guest/host() with interrupts disabled | expand

Commit Message

Thomas Gleixner Aug. 12, 2018, 6:59 p.m. UTC 
Subject: KVM: x86: SVM: Call x86_spec_ctrl_set_guest/host() with interrupts disabled
From: Thomas Gleixner <tglx@linutronix.de>
Date: Sun, 12 Aug 2018 20:41:45 +0200

Mikhail reported the following lockdep splat:

WARNING: possible irq lock inversion dependency detected
CPU 0/KVM/10284 just changed the state of lock:
  000000000d538a88 (&st->lock){+...}, at:
  speculative_store_bypass_update+0x10b/0x170

but this lock was taken by another, HARDIRQ-safe lock
in the past:

(&(&sighand->siglock)->rlock){-.-.}

   and interrupts could create inverse lock ordering between them.
 
Possible interrupt unsafe locking scenario:

    CPU0                    CPU1
    ----                    ----
   lock(&st->lock);
                           local_irq_disable();
                           lock(&(&sighand->siglock)->rlock);
                           lock(&st->lock);
    <Interrupt>
     lock(&(&sighand->siglock)->rlock);
     *** DEADLOCK ***

The code path which connects those locks is:

   speculative_store_bypass_update()
   ssb_prctl_set()
   do_seccomp()
   do_syscall_64()

In svm_vcpu_run() speculative_store_bypass_update() is called with
interupts enabled via x86_virt_spec_ctrl_set_guest/host().

Move the invocations of x86_virt_spec_ctrl_set_guest/host() into the
interrupt disabled region to cure it.

Fixes: 1f50ddb4f418 ("x86/speculation: Handle HT correctly on AMD")
Reported-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Mikhail Gavrilov <mikhail.v.gavrilov@gmail.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: kvm@vger.kernel.org
Cc: x86@kernel.org
Cc: stable@vger.kernel.org
---
 arch/x86/kvm/svm.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Paolo Bonzini Aug. 13, 2018, 7:52 a.m. UTC | #1 
On 12/08/2018 20:59, Thomas Gleixner wrote:
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5580,8 +5580,6 @@ static void svm_vcpu_run(struct kvm_vcpu
>  
>  	clgi();
>  
> -	local_irq_enable();
> -
>  	/*
>  	 * If this vCPU has touched SPEC_CTRL, restore the guest's value if
>  	 * it's non-zero. Since vmentry is serialising on affected CPUs, there
> @@ -5590,6 +5588,8 @@ static void svm_vcpu_run(struct kvm_vcpu
>  	 */
>  	x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
>  
> +	local_irq_enable();
> +

It is actually a false positive because the clgi() keeps interrupts
disabled even if IF=1.  (This complication in the AMD virtualization
extensions is there because IF=1 tells VMRUN that you should exit in
case an interrupt arrives, but they it won't be serviced until the
corresponding STGI).  Likewise for the call to x86_spec_ctrl_restore_host.

Still, the patch is correct and it's a good idea to keep the GIF=0/IF=1
area as small and self-contained as possible.  So:

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo
diff mbox series

Patch

--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5580,8 +5580,6 @@  static void svm_vcpu_run(struct kvm_vcpu
 
 	clgi();
 
-	local_irq_enable();
-
 	/*
 	 * If this vCPU has touched SPEC_CTRL, restore the guest's value if
 	 * it's non-zero. Since vmentry is serialising on affected CPUs, there
@@ -5590,6 +5588,8 @@  static void svm_vcpu_run(struct kvm_vcpu
 	 */
 	x86_spec_ctrl_set_guest(svm->spec_ctrl, svm->virt_spec_ctrl);
 
+	local_irq_enable();
+
 	asm volatile (
 		"push %%" _ASM_BP "; \n\t"
 		"mov %c[rbx](%[svm]), %%" _ASM_BX " \n\t"
@@ -5712,12 +5712,12 @@  static void svm_vcpu_run(struct kvm_vcpu
 	if (unlikely(!msr_write_intercepted(vcpu, MSR_IA32_SPEC_CTRL)))
 		svm->spec_ctrl = native_read_msr(MSR_IA32_SPEC_CTRL);
 
-	x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
-
 	reload_tss(vcpu);
 
 	local_irq_disable();
 
+	x86_spec_ctrl_restore_host(svm->spec_ctrl, svm->virt_spec_ctrl);
+
 	vcpu->arch.cr2 = svm->vmcb->save.cr2;
 	vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
 	vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;