| From 93899a679fd6b2534b5c297d9316bae039ebcbe1 Mon Sep 17 00:00:00 2001 |
| From: Alex Williamson <alex.williamson@redhat.com> |
| Date: Mon, 29 Sep 2014 17:18:39 -0600 |
| Subject: vfio-pci: Fix remove path locking |
| |
| From: Alex Williamson <alex.williamson@redhat.com> |
| |
| commit 93899a679fd6b2534b5c297d9316bae039ebcbe1 upstream. |
| |
| Locking both the remove() and release() path results in a deadlock |
| that should have been obvious. To fix this we can get and hold the |
| vfio_device reference as we evaluate whether to do a bus/slot reset. |
| This will automatically block any remove() calls, allowing us to |
| remove the explict lock. Fixes 61d792562b53. |
| |
| Signed-off-by: Alex Williamson <alex.williamson@redhat.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/vfio/pci/vfio_pci.c | 138 ++++++++++++++++++-------------------------- |
| 1 file changed, 58 insertions(+), 80 deletions(-) |
| |
| --- a/drivers/vfio/pci/vfio_pci.c |
| +++ b/drivers/vfio/pci/vfio_pci.c |
| @@ -876,15 +876,11 @@ static void vfio_pci_remove(struct pci_d |
| { |
| struct vfio_pci_device *vdev; |
| |
| - mutex_lock(&driver_lock); |
| - |
| vdev = vfio_del_group_dev(&pdev->dev); |
| if (vdev) { |
| iommu_group_put(pdev->dev.iommu_group); |
| kfree(vdev); |
| } |
| - |
| - mutex_unlock(&driver_lock); |
| } |
| |
| static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev, |
| @@ -927,108 +923,90 @@ static struct pci_driver vfio_pci_driver |
| .err_handler = &vfio_err_handlers, |
| }; |
| |
| -/* |
| - * Test whether a reset is necessary and possible. We mark devices as |
| - * needs_reset when they are released, but don't have a function-local reset |
| - * available. If any of these exist in the affected devices, we want to do |
| - * a bus/slot reset. We also need all of the affected devices to be unused, |
| - * so we abort if any device has a non-zero refcnt. driver_lock prevents a |
| - * device from being opened during the scan or unbound from vfio-pci. |
| - */ |
| -static int vfio_pci_test_bus_reset(struct pci_dev *pdev, void *data) |
| -{ |
| - bool *needs_reset = data; |
| - struct pci_driver *pci_drv = ACCESS_ONCE(pdev->driver); |
| - int ret = -EBUSY; |
| - |
| - if (pci_drv == &vfio_pci_driver) { |
| - struct vfio_device *device; |
| - struct vfio_pci_device *vdev; |
| - |
| - device = vfio_device_get_from_dev(&pdev->dev); |
| - if (!device) |
| - return ret; |
| - |
| - vdev = vfio_device_data(device); |
| - if (vdev) { |
| - if (vdev->needs_reset) |
| - *needs_reset = true; |
| - |
| - if (!vdev->refcnt) |
| - ret = 0; |
| - } |
| - |
| - vfio_device_put(device); |
| - } |
| - |
| - /* |
| - * TODO: vfio-core considers groups to be viable even if some devices |
| - * are attached to known drivers, like pci-stub or pcieport. We can't |
| - * freeze devices from being unbound to those drivers like we can |
| - * here though, so it would be racy to test for them. We also can't |
| - * use device_lock() to prevent changes as that would interfere with |
| - * PCI-core taking device_lock during bus reset. For now, we require |
| - * devices to be bound to vfio-pci to get a bus/slot reset on release. |
| - */ |
| - |
| - return ret; |
| -} |
| +struct vfio_devices { |
| + struct vfio_device **devices; |
| + int cur_index; |
| + int max_index; |
| +}; |
| |
| -/* Clear needs_reset on all affected devices after successful bus/slot reset */ |
| -static int vfio_pci_clear_needs_reset(struct pci_dev *pdev, void *data) |
| +static int vfio_pci_get_devs(struct pci_dev *pdev, void *data) |
| { |
| + struct vfio_devices *devs = data; |
| struct pci_driver *pci_drv = ACCESS_ONCE(pdev->driver); |
| |
| - if (pci_drv == &vfio_pci_driver) { |
| - struct vfio_device *device; |
| - struct vfio_pci_device *vdev; |
| - |
| - device = vfio_device_get_from_dev(&pdev->dev); |
| - if (!device) |
| - return 0; |
| - |
| - vdev = vfio_device_data(device); |
| - if (vdev) |
| - vdev->needs_reset = false; |
| + if (pci_drv != &vfio_pci_driver) |
| + return -EBUSY; |
| |
| - vfio_device_put(device); |
| - } |
| + if (devs->cur_index == devs->max_index) |
| + return -ENOSPC; |
| |
| + devs->devices[devs->cur_index] = vfio_device_get_from_dev(&pdev->dev); |
| + if (!devs->devices[devs->cur_index]) |
| + return -EINVAL; |
| + |
| + devs->cur_index++; |
| return 0; |
| } |
| |
| /* |
| * Attempt to do a bus/slot reset if there are devices affected by a reset for |
| * this device that are needs_reset and all of the affected devices are unused |
| - * (!refcnt). Callers of this function are required to hold driver_lock such |
| - * that devices can not be unbound from vfio-pci or opened by a user while we |
| - * test for and perform a bus/slot reset. |
| + * (!refcnt). Callers are required to hold driver_lock when calling this to |
| + * prevent device opens and concurrent bus reset attempts. We prevent device |
| + * unbinds by acquiring and holding a reference to the vfio_device. |
| + * |
| + * NB: vfio-core considers a group to be viable even if some devices are |
| + * bound to drivers like pci-stub or pcieport. Here we require all devices |
| + * to be bound to vfio_pci since that's the only way we can be sure they |
| + * stay put. |
| */ |
| static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) |
| { |
| + struct vfio_devices devs = { .cur_index = 0 }; |
| + int i = 0, ret = -EINVAL; |
| bool needs_reset = false, slot = false; |
| - int ret; |
| + struct vfio_pci_device *tmp; |
| |
| if (!pci_probe_reset_slot(vdev->pdev->slot)) |
| slot = true; |
| else if (pci_probe_reset_bus(vdev->pdev->bus)) |
| return; |
| |
| - if (vfio_pci_for_each_slot_or_bus(vdev->pdev, |
| - vfio_pci_test_bus_reset, |
| - &needs_reset, slot) || !needs_reset) |
| + if (vfio_pci_for_each_slot_or_bus(vdev->pdev, vfio_pci_count_devs, |
| + &i, slot) || !i) |
| return; |
| |
| - if (slot) |
| - ret = pci_try_reset_slot(vdev->pdev->slot); |
| - else |
| - ret = pci_try_reset_bus(vdev->pdev->bus); |
| - |
| - if (ret) |
| + devs.max_index = i; |
| + devs.devices = kcalloc(i, sizeof(struct vfio_device *), GFP_KERNEL); |
| + if (!devs.devices) |
| return; |
| |
| - vfio_pci_for_each_slot_or_bus(vdev->pdev, |
| - vfio_pci_clear_needs_reset, NULL, slot); |
| + if (vfio_pci_for_each_slot_or_bus(vdev->pdev, |
| + vfio_pci_get_devs, &devs, slot)) |
| + goto put_devs; |
| + |
| + for (i = 0; i < devs.cur_index; i++) { |
| + tmp = vfio_device_data(devs.devices[i]); |
| + if (tmp->needs_reset) |
| + needs_reset = true; |
| + if (tmp->refcnt) |
| + goto put_devs; |
| + } |
| + |
| + if (needs_reset) |
| + ret = slot ? pci_try_reset_slot(vdev->pdev->slot) : |
| + pci_try_reset_bus(vdev->pdev->bus); |
| + |
| +put_devs: |
| + for (i = 0; i < devs.cur_index; i++) { |
| + if (!ret) { |
| + tmp = vfio_device_data(devs.devices[i]); |
| + tmp->needs_reset = false; |
| + } |
| + vfio_device_put(devs.devices[i]); |
| + } |
| + |
| + kfree(devs.devices); |
| } |
| |
| static void __exit vfio_pci_cleanup(void) |