Re: [PATCH] Documentation: kvm: fix SRCU locking order docs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2023-01-13 at 10:20 +0100, Paolo Bonzini wrote:
> On 1/13/23 08:18, Boqun Feng wrote:
> > On Thu, Jan 12, 2023 at 07:20:48AM -0800, Paul E. McKenney wrote:
> > > On Thu, Jan 12, 2023 at 08:24:16AM +0000, David Woodhouse wrote:
> > > > On Wed, 2023-01-11 at 13:30 -0500, Paolo Bonzini wrote:
> > > > > 
> > > > > +- ``synchronize_srcu(&kvm->srcu)`` is called inside critical sections
> > > > > +  for kvm->lock, vcpu->mutex and kvm->slots_lock.  These locks _cannot_
> > > > > +  be taken inside a kvm->srcu read-side critical section; that is, the
> > > > > +  following is broken::
> > > > > +
> > > > > +      srcu_read_lock(&kvm->srcu);
> > > > > +      mutex_lock(&kvm->slots_lock);
> > > > > +
> > > > 
> > > > "Don't tell me. Tell lockdep!"
> > > > 
> > > > Did we conclude in
> > > > https://lore.kernel.org/kvm/122f38e724aae9ae8ab474233da1ba19760c20d2.camel@xxxxxxxxxxxxx/
> > > > that lockdep *could* be clever enough to catch a violation of this rule
> > > > by itself?
> > > > 
> > > > The general case of the rule would be that 'if mutex A is taken in a
> > > > read-section for SCRU B, then any synchronize_srcu(B) while mutex A is
> > > > held shall be verboten'. And vice versa.
> > > > 
> > > > If we can make lockdep catch it automatically, yay!
> > > 
> > > Unfortunately, lockdep needs to see a writer to complain, and that patch
> > > just adds a reader.  And adding that writer would make lockdep complain
> > > about things that are perfectly fine.  It should be possible to make
> > > lockdep catch this sort of thing, but from what I can see, doing so
> > > requires modifications to lockdep itself.
> > > 
> > 
> > Please see if the follow patchset works:
> > 
> >         https://lore.kernel.org/lkml/20230113065955.815667-1-boqun.feng@xxxxxxxxx
> > 
> > "I have been called. I must answer. Always." ;-)

Amazing! Thank you, Boqun!

> It's missing an important testcase; if it passes (does not warn), then 
> it should work:

I think it does.

I started with kvm/master from
https://git.kernel.org/pub/scm/virt/kvm/kvm.git/log/?h=master so that
lockdep didn't find other things to complain about first. I then:

 • Dropped the last two commits, putting us back to using kvm->lock and
   removing the dummy mutex lock that would have told lockdep that it's
   a (different) problem.

 • I then added Boqun's three commits

 • Reverted a79b53aa so that the srcu_synchronize() deadlock returns

 • Couldn't reproduce with xen_shinfo_test, so added Michal's test from
   https://lore.kernel.org/kvm/15599980-bd2e-b6c2-1479-e1eef02da0b5@xxxxxxx/

The resulting tree is at
https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/kvm-srcu-lockdep


Now when I run tools/testing/selftests/kvm/x86_64/deadlocks_test I do
see lockdep complain about it (shown below; I have a cosmetic
nit/request to make). If I restore the evtchn_reset fix then it also
complains about kvm_xen_set_evtchn() vs. kvm_xen_kvm_set_attr(), and if
I restore the xen_lock fix from the tip of kvm/master then Michal's
deadlock_test passes and there are no complaints.

So everything seems to be working as it should... *except* for the fact
that I don't quite understand why xen_shinfo_test didn't trigger the
warning. Michal, I guess you already worked that out when you came up
with your deadlock-test instead... is there something we should add to
xen_shinfo_test that would mean it *would* have triggered? I even tried
this:

--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1173,6 +1173,16 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
        if (init_srcu_struct(&kvm->irq_srcu))
                goto out_err_no_irq_srcu;
 
