Re: [PATCH v3 1/8] KVM: Use dedicated mutex to protect kvm_usage_count to avoid deadlock

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 15, 2024, Paolo Bonzini wrote:
> On Thu, Aug 15, 2024 at 4:40 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Aug 14, 2024, Paolo Bonzini wrote:
> > > On 6/8/24 02:06, Sean Christopherson wrote:
> > > > 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()
> > >
> > > Offhand I couldn't see any places where {,__}cpufreq_driver_target() is
> > > called within cpus_read_lock().  I didn't look too closely though.
> >
> > Anyways...
> >
> >   cpuhp_cpufreq_online()
> >   |
> >   -> cpufreq_online()
> >      |
> >      -> cpufreq_gov_performance_limits()
> >         |
> >         -> __cpufreq_driver_target()
> >            |
> >            -> __target_index()
> 
> Ah, I only looked in generic code.
> 
> Can you add a comment to the comment message suggesting switching the vm_list
> to RCU? All the occurrences of list_for_each_entry(..., &vm_list, ...) seem
> amenable to that, and it should be as easy to stick all or part of
> kvm_destroy_vm() behind call_rcu().

+1 to the idea of making vm_list RCU-protected, though I think we'd want to use
SRCU, e.g. set_nx_huge_pages() currently takes eash VM's slots_lock while purging
possible NX hugepages.

And I think kvm_destroy_vm() can simply do a synchronize_srcu() after removing
the VM from the list.  Trying to put kvm_destroy_vm() into an RCU callback would
probably be a bit of a disaster, e.g. kvm-intel.ko in particular currently does
some rather nasty things while destory a VM.