linux-kernel - Re: [PATCH v3 1/8] KVM: Use dedicated mutex to protect kvm_usage

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABgObfZSCZ-dgK3zWao573+RmZSPhnaoMsrify9-48UVhbKVdw@mail.gmail.com>
Date: Thu, 15 Aug 2024 18:10:21 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Chao Gao <chao.gao@...el.com>, Kai Huang <kai.huang@...el.com>
Subject: Re: [PATCH v3 1/8] KVM: Use dedicated mutex to protect
 kvm_usage_count to avoid deadlock

On Thu, Aug 15, 2024 at 4:40 PM Sean Christopherson <seanjc@...gle.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().

Thanks,

Paolo