+#ifdef CONFIG_LOCKDEP
+       /*
+        * Ensure lockdep knows that it's not permitted to lock kvm->lock
+        * from a SRCU read section on kvm->srcu.
+        */
+       mutex_lock(&kvm->lock);
+       synchronize_srcu(&kvm->srcu);
+       mutex_unlock(&kvm->lock);
+#endif
+
        refcount_set(&kvm->users_count, 1);
        for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
                for (j = 0; j < 2; j++) {




> [  845.474169] ======================================================
> [  845.474170] WARNING: possible circular locking dependency detected
> [  845.474172] 6.2.0-rc3+ #1025 Tainted: G            E     
> [  845.474175] ------------------------------------------------------
> [  845.474176] deadlocks_test/22767 is trying to acquire lock:
> [  845.474178] ffffc9000ba4b868 (&kvm->srcu){.+.+}-{0:0}, at: __synchronize_srcu+0x5/0x170
> [  845.474192] 
>                but task is already holding lock:
> [  845.474194] ffffc9000ba423c0 (&kvm->lock){+.+.}-{3:3}, at: kvm_vm_ioctl_set_msr_filter+0x188/0x220 [kvm]
> [  845.474319] 
>                which lock already depends on the new lock.

So the above part is good, and clearly tells me it was synchronize_srcu()

> [  845.474320] 
>                the existing dependency chain (in reverse order) is:
> [  845.474322] 
>                -> #1 (&kvm->lock){+.+.}-{3:3}:
> [  845.474327]        __lock_acquire+0x4b4/0x940
> [  845.474333]        lock_acquire.part.0+0xa8/0x210
> [  845.474337]        __mutex_lock+0x94/0x920
> [  845.474344]        kvm_xen_set_evtchn.part.0+0x6d/0x170 [kvm]
> [  845.474437]        kvm_xen_inject_timer_irqs+0x79/0xa0 [kvm]
> [  845.474529]        vcpu_run+0x20c/0x450 [kvm]
> [  845.474618]        kvm_arch_vcpu_ioctl_run+0x1df/0x670 [kvm]
> [  845.474707]        kvm_vcpu_ioctl+0x279/0x700 [kvm]
> [  845.474783]        __x64_sys_ioctl+0x8a/0xc0
> [  845.474787]        do_syscall_64+0x3b/0x90
> [  845.474796]        entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  845.474804] 
>                -> #0 (&kvm->srcu){.+.+}-{0:0}:
> [  845.474809]        check_prev_add+0x8f/0xc20
> [  845.474812]        validate_chain+0x3ba/0x450
> [  845.474814]        __lock_acquire+0x4b4/0x940
> [  845.474817]        lock_sync+0x99/0x110
> [  845.474820]        __synchronize_srcu+0x4d/0x170
> [  845.474824]        kvm_vm_ioctl_set_msr_filter+0x1a5/0x220 [kvm]
. [  845.474907]        kvm_arch_vm_ioctl+0x8df/0xd50 [kvm]
> [  845.474997]        kvm_vm_ioctl+0x5ca/0x800 [kvm]
> [  845.475075]        __x64_sys_ioctl+0x8a/0xc0
> [  845.475079]        do_syscall_64+0x3b/0x90
> [  845.475084]        entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  845.475089] 
>                other info that might help us debug this:
>
> [  845.475091]  Possible unsafe locking scenario:
>
> [  845.475092]        CPU0                    CPU1
> [  845.475093]        ----                    ----
> [  845.475095]   lock(&kvm->lock);
> [  845.475098]                                lock(&kvm->srcu);
> [  845.475101]                                lock(&kvm->lock);
> [  845.475103]   lock(&kvm->srcu);
> [  845.475106] 
>                 *** DEADLOCK ***

But is there any chance the above could say 'synchronize_srcu' and
'read_lock_srcu' in the appropriate places?

> [  845.475108] 1 lock held by deadlocks_test/22767:
> [  845.475110]  #0: ffffc9000ba423c0 (&kvm->lock){+.+.}-{3:3}, at: kvm_vm_ioctl_set_msr_filter+0x188/0x220 [kvm]
> [  845.475200] 
>                stack backtrace:
> [  845.475202] CPU: 10 PID: 22767 Comm: deadlocks_test Tainted: G            E      6.2.0-rc3+ #1025
> [  845.475206] Hardware name: Intel Corporation S2600CW/S2600CW, BIOS SE5C610.86B.01.01.0008.021120151325 02/11/2015
> [  845.475208] Call Trace:
> [  845.475210]  <TASK>
> [  845.475214]  dump_stack_lvl+0x56/0x73
> [  845.475221]  check_noncircular+0x102/0x120
> [  845.475229]  ? check_noncircular+0x7f/0x120
> [  845.475236]  check_prev_add+0x8f/0xc20
> [  845.475239]  ? add_chain_cache+0x10b/0x2d0
> [  845.475244]  validate_chain+0x3ba/0x450
> [  845.475249]  __lock_acquire+0x4b4/0x940
> [  845.475253]  ? __synchronize_srcu+0x5/0x170
> [  845.475258]  lock_sync+0x99/0x110
> [  845.475261]  ? __synchronize_srcu+0x5/0x170
> [  845.475265]  __synchronize_srcu+0x4d/0x170
? [  845.475269]  ? mark_held_locks+0x49/0x80
> [  845.475272]  ? _raw_spin_unlock_irqrestore+0x2d/0x60
> [  845.475278]  ? __pfx_read_tsc+0x10/0x10
> [  845.475286]  ? ktime_get_mono_fast_ns+0x3d/0x90
> [  845.475292]  kvm_vm_ioctl_set_msr_filter+0x1a5/0x220 [kvm]
> [  845.475380]  kvm_arch_vm_ioctl+0x8df/0xd50 [kvm]
> [  845.475472]  ? __lock_acquire+0x4b4/0x940
> [  845.475485]  kvm_vm_ioctl+0x5ca/0x800 [kvm]
> [  845.475566]  ? lockdep_unregister_key+0x76/0x110
> [  845.475575]  __x64_sys_ioctl+0x8a/0xc0
> [  845.475579]  do_syscall_64+0x3b/0x90
> [  845.475586]  entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [  845.475591] RIP: 0033:0x7f79de23fd1b
> [  845.475595] Code: 73 01 c3 48 8b 0d 05 a1 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 a0 1b 00 f7 d8 64 89 01 48
> [  845.475598] RSP: 002b:00007f79ddff7c98 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [  845.475602] RAX: ffffffffffffffda RBX: 00007f79ddff8640 RCX: 00007f79de23fd1b
> [  845.475605] RDX: 00007f79ddff7ca0 RSI: 000000004188aec6 RDI: 0000000000000004
> [  845.475607] RBP: 00007f79ddff85c0 R08: 0000000000000000 R09: 00007fffceb1ff2f
> [  845.475609] R10: 0000000000000008 R11: 0000000000000246 R12: 00007f79ddff7ca0
> [  845.475611] R13: 0000000001c322a0 R14: 00007f79de2a05f0 R15: 0000000000000000
> [  845.475617]  </TASK>


Attachment: smime.p7s
Description: S/MIME cryptographic signature


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux