Skip to content

Commit 1e5ae93

Browse files
sean-jckaihuang
authored andcommitted
KVM: Use dedicated mutex to protect kvm_usage_count to avoid deadlock
Use a dedicated mutex to guard kvm_usage_count to fix a potential deadlock on x86 due to a chain of locks and SRCU synchronizations. Translating the below lockdep splat, CPU1 #6 will wait on CPU0 #1, CPU0 #8 will wait on CPU2 #3, and CPU2 #7 will wait on CPU1 #4 (if there's a writer, due to the fairness of r/w semaphores). CPU0 CPU1 CPU2 1 lock(&kvm->slots_lock); 2 lock(&vcpu->mutex); 3 lock(&kvm->srcu); 4 lock(cpu_hotplug_lock); 5 lock(kvm_lock); 6 lock(&kvm->slots_lock); 7 lock(cpu_hotplug_lock); 8 sync(&kvm->srcu); Note, there are likely more potential deadlocks in KVM x86, e.g. the same pattern of taking cpu_hotplug_lock outside of kvm_lock likely exists with __kvmclock_cpufreq_notifier(), but actually triggering such deadlocks is beyond rare due to the combination of dependencies and timings involved. E.g. the cpufreq notifier is only used on older CPUs without a constant TSC, mucking with the NX hugepage mitigation while VMs are running is very uncommon, and doing so while also onlining/offlining a CPU (necessary to generate contention on cpu_hotplug_lock) would be even more unusual. ====================================================== WARNING: possible circular locking dependency detected 6.10.0-smp--c257535a0c9d-pip #330 Tainted: G S O ------------------------------------------------------ tee/35048 is trying to acquire lock: ff6a80eced71e0a8 (&kvm->slots_lock){+.+.}-{3:3}, at: set_nx_huge_pages+0x179/0x1e0 [kvm] but task is already holding lock: ffffffffc07abb08 (kvm_lock){+.+.}-{3:3}, at: set_nx_huge_pages+0x14a/0x1e0 [kvm] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (kvm_lock){+.+.}-{3:3}: __mutex_lock+0x6a/0xb40 mutex_lock_nested+0x1f/0x30 kvm_dev_ioctl+0x4fb/0xe50 [kvm] __se_sys_ioctl+0x7b/0xd0 __x64_sys_ioctl+0x21/0x30 x64_sys_call+0x15d0/0x2e60 do_syscall_64+0x83/0x160 entry_SYSCALL_64_after_hwframe+0x76/0x7e -> #2 (cpu_hotplug_lock){++++}-{0:0}: cpus_read_lock+0x2e/0xb0 static_key_slow_inc+0x16/0x30 kvm_lapic_set_base+0x6a/0x1c0 [kvm] kvm_set_apic_base+0x8f/0xe0 [kvm] kvm_set_msr_common+0x9ae/0xf80 [kvm] vmx_set_msr+0xa54/0xbe0 [kvm_intel] __kvm_set_msr+0xb6/0x1a0 [kvm] kvm_arch_vcpu_ioctl+0xeca/0x10c0 [kvm] kvm_vcpu_ioctl+0x485/0x5b0 [kvm] __se_sys_ioctl+0x7b/0xd0 __x64_sys_ioctl+0x21/0x30 x64_sys_call+0x15d0/0x2e60 do_syscall_64+0x83/0x160 entry_SYSCALL_64_after_hwframe+0x76/0x7e -> #1 (&kvm->srcu){.+.+}-{0:0}: __synchronize_srcu+0x44/0x1a0 synchronize_srcu_expedited+0x21/0x30 kvm_swap_active_memslots+0x110/0x1c0 [kvm] kvm_set_memslot+0x360/0x620 [kvm] __kvm_set_memory_region+0x27b/0x300 [kvm] kvm_vm_ioctl_set_memory_region+0x43/0x60 [kvm] kvm_vm_ioctl+0x295/0x650 [kvm] __se_sys_ioctl+0x7b/0xd0 __x64_sys_ioctl+0x21/0x30 x64_sys_call+0x15d0/0x2e60 do_syscall_64+0x83/0x160 entry_SYSCALL_64_after_hwframe+0x76/0x7e -> #0 (&kvm->slots_lock){+.+.}-{3:3}: __lock_acquire+0x15ef/0x2e30 lock_acquire+0xe0/0x260 __mutex_lock+0x6a/0xb40 mutex_lock_nested+0x1f/0x30 set_nx_huge_pages+0x179/0x1e0 [kvm] param_attr_store+0x93/0x100 module_attr_store+0x22/0x40 sysfs_kf_write+0x81/0xb0 kernfs_fop_write_iter+0x133/0x1d0 vfs_write+0x28d/0x380 ksys_write+0x70/0xe0 __x64_sys_write+0x1f/0x30 x64_sys_call+0x281b/0x2e60 do_syscall_64+0x83/0x160 entry_SYSCALL_64_after_hwframe+0x76/0x7e Cc: Chao Gao <chao.gao@intel.com> Fixes: 0bf5049 ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock") Cc: stable@vger.kernel.org Signed-off-by: Sean Christopherson <seanjc@google.com>
1 parent a694b94 commit 1e5ae93

File tree

2 files changed

+29
-21
lines changed

2 files changed

+29
-21
lines changed

Documentation/virt/kvm/locking.rst

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,13 @@ time it will be set using the Dirty tracking mechanism described above.
227227
:Type: mutex
228228
:Arch: any
229229
:Protects: - vm_list
230-
- kvm_usage_count
230+
231+
``kvm_usage_count``
232+
^^^^^^^^^^^^^^^^^^^
233+
234+
:Type: mutex
235+
:Arch: any
236+
:Protects: - kvm_usage_count
231237
- hardware virtualization enable/disable
232238
:Comment: KVM also disables CPU hotplug via cpus_read_lock() during
233239
enable/disable.
@@ -290,11 +296,12 @@ time it will be set using the Dirty tracking mechanism described above.
290296
wakeup.
291297

