2.6.29-rc3 circular locking dependency detected

2.6.29-rc3 circular locking dependency detected

[Mark McLoughlin]

Hi,

Just saw this when starting a guest with an assigned device.

Cheers,
Mark.

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.29-0.74.rc3.git3.fc11.x86_64 #1
-------------------------------------------------------
qemu-kvm/3706 is trying to acquire lock:
 (&kvm->lock){--..}, at: [<ffffffffa013a25f>] kvm_emulate_pio+0x1ab/0x1ff [kvm]

but task is already holding lock:
 (&kvm->slots_lock){----}, at: [<ffffffffa013c4c0>] kvm_arch_vcpu_ioctl_run+0x49
7/0x73a [kvm]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&kvm->slots_lock){----}:
       [<ffffffff8106e9c1>] __lock_acquire+0xaab/0xc41
       [<ffffffff8106ebe4>] lock_acquire+0x8d/0xba
       [<ffffffff813826ae>] down_read+0x4b/0x7f
       [<ffffffffa0137ff2>] kvm_iommu_map_guest+0x62/0xb8 [kvm]
       [<ffffffffa01363ea>] kvm_vm_ioctl+0x3f4/0x7f1 [kvm]
       [<ffffffff810eac30>] vfs_ioctl+0x2a/0x78
       [<ffffffff810eb0e9>] do_vfs_ioctl+0x46b/0x4ab
       [<ffffffff810eb17e>] sys_ioctl+0x55/0x77
       [<ffffffff810112ba>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #0 (&kvm->lock){--..}:
       [<ffffffff8106e862>] __lock_acquire+0x94c/0xc41
       [<ffffffff8106ebe4>] lock_acquire+0x8d/0xba
       [<ffffffff8138205a>] __mutex_lock_common+0x107/0x39c
       [<ffffffff81382398>] mutex_lock_nested+0x35/0x3a
       [<ffffffffa013a25f>] kvm_emulate_pio+0x1ab/0x1ff [kvm]
       [<ffffffffa015c875>] handle_io+0x6e/0x76 [kvm_intel]
       [<ffffffffa015d202>] kvm_handle_exit+0x1ba/0x1db [kvm_intel]
       [<ffffffffa013c534>] kvm_arch_vcpu_ioctl_run+0x50b/0x73a [kvm]
       [<ffffffffa01344a7>] kvm_vcpu_ioctl+0xfc/0x48b [kvm]
       [<ffffffff810eac30>] vfs_ioctl+0x2a/0x78
       [<ffffffff810eb0e9>] do_vfs_ioctl+0x46b/0x4ab
       [<ffffffff810eb17e>] sys_ioctl+0x55/0x77
       [<ffffffff810112ba>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

2 locks held by qemu-kvm/3706:
 #0:  (&vcpu->mutex){--..}, at: [<ffffffffa0136ceb>] vcpu_load+0x15/0x37 [kvm]
 #1:  (&kvm->slots_lock){----}, at: [<ffffffffa013c4c0>] kvm_arch_vcpu_ioctl_run
+0x497/0x73a [kvm]

stack backtrace:
Pid: 3706, comm: qemu-kvm Not tainted 2.6.29-0.74.rc3.git3.fc11.x86_64 #1
Call Trace:
 [<ffffffff8106dc65>] print_circular_bug_tail+0x71/0x7c
 [<ffffffff8106e862>] __lock_acquire+0x94c/0xc41
 [<ffffffff8106ebe4>] lock_acquire+0x8d/0xba
 [<ffffffffa013a25f>] ? kvm_emulate_pio+0x1ab/0x1ff [kvm]
 [<ffffffff8138205a>] __mutex_lock_common+0x107/0x39c
 [<ffffffffa013a25f>] ? kvm_emulate_pio+0x1ab/0x1ff [kvm]
 [<ffffffffa013a25f>] ? kvm_emulate_pio+0x1ab/0x1ff [kvm]
 [<ffffffff81382398>] mutex_lock_nested+0x35/0x3a
 [<ffffffffa013a25f>] kvm_emulate_pio+0x1ab/0x1ff [kvm]
 [<ffffffffa015b695>] ? kvm_register_read+0x26/0x35 [kvm_intel]
 [<ffffffffa015c875>] handle_io+0x6e/0x76 [kvm_intel]
 [<ffffffffa015d202>] kvm_handle_exit+0x1ba/0x1db [kvm_intel]
 [<ffffffffa013c534>] kvm_arch_vcpu_ioctl_run+0x50b/0x73a [kvm]
 [<ffffffffa01344a7>] kvm_vcpu_ioctl+0xfc/0x48b [kvm]
 [<ffffffff81163618>] ? inode_has_perm+0x6c/0x72
 [<ffffffff810eac30>] vfs_ioctl+0x2a/0x78
 [<ffffffff810eb0e9>] do_vfs_ioctl+0x46b/0x4ab
 [<ffffffff810eb17e>] sys_ioctl+0x55/0x77
 [<ffffffff810112ba>] system_call_fastpath+0x16/0x1b

Re: 2.6.29-rc3 circular locking dependency detected

[Avi Kivity]

Mark McLoughlin wrote:
> Hi,
>
> Just saw this when starting a guest with an assigned device.
>
> Cheers,
> Mark.
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.29-0.74.rc3.git3.fc11.x86_64 #1
> -------------------------------------------------------
> qemu-kvm/3706 is trying to acquire lock:
>  (&kvm->lock){--..}, at: [<ffffffffa013a25f>] kvm_emulate_pio+0x1ab/0x1ff [kvm]
>
> but task is already holding lock:
>  (&kvm->slots_lock){----}, at: [<ffffffffa013c4c0>] kvm_arch_vcpu_ioctl_run+0x49
> 7/0x73a [kvm]
>   

This is the expected nesting. kvm->slots_lock is outer to kvm->lock.

> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&kvm->slots_lock){----}:
>        [<ffffffff8106e9c1>] __lock_acquire+0xaab/0xc41
>        [<ffffffff8106ebe4>] lock_acquire+0x8d/0xba
>        [<ffffffff813826ae>] down_read+0x4b/0x7f
>        [<ffffffffa0137ff2>] kvm_iommu_map_guest+0x62/0xb8 [kvm]
>        [<ffffffffa01363ea>] kvm_vm_ioctl+0x3f4/0x7f1 [kvm]
>        [<ffffffff810eac30>] vfs_ioctl+0x2a/0x78
>        [<ffffffff810eb0e9>] do_vfs_ioctl+0x46b/0x4ab
>        [<ffffffff810eb17e>] sys_ioctl+0x55/0x77
>        [<ffffffff810112ba>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
>   

I think taking slots_lock in kvm_vm_ioctl_assign_device() (and dropping 
it from kvm_iommu_map_guest) should suffice, no?


-- 
error compiling committee.c: too many arguments to function

Re: 2.6.29-rc3 circular locking dependency detected

[Mark McLoughlin]

On Tue, 2009-02-03 at 12:47 +0200, Avi Kivity wrote:
> Mark McLoughlin wrote:

> > which lock already depends on the new lock.
> >
> > the existing dependency chain (in reverse order) is:
> >
> > -> #1 (&kvm->slots_lock){----}:
> >        [<ffffffff8106e9c1>] __lock_acquire+0xaab/0xc41
> >        [<ffffffff8106ebe4>] lock_acquire+0x8d/0xba
> >        [<ffffffff813826ae>] down_read+0x4b/0x7f
> >        [<ffffffffa0137ff2>] kvm_iommu_map_guest+0x62/0xb8 [kvm]
> >        [<ffffffffa01363ea>] kvm_vm_ioctl+0x3f4/0x7f1 [kvm]
> >        [<ffffffff810eac30>] vfs_ioctl+0x2a/0x78
> >        [<ffffffff810eb0e9>] do_vfs_ioctl+0x46b/0x4ab
> >        [<ffffffff810eb17e>] sys_ioctl+0x55/0x77
> >        [<ffffffff810112ba>] system_call_fastpath+0x16/0x1b
> >        [<ffffffffffffffff>] 0xffffffffffffffff
> >   
> 
> I think taking slots_lock in kvm_vm_ioctl_assign_device() (and dropping 
> it from kvm_iommu_map_guest) should suffice, no?

Just from a quick look, that seems right - also need to remove the
locking from kvm_iommu_unmap_memslots()

Cheers,
Mark.

[PATCH 1/1] kvm: fix circular locking dependency

[Mark McLoughlin]

kvm->slots_lock is outer to kvm->lock, so take slots_lock
in kvm_vm_ioctl_assign_device() before taking kvm->lock,
rather than taking it in kvm_iommu_map_memslots().

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 virt/kvm/iommu.c    |    6 ++----
 virt/kvm/kvm_main.c |    3 +++
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index e9693a2..4c40375 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -73,14 +73,13 @@ static int kvm_iommu_map_memslots(struct kvm *kvm)
 {
 	int i, r = 0;
 
-	down_read(&kvm->slots_lock);
 	for (i = 0; i < kvm->nmemslots; i++) {
 		r = kvm_iommu_map_pages(kvm, kvm->memslots[i].base_gfn,
 					kvm->memslots[i].npages);
 		if (r)
 			break;
 	}
-	up_read(&kvm->slots_lock);
+
 	return r;
 }
 
@@ -190,12 +189,11 @@ static void kvm_iommu_put_pages(struct kvm *kvm,
 static int kvm_iommu_unmap_memslots(struct kvm *kvm)
 {
 	int i;
-	down_read(&kvm->slots_lock);
+
 	for (i = 0; i < kvm->nmemslots; i++) {
 		kvm_iommu_put_pages(kvm, kvm->memslots[i].base_gfn,
 				    kvm->memslots[i].npages);
 	}
-	up_read(&kvm->slots_lock);
 
 	return 0;
 }
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3a5a082..dc948c6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -455,6 +455,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 	struct kvm_assigned_dev_kernel *match;
 	struct pci_dev *dev;
 
+	down_read(&kvm->slots_lock);
 	mutex_lock(&kvm->lock);
 
 	match = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
@@ -516,6 +517,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm,
 
 out:
 	mutex_unlock(&kvm->lock);
+	up_read(&kvm->slots_lock);
 	return r;
 out_list_del:
 	list_del(&match->list);
@@ -527,6 +529,7 @@ out_put:
 out_free:
 	kfree(match);
 	mutex_unlock(&kvm->lock);
+	up_read(&kvm->slots_lock);
 	return r;
 }
 #endif
-- 
1.6.0.6

Re: [PATCH 1/1] kvm: fix circular locking dependency

[Chris Wright]

* Mark McLoughlin (markmc@redhat.com) wrote:
> kvm->slots_lock is outer to kvm->lock, so take slots_lock
> in kvm_vm_ioctl_assign_device() before taking kvm->lock,
> rather than taking it in kvm_iommu_map_memslots().

stable?  maint/2.6.29?

Re: [PATCH 1/1] kvm: fix circular locking dependency

[Mark McLoughlin]

On Thu, 2009-02-05 at 10:52 -0800, Chris Wright wrote:
> * Mark McLoughlin (markmc@redhat.com) wrote:
> > kvm->slots_lock is outer to kvm->lock, so take slots_lock
> > in kvm_vm_ioctl_assign_device() before taking kvm->lock,
> > rather than taking it in kvm_iommu_map_memslots().
> 
> stable?  maint/2.6.29?

Yep, my bad - Avi, please add:

Cc: stable@kernel.org

and pull into maint/2.6.29

Thanks,
Mark.

Re: [PATCH 1/1] kvm: fix circular locking dependency

[Marcelo Tosatti]

On Fri, Feb 06, 2009 at 08:57:31AM +0000, Mark McLoughlin wrote:
> On Thu, 2009-02-05 at 10:52 -0800, Chris Wright wrote:
> > * Mark McLoughlin (markmc@redhat.com) wrote:
> > > kvm->slots_lock is outer to kvm->lock, so take slots_lock
> > > in kvm_vm_ioctl_assign_device() before taking kvm->lock,
> > > rather than taking it in kvm_iommu_map_memslots().
> > 
> > stable?  maint/2.6.29?
> 
> Yep, my bad - Avi, please add:
> 
> Cc: stable@kernel.org
> 
> and pull into maint/2.6.29

ACK.

Re: [PATCH 1/1] kvm: fix circular locking dependency

[Avi Kivity]

Marcelo Tosatti wrote:
> On Fri, Feb 06, 2009 at 08:57:31AM +0000, Mark McLoughlin wrote:
>   
>> On Thu, 2009-02-05 at 10:52 -0800, Chris Wright wrote:
>>     
>>> * Mark McLoughlin (markmc@redhat.com) wrote:
>>>       
>>>> kvm->slots_lock is outer to kvm->lock, so take slots_lock
>>>> in kvm_vm_ioctl_assign_device() before taking kvm->lock,
>>>> rather than taking it in kvm_iommu_map_memslots().
>>>>         

Applied, but,

>>> stable?  maint/2.6.29?
>>>       
>> Yep, my bad - Avi, please add:
>>
>> Cc: stable@kernel.org
>>
>> and pull into maint/2.6.29
>>     
>
> ACK.
>
>   

At this point in maint/2.6.29's lifetime, it gets updated by pulling 
from Linus, and stable is only updated after Linus is updated.

(It's the same rule, really -- only commit to a maintenance branch after 
upstream has been fixed, to ensure we don't have a maint fix without a 
corresponding upstream fix)

-- 
error compiling committee.c: too many arguments to function