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

[Marcelo Tosatti <mtosatti@xxxxxxxxxx>]

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@xxxxxx>
From: Sheng Yang <sheng.yang@xxxxxxxxx>
Signed-off-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>


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);
 

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);