292298
``vendor_module_lock``
293-
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
299+
^^^^^^^^^^^^^^^^^^^^^^
294300
:Type: mutex
295301
:Arch: x86
296302
:Protects: loading a vendor module (kvm_amd or kvm_intel)
297-
:Comment: Exists because using kvm_lock leads to deadlock. cpu_hotplug_lock is
298-
taken outside of kvm_lock, e.g. in KVM's CPU online/offline callbacks, and
299-
many operations need to take cpu_hotplug_lock when loading a vendor module,
300-
e.g. updating static calls.
303+
:Comment: Exists because using kvm_lock leads to deadlock. kvm_lock is taken
304+
in notifiers, e.g. __kvmclock_cpufreq_notifier(), that may be invoked while
305+
cpu_hotplug_lock is held, e.g. from cpufreq_boost_trigger_state(), and many
306+
operations need to take cpu_hotplug_lock when loading a vendor module, e.g.
307+
updating static calls.

virt/kvm/kvm_main.c

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5573,6 +5573,7 @@ __visible bool kvm_rebooting;
55735573
EXPORT_SYMBOL_GPL(kvm_rebooting);
55745574

55755575
static DEFINE_PER_CPU(bool, hardware_enabled);
5576+
static DEFINE_MUTEX(kvm_usage_lock);
55765577
static int kvm_usage_count;
55775578

55785579
static int __hardware_enable_nolock(void)
@@ -5605,10 +5606,10 @@ static int kvm_online_cpu(unsigned int cpu)
56055606
* be enabled. Otherwise running VMs would encounter unrecoverable
56065607
* errors when scheduled to this CPU.
56075608
*/
5608-
mutex_lock(&kvm_lock);
5609+
mutex_lock(&kvm_usage_lock);
56095610
if (kvm_usage_count)
56105611
ret = __hardware_enable_nolock();
5611-
mutex_unlock(&kvm_lock);
5612+
mutex_unlock(&kvm_usage_lock);
56125613
return ret;
56135614
}
56145615

@@ -5628,10 +5629,10 @@ static void hardware_disable_nolock(void *junk)
56285629

56295630
static int kvm_offline_cpu(unsigned int cpu)
56305631
{
5631-
mutex_lock(&kvm_lock);
5632+
mutex_lock(&kvm_usage_lock);
56325633
if (kvm_usage_count)
56335634
hardware_disable_nolock(NULL);
5634-
mutex_unlock(&kvm_lock);
5635+
mutex_unlock(&kvm_usage_lock);
56355636
return 0;
56365637
}
56375638

@@ -5647,9 +5648,9 @@ static void hardware_disable_all_nolock(void)
56475648
static void hardware_disable_all(void)
56485649
{
56495650
cpus_read_lock();
5650-
mutex_lock(&kvm_lock);
5651+
mutex_lock(&kvm_usage_lock);
56515652
hardware_disable_all_nolock();
5652-
mutex_unlock(&kvm_lock);
5653+
mutex_unlock(&kvm_usage_lock);
56535654
cpus_read_unlock();
56545655
}
56555656

@@ -5680,7 +5681,7 @@ static int hardware_enable_all(void)
56805681
* enable hardware multiple times.
56815682
*/
56825683
cpus_read_lock();
5683-
mutex_lock(&kvm_lock);
5684+
mutex_lock(&kvm_usage_lock);
56845685

56855686
r = 0;
56865687

@@ -5694,7 +5695,7 @@ static int hardware_enable_all(void)
56945695
}
56955696
}
56965697

5697-
mutex_unlock(&kvm_lock);
5698+
mutex_unlock(&kvm_usage_lock);
56985699
cpus_read_unlock();
56995700

57005701
return r;
@@ -5722,13 +5723,13 @@ static int kvm_suspend(void)
57225723
{
57235724
/*
57245725
* Secondary CPUs and CPU hotplug are disabled across the suspend/resume
5725-
* callbacks, i.e. no need to acquire kvm_lock to ensure the usage count
5726-
* is stable. Assert that kvm_lock is not held to ensure the system
5727-
* isn't suspended while KVM is enabling hardware. Hardware enabling
5728-
* can be preempted, but the task cannot be frozen until it has dropped
5729-
* all locks (userspace tasks are frozen via a fake signal).
5726+
* callbacks, i.e. no need to acquire kvm_usage_lock to ensure the usage
5727+
* count is stable. Assert that kvm_usage_lock is not held to ensure
5728+
* the system isn't suspended while KVM is enabling hardware. Hardware
5729+
* enabling can be preempted, but the task cannot be frozen until it has
5730+
* dropped all locks (userspace tasks are frozen via a fake signal).
57305731
*/
5731-
lockdep_assert_not_held(&kvm_lock);
5732+
lockdep_assert_not_held(&kvm_usage_lock);
57325733
lockdep_assert_irqs_disabled();
57335734

57345735
if (kvm_usage_count)
@@ -5738,7 +5739,7 @@ static int kvm_suspend(void)
57385739

57395740
static void kvm_resume(void)
57405741
{
5741-
lockdep_assert_not_held(&kvm_lock);
5742+
lockdep_assert_not_held(&kvm_usage_lock);
57425743
lockdep_assert_irqs_disabled();
57435744

57445745
if (kvm_usage_count)

0 commit comments

Comments
 (0)