linux-kernel - [PATCH] KVM: x86: Temporarily drop kvm->srcu when uninitialized vCPU is blocking

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>] [day] [month] [year] [list]

Message-Id: <20220224212646.3544811-1-seanjc@google.com>
Date:   Thu, 24 Feb 2022 21:26:46 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Like Xu <like.xu.linux@...il.com>
Subject: [PATCH] KVM: x86: Temporarily drop kvm->srcu when uninitialized vCPU
 is blocking

Temporarily drop kvm->srcu before invoking kvm_vcpu_block() on a vCPU
that hasn't yet been initialized.  Best case scenario, blocking while
holding kvm->srcu will degrade guest performance.  Worst case scenario,
the vCPU will never get a wake event and the VM's tasks will hang
indefinitely on synchronize_srcu(), e.g. when trying update memslots.

E.g. running the "apic" KVM unit test often results in the test hanging

  ==> 6409/stack <==
  [<0>] __synchronize_srcu.part.0+0x7a/0xa0
  [<0>] kvm_swap_active_memslots+0x141/0x180
  [<0>] kvm_set_memslot+0x2f9/0x470
  [<0>] kvm_set_memory_region+0x29/0x40
  [<0>] kvm_vm_ioctl+0x2c3/0xd70
  [<0>] __x64_sys_ioctl+0x83/0xb0
  [<0>] do_syscall_64+0x3b/0xc0
  [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xae

  ==> 6410/stack <==
  [<0>] kvm_vcpu_block+0x36/0x80
  [<0>] kvm_arch_vcpu_ioctl_run+0x17b1/0x1f50
  [<0>] kvm_vcpu_ioctl+0x279/0x690
  [<0>] __x64_sys_ioctl+0x83/0xb0
  [<0>] do_syscall_64+0x3b/0xc0
  [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xae

While it's tempting to never acquire kvm->srcu for an uninitialized vCPU,
practically speaking there's no penalty to acquiring kvm->srcu "early"
as the KVM_MP_STATE_UNINITIALIZED path is a one-time thing per vCPU.  On
the other hand, seemingly innocuous helpers like kvm_apic_accept_events()
and sync_regs() can theoretically reach code that might access
SRCU-protected data structures, e.g. sync_regs() can trigger forced
existing of nested mode via kvm_vcpu_ioctl_x86_set_vcpu_events().

Fixes: 5d8d2bfc5e65 ("KVM: x86: pull kvm->srcu read-side to kvm_arch_vcpu_ioctl_run")
Cc: Like Xu <like.xu.linux@...il.com>
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/x86.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e55de9b48d1a..8fd60887f38d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10411,7 +10411,11 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
 		 * use before KVM has ever run the vCPU.
 		 */
 		WARN_ON_ONCE(kvm_lapic_hv_timer_in_use(vcpu));
+
+		srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
 		kvm_vcpu_block(vcpu);
+		vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
+
 		if (kvm_apic_accept_events(vcpu) < 0) {
 			r = 0;
 			goto out;

base-commit: 991f988b43c5ee82ef681907bfe979bee93a55c2
-- 
2.35.1.574.g5d30c73bfb-goog