Skip to content

Commit 3498b0f

Browse files
sean-jckaihuang
authored andcommittedJun 30, 2024
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 be294be commit 3498b0f

File tree

2 files changed

+29
-21
lines changed

2 files changed

+29
-21
lines changed
 

‎Documentation/virt/kvm/locking.rst

+13-6
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

+16-15
Original file line numberDiff line numberDiff line change
@@ -5572,6 +5572,7 @@ __visible bool kvm_rebooting;
55725572
EXPORT_SYMBOL_GPL(kvm_rebooting);
55735573

55745574
static DEFINE_PER_CPU(bool, hardware_enabled);
5575+
static DEFINE_MUTEX(kvm_usage_lock);
55755576
static int kvm_usage_count;
55765577

55775578
static int __hardware_enable_nolock(void)
@@ -5604,10 +5605,10 @@ static int kvm_online_cpu(unsigned int cpu)
56045605
* be enabled. Otherwise running VMs would encounter unrecoverable
56055606
* errors when scheduled to this CPU.
56065607
*/
5607-
mutex_lock(&kvm_lock);
5608+
mutex_lock(&kvm_usage_lock);
56085609
if (kvm_usage_count)
56095610
ret = __hardware_enable_nolock();
5610-
mutex_unlock(&kvm_lock);
5611+
mutex_unlock(&kvm_usage_lock);
56115612
return ret;
56125613
}
56135614

@@ -5627,10 +5628,10 @@ static void hardware_disable_nolock(void *junk)
56275628

56285629
static int kvm_offline_cpu(unsigned int cpu)
56295630
{
5630-
mutex_lock(&kvm_lock);
5631+
mutex_lock(&kvm_usage_lock);
56315632
if (kvm_usage_count)
56325633
hardware_disable_nolock(NULL);
5633-
mutex_unlock(&kvm_lock);
5634+
mutex_unlock(&kvm_usage_lock);
56345635
return 0;
56355636
}
56365637

@@ -5646,9 +5647,9 @@ static void hardware_disable_all_nolock(void)
56465647
static void hardware_disable_all(void)
56475648
{
56485649
cpus_read_lock();
5649-
mutex_lock(&kvm_lock);
5650+
mutex_lock(&kvm_usage_lock);
56505651
hardware_disable_all_nolock();
5651-
mutex_unlock(&kvm_lock);
5652+
mutex_unlock(&kvm_usage_lock);
56525653
cpus_read_unlock();
56535654
}
56545655

@@ -5679,7 +5680,7 @@ static int hardware_enable_all(void)
56795680
* enable hardware multiple times.
56805681
*/
56815682
cpus_read_lock();
5682-
mutex_lock(&kvm_lock);
5683+
mutex_lock(&kvm_usage_lock);
56835684

56845685
r = 0;
56855686

@@ -5693,7 +5694,7 @@ static int hardware_enable_all(void)
56935694
}
56945695
}
56955696

5696-
mutex_unlock(&kvm_lock);
5697+
mutex_unlock(&kvm_usage_lock);
56975698
cpus_read_unlock();
56985699

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

57335734
if (kvm_usage_count)
@@ -5737,7 +5738,7 @@ static int kvm_suspend(void)
57375738

57385739
static void kvm_resume(void)
57395740
{
5740-
lockdep_assert_not_held(&kvm_lock);
5741+
lockdep_assert_not_held(&kvm_usage_lock);
57415742
lockdep_assert_irqs_disabled();
57425743

57435744
if (kvm_usage_count)

0 commit comments

Comments
 (0)