kvm: lockdep annotate mmu_lock wait_lock

PATCH In review Marcelo Tosatti 5 participants 8 messages Jul 18, 2017 – Aug 7, 2017 KVM

lore

Share & tools

lore

Marcelo Tosatti

author

2017-07-18 12:03 UTC #0 lore raw

PATCH 1/1 kvm: lockdep annotate mmu_lock wait_lock

wait_lock for mmu_lock, being allocated in kmalloc memory, can't be tracked by LOCKDEP.

Disable LOCKDEP verification for it.

Fixes

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

 CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1
 Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011
 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc
 ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565
 ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000
 Call Trace:
 [<ffffffffa57534fc>] dump_stack+0x19/0x1b
 [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c
 [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0
 [<ffffffffa510efb2>] lock_acquire+0xb2/0x230
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60
 [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm]
 [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm]
 [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm]
 [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm]
 [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0
 [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0
 [<ffffffffa57656ed>] tracesys+0xdd/0xe2
 ---------------------------
 | preempt count: 00000001 ]
 | 1-level deep critical section nesting:
 ----------------------------------------
 .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0
 .....[<ffffffffa5759ccd>] ..   ( <= rt_spin_lock_slowlock+0x6d/0x3c0)

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/kvm_main.c +2 -0

index 7e80f62..6375db8 100644
@@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		return ERR_PTR(-ENOMEM);
 
 	spin_lock_init(&kvm->mmu_lock);
+
+	lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
 	mmgrab(current->mm);
 	kvm->mm = current->mm;
 	kvm_eventfd_init(kvm);

Peter Zijlstra

2017-07-18 12:16 UTC #1 lore raw

Quoted 48 lines · Marcelo Tosatti

wait_lock for mmu_lock, being allocated in kmalloc memory, can't be tracked by LOCKDEP.

Disable LOCKDEP verification for it.

Fixes

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

 CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1
 Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011
 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc
 ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565
 ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000
 Call Trace:
 [<ffffffffa57534fc>] dump_stack+0x19/0x1b
 [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c
 [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0
 [<ffffffffa510efb2>] lock_acquire+0xb2/0x230
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60
 [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm]
 [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm]
 [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm]
 [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm]
 [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0
 [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0
 [<ffffffffa57656ed>] tracesys+0xdd/0xe2
 ---------------------------
 | preempt count: 00000001 ]
 | 1-level deep critical section nesting:
 ----------------------------------------
 .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0
 .....[<ffffffffa5759ccd>] ..   ( <= rt_spin_lock_slowlock+0x6d/0x3c0)

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/kvm_main.c +0 -0

index 7e80f62..6375db8 100644

virt/kvm/kvm_main.c static struct kvm *kvm_create_vm(unsigned long type) · line 620

Quoted 8 lines

virt/kvm/kvm_main.c +2 -0

@@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		return ERR_PTR(-ENOMEM);
 
 	spin_lock_init(&kvm->mmu_lock);
+
+	lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
 	mmgrab(current->mm);
 	kvm->mm = current->mm;
 	kvm_eventfd_init(kvm);

This doesn't make sense... It looks like a spin_lock_init() is missing, at which point it'll try and use the lock address itself and then bails because that is in dynamic memory.

That doesn't mean you cannot initialize it properly.

And using novalidate for this is absolutely bonkers.

Marcelo Tosatti

author

2017-07-18 12:22 UTC #2 lore raw

Quoted 51 lines · Peter Zijlstra

Quoted 50 lines · Marcelo Tosatti

wait_lock for mmu_lock, being allocated in kmalloc memory, can't be tracked by LOCKDEP.

Disable LOCKDEP verification for it.

Fixes

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

 CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1
 Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011
 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc
 ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565
 ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000
 Call Trace:
 [<ffffffffa57534fc>] dump_stack+0x19/0x1b
 [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c
 [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0
 [<ffffffffa510efb2>] lock_acquire+0xb2/0x230
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0
 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0
 [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60
 [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm]
 [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm]
 [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm]
 [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm]
 [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0
 [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0
 [<ffffffffa57656ed>] tracesys+0xdd/0xe2
 ---------------------------
 | preempt count: 00000001 ]
 | 1-level deep critical section nesting:
 ----------------------------------------
 .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0
 .....[<ffffffffa5759ccd>] ..   ( <= rt_spin_lock_slowlock+0x6d/0x3c0)

Signed-off-by Marcelo Tosatti <[email protected]>

virt/kvm/kvm_main.c +0 -0

index 7e80f62..6375db8 100644
@@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		return ERR_PTR(-ENOMEM);

Quoted 10 lines

virt/kvm/kvm_main.c static struct kvm *kvm_create_vm(unsigned long type) · line 620
Quoted 6 lines
virt/kvm/kvm_main.c +2 -0
 	spin_lock_init(&kvm->mmu_lock);
+
+	lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
 	mmgrab(current->mm);
 	kvm->mm = current->mm;
 	kvm_eventfd_init(kvm);
This doesn't make sense... It looks like a spin_lock_init() is missing, at which point it'll try and use the lock address itself and then bails because that is in dynamic memory.

Do you see the spin_lock_init just above, after "return PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose.

"struct kvm" (which contains the mmu_lock spinlock) is allocated with kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory?

Quoted 3 lines

That doesn't mean you cannot initialize it properly.
And using novalidate for this is absolutely bonkers.

I imagined so, not entirely sure what is the proper LOCKDEP annotation in this case...

Peter Zijlstra

2017-07-18 12:45 UTC #3 lore raw

Quoted 18 lines · Marcelo Tosatti

Quoted 7 lines
Quoted 7 lines
virt/kvm/kvm_main.c +0 -0
index 7e80f62..6375db8 100644
@@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		return ERR_PTR(-ENOMEM);
 
Quoted 10 lines
virt/kvm/kvm_main.c static struct kvm *kvm_create_vm(unsigned long type) · line 620
Quoted 6 lines
virt/kvm/kvm_main.c +2 -0
 	spin_lock_init(&kvm->mmu_lock);
+
+	lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
 	mmgrab(current->mm);
 	kvm->mm = current->mm;
 	kvm_eventfd_init(kvm);
This doesn't make sense... It looks like a spin_lock_init() is missing, at which point it'll try and use the lock address itself and then bails because that is in dynamic memory.

Quoted 2 lines

Do you see the spin_lock_init just above, after "return PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose.

D'0h... so much for being 'awake'..

Quoted 2 lines

"struct kvm" (which contains the mmu_lock spinlock) is allocated with kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory?

Yep, what _should_ happen is that we use a macro like:

(I don't have an -RT tree handy atm)

# define raw_spin_lock_init(lock)                               \
do {                                                            \
        static struct lock_class_key __key;                     \
                                                                \
        __raw_spin_lock_init((lock), #lock, &__key);            \
} while (0)

Which defines a key per callsite. So while the lock itself is in dynamic memory, the key will be static.

Julia Cartwright

2017-07-19 20:31 UTC #4 lore raw

Quoted 38 lines · Peter Zijlstra

Quoted 18 lines · Marcelo Tosatti
Quoted 7 lines
Quoted 7 lines
virt/kvm/kvm_main.c +0 -0
index 7e80f62..6375db8 100644
@@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 		return ERR_PTR(-ENOMEM);
 
Quoted 10 lines
virt/kvm/kvm_main.c static struct kvm *kvm_create_vm(unsigned long type) · line 620
Quoted 6 lines
virt/kvm/kvm_main.c +2 -0
 	spin_lock_init(&kvm->mmu_lock);
+
+	lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
 	mmgrab(current->mm);
 	kvm->mm = current->mm;
 	kvm_eventfd_init(kvm);
This doesn't make sense... It looks like a spin_lock_init() is missing, at which point it'll try and use the lock address itself and then bails because that is in dynamic memory.
Quoted 2 lines
Do you see the spin_lock_init just above, after "return PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose.
D'0h... so much for being 'awake'..
Quoted 2 lines
"struct kvm" (which contains the mmu_lock spinlock) is allocated with kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory?
Yep, what _should_ happen is that we use a macro like:
(I don't have an -RT tree handy atm)
# define raw_spin_lock_init(lock)                               \
do {                                                            \
        static struct lock_class_key __key;                     \
                                                                \
        __raw_spin_lock_init((lock), #lock, &__key);            \
} while (0)

Quoted 2 lines

Which defines a key per callsite. So while the lock itself is in dynamic memory, the key will be static.

One other interesting observation: the dump_stack() comes from the acquire path, not from the _init() path, even though the _init() path itself includes a check to ensure that the lock_class_key points to static data. (__raw_spin_lock_init -> lockdep_init_map -> if (!static_obj(key)) BUG)

So, that would lead me to believe that if we got this far, at some point lock->key _was_ pointing to static data, but now is not?

Julia

Caveat: I'm looking at v3.10.53-rt56; I guessed it was the closest public stable tag to the undecipherable RHEL version.

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

kbuild test robot

build bot

2017-07-19 15:59 UTC #5 lore raw

In reply to Marcelo Tosatti

reproduce:

Hi Marcelo,

[auto build test ERROR on linux-rt-devel/for-kbuild-bot/current-stable]

url:    https://github.com/0day-ci/linux/commits/Marcelo-Tosatti/kvm-lockdep-annotate-mmu_lock-wait_lock/20170719-025327
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git for-kbuild-bot/current-stable
config: x86_64-allyesdebian (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

All errors (new ones prefixed by >>):

In file included from include/linux/hardirq.h:5:0,

                    from include/linux/kvm_host.h:10,
                    from arch/x86/kvm/../../../virt/kvm/kvm_main.c:21:
   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_create_vm':
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:45: error: 'spinlock_t {aka struct spinlock}' has no member named 'lock'; did you mean 'rlock'?
     lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
                                                ^
   include/linux/lockdep.h:300:22: note: in definition of macro 'lockdep_set_class_and_name'
      lockdep_init_map(&(lock)->dep_map, name, key, 0)
                         ^~~~
   arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:2: note: in expansion of macro 'lockdep_set_novalidate_class'
     lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +617 arch/x86/kvm/../../../virt/kvm/kvm_main.c

606
607 static struct kvm *kvm_create_vm(unsigned long type)
608 {
609 int r, i;
610 struct kvm *kvm = kvm_arch_alloc_vm();
611
612 if (!kvm)
613 return ERR_PTR(-ENOMEM);
614
615 spin_lock_init(&kvm->mmu_lock);
616

 > 617		lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);

618 mmgrab(current->mm);
619 kvm->mm = current->mm;
620 kvm_eventfd_init(kvm);
621 mutex_init(&kvm->lock);
622 mutex_init(&kvm->irq_lock);
623 mutex_init(&kvm->slots_lock);
624 refcount_set(&kvm->users_count, 1);
625 INIT_LIST_HEAD(&kvm->devices);
626
627 r = kvm_arch_init_vm(kvm, type);
628 if (r)
629 goto out_err_no_disable;
630
631 r = hardware_enable_all();
632 if (r)
633 goto out_err_no_disable;
634

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2017-07-20 02:35 UTC #6 lore raw

In reply to Marcelo Tosatti

reproduce:

Hi Marcelo,

[auto build test WARNING on linux-rt-devel/for-kbuild-bot/current-stable]

url:    https://github.com/0day-ci/linux/commits/Marcelo-Tosatti/kvm-lockdep-annotate-mmu_lock-wait_lock/20170719-025327
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git for-kbuild-bot/current-stable
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

In file included from include/linux/hardirq.h:5:0,

                    from include/linux/kvm_host.h:10,
                    from arch/x86/kvm/../../../virt/kvm/kvm_main.c:21:
   arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_create_vm':
   arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:45: error: 'spinlock_t {aka struct spinlock}' has no member named 'lock'; did you mean 'rlock'?
     lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
                                                ^
   include/linux/lockdep.h:300:22: note: in definition of macro 'lockdep_set_class_and_name'
      lockdep_init_map(&(lock)->dep_map, name, key, 0)
                         ^~~~
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:2: note: in expansion of macro 'lockdep_set_novalidate_class'
     lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/lockdep_set_novalidate_class +617 arch/x86/kvm/../../../virt/kvm/kvm_main.c

 > 617		lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Sebastian Andrzej Siewior

2017-08-07 13:35 UTC #7 lore raw

In reply to Marcelo Tosatti

Quoted 3 lines · Marcelo Tosatti

wait_lock for mmu_lock, being allocated in kmalloc memory, can't be tracked by LOCKDEP.

there is:

	struct kvm *kvm = kvm_arch_alloc_vm(); 

	spin_lock_init(&kvm->mmu_lock);

and kvm_arch_alloc_vm() itself is "kzalloc(sizeof(struct kvm), GFP_KERNEL);". I don't see anything wrong with it. This does not explain why this lockdep warning.

Quoted 8 lines

Disable LOCKDEP verification for it.
Fixes
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.

not convinced.

Quoted 1 line

 CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1

oh.

Sebastian