Re: [PATCH 1/1] KVM: Fix potentially recursively get kvm lock

Marcelo Tosatti
2009-05-12T22:13:49Z

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

On Tue, May 12, 2009 at 03:36:27PM -0600, Alex Williamson wrote:
> On Tue, 2009-05-12 at 16:44 -0300, Marcelo Tosatti wrote:
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 4d00942..ba067db 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
> > disable_irq_nosync(assigned_dev->
> > host_msix_entries[i].vector);
> >
> > + /*
> > + * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock
> > + * with cancel_work_sync, since it requires kvm->lock for irq
> > + * injection. This is a hack, the irq code must use
> > + * a separate lock.
> > + */
> > + mutex_unlock(&kvm->lock);
> > cancel_work_sync(&assigned_dev->interrupt_work);
> > + mutex_lock(&kvm->lock);
>
> Seems to work, I assume you've got a similar unlock/lock for the
> MSI/INTx block. Thanks,

KVM: workaround workqueue / deassign_host_irq deadlock

I think I'm running into the following deadlock in the kvm kernel module
when trying to use device assignment:

CPU A CPU B
kvm_vm_ioctl_deassign_dev_irq()
mutex_lock(&kvm->lock); worker_thread()
-> kvm_deassign_irq() ->
kvm_assigned_dev_interrupt_work_handler()
-> deassign_host_irq() mutex_lock(&kvm->lock);
-> cancel_work_sync() [blocked]

Workaround the issue by dropping kvm->lock for cancel_work_sync().

Reported-by: Alex Williamson <alex.williamson@hp.com>
From: Sheng Yang <sheng.yang@intel.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4d00942..d4af719 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
disable_irq_nosync(assigned_dev->
host_msix_entries[i].vector);

+ /*
+ * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock
+ * with cancel_work_sync, since it requires kvm->lock for irq
+ * injection. This is a hack, the irq code must use
+ * a separate lock. Same below for MSI.
+ */
+ mutex_unlock(&kvm->lock);
cancel_work_sync(&assigned_dev->interrupt_work);
+ mutex_lock(&kvm->lock);

for (i = 0; i < assigned_dev->entries_nr; i++)
free_irq(assigned_dev->host_msix_entries[i].vector,
@@ -263,7 +271,9 @@ static void deassign_host_irq(struct kvm *kvm,
} else {
/* Deal with MSI and INTx */
disable_irq_nosync(assigned_dev->host_irq);
+ mutex_unlock(&kvm->lock);
cancel_work_sync(&assigned_dev->interrupt_work);
+ mutex_lock(&kvm->lock);

free_irq(assigned_dev->host_irq, (void *)assigned_dev);

[-- Attachment #2: assigned-dev-cancel-work-deadlock.patch --]
[-- Type: text/plain, Size: 1070 bytes --]

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4d00942..d4af719 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -250,7 +250,15 @@ static void deassign_host_irq(struct kvm *kvm,
disable_irq_nosync(assigned_dev->
host_msix_entries[i].vector);

+ /*
+ * FIXME: kvm_assigned_dev_interrupt_work_handler can deadlock
+ * with cancel_work_sync, since it requires kvm->lock for irq
+ * injection. This is a hack, the irq code must use
+ * a separate lock. Same below for MSI.
+ */
+ mutex_unlock(&kvm->lock);
cancel_work_sync(&assigned_dev->interrupt_work);
+ mutex_lock(&kvm->lock);

for (i = 0; i < assigned_dev->entries_nr; i++)
free_irq(assigned_dev->host_msix_entries[i].vector,
@@ -263,7 +271,9 @@ static void deassign_host_irq(struct kvm *kvm,
} else {
/* Deal with MSI and INTx */
disable_irq_nosync(assigned_dev->host_irq);
+ mutex_unlock(&kvm->lock);
cancel_work_sync(&assigned_dev->interrupt_work);
+ mutex_lock(&kvm->lock);

free_irq(assigned_dev->host_irq, (void *)assigned_dev);