[PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[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 <mtosatti@redhat.com>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7e80f62..6375db8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[Peter Zijlstra]

On Tue, Jul 18, 2017 at 09:03:47AM -0300, Marcelo Tosatti wrote:
> 
> 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 <mtosatti@redhat.com>
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 7e80f62..6375db8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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.

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[Marcelo Tosatti]

On Tue, Jul 18, 2017 at 02:16:06PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 18, 2017 at 09:03:47AM -0300, Marcelo Tosatti wrote:
> > 
> > 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 <mtosatti@redhat.com>
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 7e80f62..6375db8 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -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.

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?

> 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...

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[Peter Zijlstra]

On Tue, Jul 18, 2017 at 09:22:38AM -0300, Marcelo Tosatti wrote:
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 7e80f62..6375db8 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -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.
> 
> 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'..

> "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.

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38522 bytes --]

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[Julia Cartwright]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

On Tue, Jul 18, 2017 at 02:45:04PM +0200, Peter Zijlstra wrote:
> On Tue, Jul 18, 2017 at 09:22:38AM -0300, Marcelo Tosatti wrote:
> > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > > index 7e80f62..6375db8 100644
> > > > --- a/virt/kvm/kvm_main.c
> > > > +++ b/virt/kvm/kvm_main.c
> > > > @@ -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.
> > 
> > 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'..
> 
> > "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.

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2500 bytes --]

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

   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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59017 bytes --]

Re: [PATCH -rt] kvm: lockdep annotate mmu_lock wait_lock

[Sebastian Andrzej Siewior]

On 2017-07-18 09:03:47 [-0300], Marcelo Tosatti wrote:
> 
> 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.

> 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. 

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

Sebastian