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

[Alex Williamson <alex.williamson@redhat.com>]

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


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