[RFC PATCH] vfio-pci: avoid deadlock between unbind and VFIO_DEVICE_RESET

[Alex Williamson]

On Mon, 2014-03-03 at 12:28 -0300, Thadeu Lima de Souza Cascardo wrote:
> On Mon, Mar 03, 2014 at 08:09:22AM -0700, Alex Williamson wrote:
> > On Mon, 2014-03-03 at 11:33 -0300, Thadeu Lima de Souza Cascardo wrote:
> > > When we unbind vfio-pci from a device, while running a guest, we might
> > > have a deadlock when such a guest reboots.
> > > 
> > > Unbind takes device_lock at device_release_driver, and waits for
> > > release_q at vfio_del_group_dev.
> > > 
> > > release_q will only be woken up when all references to vfio_device are
> > > gone, and that includes open file descriptors, like the ones a guest
> > > on qemu will hold.
> > > 
> > > If you try to reboot the guest, it will call VFIO_DEVICE_RESET, which
> > > calls pci_reset_function, which now grabs the device_lock, and we are
> > > deadlocked.
> > > 
> > > Using device_trylock allow us to handle the case when the lock is
> > > already taken, and avoid this situation.
> > > 
> > > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at linux.vnet.ibm.com>
> > > ---
> > > 
> > > Not tested yet, but I would like some comments now, like would it be
> > > better to have a pci_try_reset_function, or do trylock on
> > > pci_reset_function itself?
> > 
> > 
> > We already have it:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=61cf16d8bd38c3dc52033ea75d5b1f8368514a17
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=890ed578df82f5b7b5a874f9f2fa4f117305df5f
> > 
> > Is there something insufficient about these or are you testing on and
> > older kernel?  Thanks,
> > 
> > Alex
> 
> Sorry I missed it. On the rush to report and fix it, I looked only on my
> local branch. Should we backport those two patches to long term stable
> 3.10? I can reproduce the issue there.

Sure, if you're trying to exercise this path, it's easy to reproduce.
It's also relatively easy to avoid once you know it's there.  It's not
obvious to me that this fix meets the stable patch rules though, it's
bigger than suggested, I'm not sure it really bothers people outside of
QA testing, it does cause a hang, but not a system hang.  I'd certainly
suggest any downstream based on 3.10 that cares about vfio to pick it
up, does that make it sufficient for upstream stable?  Thanks,

Alex
 
> > > ---
> > >  drivers/vfio/pci/vfio_pci.c |   14 ++++++++++++--
> > >  1 files changed, 12 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > index 3b76dc8..d1d2242 100644
> > > --- a/drivers/vfio/pci/vfio_pci.c
> > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > @@ -513,8 +513,18 @@ static long vfio_pci_ioctl(void *device_data,
> > >  		return ret;
> > >  
> > >  	} else if (cmd == VFIO_DEVICE_RESET) {
> > > -		return vdev->reset_works ?
> > > -			pci_reset_function(vdev->pdev) : -EINVAL;
> > > +		struct pci_dev *pdev = vdev->pdev;
> > > +		int ret = -EBUSY;
> > > +		if (!vdev->reset_works)
> > > +			return -EINVAL;
> > > +		if (pci_cfg_access_trylock(pdev)) {
> > > +			if (device_trylock(&pdev->dev)) {
> > > +				ret = __pci_reset_function_locked(pdev);
> > > +				device_unlock(&pdev->dev);
> > > +			}
> > > +			pci_cfg_access_unlock(pdev);
> > > +		}
> > > +		return ret;
> > >  
> > >  	} else if (cmd == VFIO_DEVICE_GET_PCI_HOT_RESET_INFO) {
> > >  		struct vfio_pci_hot_reset_info hdr;
> > 
> > 
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html