KVM: Use dedicated mutex to protect kvm_usage_count to avoid deadlock
commit 44d17459626052a2390457e550a12cb973506b2f upstream.
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():
cpuhp_cpufreq_online()
|
-> cpufreq_online()
|
-> cpufreq_gov_performance_limits()
|
-> __cpufreq_driver_target()
|
-> __target_index()
|
-> cpufreq_freq_transition_begin()
|
-> cpufreq_notify_transition()
|
-> ... __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.
The most robust solution to the general cpu_hotplug_lock issue is likely
to switch vm_list to be an RCU-protected list, e.g. so that x86's cpufreq
notifier doesn't to take kvm_lock. For now, settle for fixing the most
blatant deadlock, as switching to an RCU-protected list is a much more
involved change, but add a comment in locking.rst to call out that care
needs to be taken when walking holding kvm_lock and walking vm_list.
======================================================
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: 0bf50497f0 ("KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock")
Cc: stable@vger.kernel.org
Reviewed-by: Kai Huang <kai.huang@intel.com>
Acked-by: Kai Huang <kai.huang@intel.com>
Tested-by: Farrah Chen <farrah.chen@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
Message-ID: <20240830043600.127750-2-seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
This commit is contained in:
Sean Christopherson
Greg Kroah-Hartman
parent
beef3353c6
commit
4777225ec8
|
|
@ -9,7 +9,7 @@ KVM Lock Overview
|
|||
|
||||
The acquisition orders for mutexes are as follows:
|
||||
|
||||
- cpus_read_lock() is taken outside kvm_lock
|
||||
- cpus_read_lock() is taken outside kvm_lock and kvm_usage_lock
|
||||
|
||||
- kvm->lock is taken outside vcpu->mutex
|
||||
|
||||
|
|
@ -24,6 +24,12 @@ The acquisition orders for mutexes are as follows:
|
|||
are taken on the waiting side when modifying memslots, so MMU notifiers
|
||||
must not take either kvm->slots_lock or kvm->slots_arch_lock.
|
||||
|
||||
cpus_read_lock() vs kvm_lock:
|
||||
- Taking cpus_read_lock() outside of kvm_lock is problematic, despite that
|
||||
being the official ordering, as it is quite easy to unknowingly trigger
|
||||
cpus_read_lock() while holding kvm_lock. Use caution when walking vm_list,
|
||||
e.g. avoid complex operations when possible.
|
||||
|
||||
For SRCU:
|
||||
|
||||
- ``synchronize_srcu(&kvm->srcu)`` is called inside critical sections
|
||||
|
|
@ -228,10 +234,17 @@ time it will be set using the Dirty tracking mechanism described above.
|
|||
:Type: mutex
|
||||
:Arch: any
|
||||
:Protects: - vm_list
|
||||
- kvm_usage_count
|
||||
|
||||
``kvm_usage_lock``
|
||||
^^^^^^^^^^^^^^^^^^
|
||||
|
||||
:Type: mutex
|
||||
:Arch: any
|
||||
:Protects: - kvm_usage_count
|
||||
- hardware virtualization enable/disable
|
||||
:Comment: KVM also disables CPU hotplug via cpus_read_lock() during
|
||||
enable/disable.
|
||||
:Comment: Exists because using kvm_lock leads to deadlock (see earlier comment
|
||||
on cpus_read_lock() vs kvm_lock). Note, KVM also disables CPU hotplug via
|
||||
cpus_read_lock() when enabling/disabling virtualization.
|
||||
|
||||
``kvm->mn_invalidate_lock``
|
||||
^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
|
|
@ -291,11 +304,12 @@ time it will be set using the Dirty tracking mechanism described above.
|
|||
wakeup.
|
||||
|
||||
``vendor_module_lock``
|
||||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
||||
^^^^^^^^^^^^^^^^^^^^^^
|
||||
:Type: mutex
|
||||
:Arch: x86
|
||||
:Protects: loading a vendor module (kvm_amd or kvm_intel)
|
||||
:Comment: Exists because using kvm_lock leads to deadlock. cpu_hotplug_lock is
|
||||
taken outside of kvm_lock, e.g. in KVM's CPU online/offline callbacks, and
|
||||
many operations need to take cpu_hotplug_lock when loading a vendor module,
|
||||
e.g. updating static calls.
|
||||
:Comment: Exists because using kvm_lock leads to deadlock. kvm_lock is taken
|
||||
in notifiers, e.g. __kvmclock_cpufreq_notifier(), that may be invoked while
|
||||
cpu_hotplug_lock is held, e.g. from cpufreq_boost_trigger_state(), and many
|
||||
operations need to take cpu_hotplug_lock when loading a vendor module, e.g.
|
||||
updating static calls.
|
||||
|
|
|
|||
|
|
@ -5174,6 +5174,7 @@ __visible bool kvm_rebooting;
|
|||
EXPORT_SYMBOL_GPL(kvm_rebooting);
|
||||
|
||||
static DEFINE_PER_CPU(bool, hardware_enabled);
|
||||
static DEFINE_MUTEX(kvm_usage_lock);
|
||||
static int kvm_usage_count;
|
||||
|
||||
static int __hardware_enable_nolock(void)
|
||||
|
|
@ -5206,10 +5207,10 @@ static int kvm_online_cpu(unsigned int cpu)
|
|||
* be enabled. Otherwise running VMs would encounter unrecoverable
|
||||
* errors when scheduled to this CPU.
|
||||
*/
|
||||
mutex_lock(&kvm_lock);
|
||||
mutex_lock(&kvm_usage_lock);
|
||||
if (kvm_usage_count)
|
||||
ret = __hardware_enable_nolock();
|
||||
mutex_unlock(&kvm_lock);
|
||||
mutex_unlock(&kvm_usage_lock);
|
||||
return ret;
|
||||
}
|
||||
|
||||
|
|
@ -5229,10 +5230,10 @@ static void hardware_disable_nolock(void *junk)
|
|||
|
||||
static int kvm_offline_cpu(unsigned int cpu)
|
||||
{
|
||||
mutex_lock(&kvm_lock);
|
||||
mutex_lock(&kvm_usage_lock);
|
||||
if (kvm_usage_count)
|
||||
hardware_disable_nolock(NULL);
|
||||
mutex_unlock(&kvm_lock);
|
||||
mutex_unlock(&kvm_usage_lock);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
@ -5248,9 +5249,9 @@ static void hardware_disable_all_nolock(void)
|
|||
static void hardware_disable_all(void)
|
||||
{
|
||||
cpus_read_lock();
|
||||
mutex_lock(&kvm_lock);
|
||||
mutex_lock(&kvm_usage_lock);
|
||||
hardware_disable_all_nolock();
|
||||
mutex_unlock(&kvm_lock);
|
||||
mutex_unlock(&kvm_usage_lock);
|
||||
cpus_read_unlock();
|
||||
}
|
||||
|
||||
|
|
@ -5281,7 +5282,7 @@ static int hardware_enable_all(void)
|
|||
* enable hardware multiple times.
|
||||
*/
|
||||
cpus_read_lock();
|
||||
mutex_lock(&kvm_lock);
|
||||
mutex_lock(&kvm_usage_lock);
|
||||
|
||||
r = 0;
|
||||
|
||||
|
|
@ -5295,7 +5296,7 @@ static int hardware_enable_all(void)
|
|||
}
|
||||
}
|
||||
|
||||
mutex_unlock(&kvm_lock);
|
||||
mutex_unlock(&kvm_usage_lock);
|
||||
cpus_read_unlock();
|
||||
|
||||
return r;
|
||||
|
|
@ -5323,13 +5324,13 @@ static int kvm_suspend(void)
|
|||
{
|
||||
/*
|
||||
* Secondary CPUs and CPU hotplug are disabled across the suspend/resume
|
||||
* callbacks, i.e. no need to acquire kvm_lock to ensure the usage count
|
||||
* is stable. Assert that kvm_lock is not held to ensure the system
|
||||
* isn't suspended while KVM is enabling hardware. Hardware enabling
|
||||
* can be preempted, but the task cannot be frozen until it has dropped
|
||||
* all locks (userspace tasks are frozen via a fake signal).
|
||||
* callbacks, i.e. no need to acquire kvm_usage_lock to ensure the usage
|
||||
* count is stable. Assert that kvm_usage_lock is not held to ensure
|
||||
* the system isn't suspended while KVM is enabling hardware. Hardware
|
||||
* enabling can be preempted, but the task cannot be frozen until it has
|
||||
* dropped all locks (userspace tasks are frozen via a fake signal).
|
||||
*/
|
||||
lockdep_assert_not_held(&kvm_lock);
|
||||
lockdep_assert_not_held(&kvm_usage_lock);
|
||||
lockdep_assert_irqs_disabled();
|
||||
|
||||
if (kvm_usage_count)
|
||||
|
|
@ -5339,7 +5340,7 @@ static int kvm_suspend(void)
|
|||
|
||||
static void kvm_resume(void)
|
||||
{
|
||||
lockdep_assert_not_held(&kvm_lock);
|
||||
lockdep_assert_not_held(&kvm_usage_lock);
|
||||
lockdep_assert_irqs_disabled();
|
||||
|
||||
if (kvm_usage_count)
|
||||
|
|
|
|||
Loading…
Reference in New Issue