| Message ID | 20221102231911.3107438-14-seanjc@google.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | KVM: Rework kvm_init() and hardware enabling | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/apply | fail | Patch does not apply to for-next |
| conchuod/tree_selection | success | Guessing tree name failed |
On Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > doing hardware setup to ensure that concurrent calls are fully serialized. > KVM rejects attempts to load vendor modules if a different module has > already been loaded, but doesn't handle the case where multiple vendor > modules are loaded at the same time, and module_init() doesn't run under > the global module_mutex. > > Note, in practice, this is likely a benign bug as no platform exists that > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > vendor modules is guaranteed to fail a support check before modifying > common KVM state. > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > but that comes with its own ugliness as it would require setting > .hardware_enable before success is guaranteed, e.g. attempting to load > the "wrong" could result in spurious failure to load the "right" module. > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > to kvm_lock being taken under cpus_write_lock(), and in the future, under > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > cpus_read_lock() to register a callback. In theory, KVM could avoid > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > updating static calls also acquires cpus_read_lock(). > > Inverting the lock ordering, i.e. always taking kvm_lock outside > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > kvm_lock and is called under cpus_write_lock(). "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't happened yet. > > The lockdep splat below is dependent on future patches to take > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > already is already possible. IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it doesn't fit to protect multiple vendor module loading. Looks above argument is good enough. I am not sure whether we need additional justification which comes from future patches. :) Also, do you also want to update Documentation/virt/kvm/locking.rst" in this patch? > > > ====================================================== > WARNING: possible circular locking dependency detected > 6.0.0-smp--7ec93244f194-init2 #27 Tainted: G O > ------------------------------------------------------ > stable/251833 is trying to acquire lock: > ffffffffc097ea28 (kvm_lock){+.+.}-{3:3}, at: hardware_enable_all+0x1f/0xc0 [kvm] > > but task is already holding lock: > ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm] > > which lock already depends on the new lock. > > the existing dependency chain (in reverse order) is: > > -> #1 (cpu_hotplug_lock){++++}-{0:0}: > cpus_read_lock+0x2a/0xa0 > __cpuhp_setup_state+0x2b/0x60 > __kvm_x86_vendor_init+0x16a/0x1870 [kvm] > kvm_x86_vendor_init+0x23/0x40 [kvm] > 0xffffffffc0a4d02b > do_one_initcall+0x110/0x200 > do_init_module+0x4f/0x250 > load_module+0x1730/0x18f0 > __se_sys_finit_module+0xca/0x100 > __x64_sys_finit_module+0x1d/0x20 > do_syscall_64+0x3d/0x80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > -> #0 (kvm_lock){+.+.}-{3:3}: > __lock_acquire+0x16f4/0x30d0 > lock_acquire+0xb2/0x190 > __mutex_lock+0x98/0x6f0 > mutex_lock_nested+0x1b/0x20 > hardware_enable_all+0x1f/0xc0 [kvm] > kvm_dev_ioctl+0x45e/0x930 [kvm] > __se_sys_ioctl+0x77/0xc0 > __x64_sys_ioctl+0x1d/0x20 > do_syscall_64+0x3d/0x80 > entry_SYSCALL_64_after_hwframe+0x63/0xcd > > other info that might help us debug this: > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(cpu_hotplug_lock); > lock(kvm_lock); > lock(cpu_hotplug_lock); > lock(kvm_lock); > > *** DEADLOCK *** > > 1 lock held by stable/251833: > #0: ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm] > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/x86.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index a0ca401d3cdf..218707597bea 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -128,6 +128,7 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu); > static int __set_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2); > static void __get_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2); > > +static DEFINE_MUTEX(vendor_module_lock); > struct kvm_x86_ops kvm_x86_ops __read_mostly; > > #define KVM_X86_OP(func) \ > @@ -9280,7 +9281,7 @@ void kvm_arch_exit(void) > > } > > -int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > +static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > { > u64 host_pat; > int r; > @@ -9413,6 +9414,17 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > kmem_cache_destroy(x86_emulator_cache); > return r; > } > + > +int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) > +{ > + int r; > + > + mutex_lock(&vendor_module_lock); > + r = __kvm_x86_vendor_init(ops); > + mutex_unlock(&vendor_module_lock); > + > + return r; > +} > EXPORT_SYMBOL_GPL(kvm_x86_vendor_init); > > void kvm_x86_vendor_exit(void) > @@ -9435,7 +9447,6 @@ void kvm_x86_vendor_exit(void) > cancel_work_sync(&pvclock_gtod_work); > #endif > static_call(kvm_x86_hardware_unsetup)(); > - kvm_x86_ops.hardware_enable = NULL; > kvm_mmu_vendor_module_exit(); > free_percpu(user_return_msrs); > kmem_cache_destroy(x86_emulator_cache); > @@ -9443,6 +9454,9 @@ void kvm_x86_vendor_exit(void) > static_key_deferred_flush(&kvm_xen_enabled); > WARN_ON(static_branch_unlikely(&kvm_xen_enabled.key)); > #endif > + mutex_lock(&vendor_module_lock); > + kvm_x86_ops.hardware_enable = NULL; > + mutex_unlock(&vendor_module_lock); > } > EXPORT_SYMBOL_GPL(kvm_x86_vendor_exit); >
On Wed, Nov 16, 2022, Huang, Kai wrote: > On Wed, 2022-11-02 at 23:18 +0000, Sean Christopherson wrote: > > Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while > > doing hardware setup to ensure that concurrent calls are fully serialized. > > KVM rejects attempts to load vendor modules if a different module has > > already been loaded, but doesn't handle the case where multiple vendor > > modules are loaded at the same time, and module_init() doesn't run under > > the global module_mutex. > > > > Note, in practice, this is likely a benign bug as no platform exists that > > supports both SVM and VMX, i.e. barring a weird VM setup, one of the > > vendor modules is guaranteed to fail a support check before modifying > > common KVM state. > > > > Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, > > but that comes with its own ugliness as it would require setting > > .hardware_enable before success is guaranteed, e.g. attempting to load > > the "wrong" could result in spurious failure to load the "right" module. > > > > Introduce a new mutex as using kvm_lock is extremely deadlock prone due > > to kvm_lock being taken under cpus_write_lock(), and in the future, under > > under cpus_read_lock(). Any operation that takes cpus_read_lock() while > > holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes > > cpus_read_lock() to register a callback. In theory, KVM could avoid > > such problematic paths, i.e. do less setup under kvm_lock, but avoiding > > all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. > > updating static calls also acquires cpus_read_lock(). > > > > Inverting the lock ordering, i.e. always taking kvm_lock outside > > cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes > > kvm_lock and is called under cpus_write_lock(). > > "kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock()" hasn't > happened yet. Doh, right. Thanks! > > The lockdep splat below is dependent on future patches to take > > cpus_read_lock() in hardware_enable_all(), but as above, deadlock is > > already is already possible. > > IIUC kvm_lock by design is supposed to protect vm_list, thus IMHO naturally it > doesn't fit to protect multiple vendor module loading. A different way to look at it is that kvm_lock protects anything that is global to all of KVM, and it just so happens that lists and counters of VMs are the only such resources (lumping in the usage in vm_uevent_notify_change() and the future usage to protect kvm_usage_count). > Looks above argument is good enough. I am not sure whether we need additional > justification which comes from future patches. :) To try to prevent someone from trying to eliminate the "extra" lock, like this series does for kvm_count_lock. Hopefully future someones that want to clean up the code do a git blame to understand why the lock was introduced and don't waste their time running into the same issues (or worse, don't run into the issues and break KVM). > Also, do you also want to update Documentation/virt/kvm/locking.rst" in this > patch? Hmm, yeah. That'd also be a good place to document why kvm_lock isn't used.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index a0ca401d3cdf..218707597bea 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -128,6 +128,7 @@ static int kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu); static int __set_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2); static void __get_sregs2(struct kvm_vcpu *vcpu, struct kvm_sregs2 *sregs2); +static DEFINE_MUTEX(vendor_module_lock); struct kvm_x86_ops kvm_x86_ops __read_mostly; #define KVM_X86_OP(func) \ @@ -9280,7 +9281,7 @@ void kvm_arch_exit(void) } -int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) +static int __kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) { u64 host_pat; int r; @@ -9413,6 +9414,17 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) kmem_cache_destroy(x86_emulator_cache); return r; } + +int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops) +{ + int r; + + mutex_lock(&vendor_module_lock); + r = __kvm_x86_vendor_init(ops); + mutex_unlock(&vendor_module_lock); + + return r; +} EXPORT_SYMBOL_GPL(kvm_x86_vendor_init); void kvm_x86_vendor_exit(void) @@ -9435,7 +9447,6 @@ void kvm_x86_vendor_exit(void) cancel_work_sync(&pvclock_gtod_work); #endif static_call(kvm_x86_hardware_unsetup)(); - kvm_x86_ops.hardware_enable = NULL; kvm_mmu_vendor_module_exit(); free_percpu(user_return_msrs); kmem_cache_destroy(x86_emulator_cache); @@ -9443,6 +9454,9 @@ void kvm_x86_vendor_exit(void) static_key_deferred_flush(&kvm_xen_enabled); WARN_ON(static_branch_unlikely(&kvm_xen_enabled.key)); #endif + mutex_lock(&vendor_module_lock); + kvm_x86_ops.hardware_enable = NULL; + mutex_unlock(&vendor_module_lock); } EXPORT_SYMBOL_GPL(kvm_x86_vendor_exit);
Acquire a new mutex, vendor_module_lock, in kvm_x86_vendor_init() while doing hardware setup to ensure that concurrent calls are fully serialized. KVM rejects attempts to load vendor modules if a different module has already been loaded, but doesn't handle the case where multiple vendor modules are loaded at the same time, and module_init() doesn't run under the global module_mutex. Note, in practice, this is likely a benign bug as no platform exists that supports both SVM and VMX, i.e. barring a weird VM setup, one of the vendor modules is guaranteed to fail a support check before modifying common KVM state. Alternatively, KVM could perform an atomic CMPXCHG on .hardware_enable, but that comes with its own ugliness as it would require setting .hardware_enable before success is guaranteed, e.g. attempting to load the "wrong" could result in spurious failure to load the "right" module. Introduce a new mutex as using kvm_lock is extremely deadlock prone due to kvm_lock being taken under cpus_write_lock(), and in the future, under under cpus_read_lock(). Any operation that takes cpus_read_lock() while holding kvm_lock would potentially deadlock, e.g. kvm_timer_init() takes cpus_read_lock() to register a callback. In theory, KVM could avoid such problematic paths, i.e. do less setup under kvm_lock, but avoiding all calls to cpus_read_lock() is subtly difficult and thus fragile. E.g. updating static calls also acquires cpus_read_lock(). Inverting the lock ordering, i.e. always taking kvm_lock outside cpus_read_lock(), is not a viable option, e.g. kvm_online_cpu() takes kvm_lock and is called under cpus_write_lock(). The lockdep splat below is dependent on future patches to take cpus_read_lock() in hardware_enable_all(), but as above, deadlock is already is already possible. ====================================================== WARNING: possible circular locking dependency detected 6.0.0-smp--7ec93244f194-init2 #27 Tainted: G O ------------------------------------------------------ stable/251833 is trying to acquire lock: ffffffffc097ea28 (kvm_lock){+.+.}-{3:3}, at: hardware_enable_all+0x1f/0xc0 [kvm] but task is already holding lock: ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm] which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (cpu_hotplug_lock){++++}-{0:0}: cpus_read_lock+0x2a/0xa0 __cpuhp_setup_state+0x2b/0x60 __kvm_x86_vendor_init+0x16a/0x1870 [kvm] kvm_x86_vendor_init+0x23/0x40 [kvm] 0xffffffffc0a4d02b do_one_initcall+0x110/0x200 do_init_module+0x4f/0x250 load_module+0x1730/0x18f0 __se_sys_finit_module+0xca/0x100 __x64_sys_finit_module+0x1d/0x20 do_syscall_64+0x3d/0x80 entry_SYSCALL_64_after_hwframe+0x63/0xcd -> #0 (kvm_lock){+.+.}-{3:3}: __lock_acquire+0x16f4/0x30d0 lock_acquire+0xb2/0x190 __mutex_lock+0x98/0x6f0 mutex_lock_nested+0x1b/0x20 hardware_enable_all+0x1f/0xc0 [kvm] kvm_dev_ioctl+0x45e/0x930 [kvm] __se_sys_ioctl+0x77/0xc0 __x64_sys_ioctl+0x1d/0x20 do_syscall_64+0x3d/0x80 entry_SYSCALL_64_after_hwframe+0x63/0xcd other info that might help us debug this: Possible unsafe locking scenario: CPU0 CPU1 ---- ---- lock(cpu_hotplug_lock); lock(kvm_lock); lock(cpu_hotplug_lock); lock(kvm_lock); *** DEADLOCK *** 1 lock held by stable/251833: #0: ffffffffa2456828 (cpu_hotplug_lock){++++}-{0:0}, at: hardware_enable_all+0xf/0xc0 [kvm] Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/x86.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)