vfio: fixup notifiers and avoid possible deadlock
Subsystems (2)
cover vfio: fixup notifiers and avoid possible deadlock
Cleanup a couple of the notifier paths to remove bogus WARN_ON calls. One is pretty easy to hit and neither really signifies a problems. Fix remove path to avoid potential deadlock with other device_lock holders. Thanks,
Alex
---
Alex Williamson (3):
vfio: Don't overreact to DEL_DEVICE
vfio: Ignore sprurious notifies
vfio-pci: Avoid deadlock on remove
drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- drivers/vfio/vfio.c | 37 ++++++++++--------------------------- 2 files changed, 31 insertions(+), 29 deletions(-)
PATCH 1/3 vfio: Don't overreact to DEL_DEVICE
BUS_NOTIFY_DEL_DEVICE triggers IOMMU drivers to remove devices from their iommu group, but there's really nothing we can do about it at this point. If the device is in use, then the vfio sub-driver will block the device_del from completing until it's released. If the device is not in use or not owned by a vfio sub-driver, then we really don't care that it's being removed.
The current code can be triggered just by unloading an sr-iov driver (ex. igb) while the VFs are attached to vfio-pci because it makes an incorrect assumption about the ordering of driver remove callbacks vs the DEL_DEVICE notification.
--- drivers/vfio/vfio.c | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-)
drivers/vfio/vfio.c +7 -22
@@ -492,27 +492,6 @@ static int vfio_group_nb_add_dev(struct vfio_group *group, struct device *dev)return 0 ; } -static int vfio_group_nb_del_dev (struct vfio_group *group,struct device *dev) -{ -struct vfio_device *device; - -/* -* Expect to fall out here. If a device was in use, it would -* have been bound to a vfio sub-driver, which would have blocked -* in .remove at vfio_del_group_dev. Sanity check that we no -* longer track the device, so it's safe to remove. -*/ - device =vfio_group_get_device (group, dev); -if (likely (!device)) -return 0 ; - -WARN ("Device %s removed from live group %d!\n" ,dev_name (dev), -iommu_group_id (group->iommu_group)); - -vfio_device_put (device); -return 0 ; -} -static int vfio_group_nb_verify (struct vfio_group *group,struct device *dev) {/* We don't care what happens when the group isn't in use */
@@ -543,7 +522,13 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,vfio_group_nb_add_dev (group, dev);break ;case IOMMU_GROUP_NOTIFY_DEL_DEVICE : -vfio_group_nb_del_dev (group, dev); +/* +* Nothing to do here. If the device is in use, then the +* vfio sub-driver should block the remove callback until +* it is unused. If the device is unused or attached to a +* stub driver, then it should be released and we don't +* care that it will be going away. +*/ break ;case IOMMU_GROUP_NOTIFY_BIND_DRIVER : pr_debug("%s: Device %s, group %d binding to driver\n",
PATCH 2/3 vfio: Ignore sprurious notifies
In reply to Alex WilliamsonRemove debugging WARN_ON if we get a spurious notify for a group that no longer exists. No reports of anyone hitting this, but it would likely be a race and not a bug if they did.
--- drivers/vfio/vfio.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/vfio/vfio.c +3 -5
@@ -508,13 +508,11 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,struct device *dev = data;/* -* Need to go through a group_lock lookup to get a reference or -* we risk racing a group being removed. Leave a WARN_ON for -* debuging, but if the group no longer exists, a spurious notify -* is harmless. +* Need to go through a group_lock lookup to get a reference or we +* risk racing a group being removed. Ignore spurious notifies. */ group =vfio_group_try_get (group); -if (WARN_ON (!group)) +if (!group)return NOTIFY_OK ;switch (action) {
PATCH 3/3 vfio-pci: Avoid deadlock on remove
In reply to Alex WilliamsonIf an attempt is made to unbind a device from vfio-pci while that device is in use, the request is blocked until the device becomes unused. Unfortunately, that unbind path still grabs the device_lock, which certain things like __pci_reset_function() also want to take. This means we need to try to acquire the locks ourselves and use the pre-locked version, __pci_reset_function_locked().
--- drivers/vfio/pci/vfio_pci.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)
drivers/vfio/pci/vfio_pci.c +21 -2
@@ -137,8 +137,27 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)*/ pci_write_config_word (pdev,PCI_COMMAND ,PCI_COMMAND_INTX_DISABLE ); -if (vdev->reset_works) -__pci_reset_function (pdev); +/* +* Careful, device_lock may already be held. This is the case if +* a driver unbind is blocked. Try to get the locks ourselves to +* prevent a deadlock. +*/ +if (vdev->reset_works) { +bool reset_done = false; + +if (pci_cfg_access_trylock (pdev)) { +if (device_trylock (&pdev->dev)) { +__pci_reset_function_locked (pdev); + reset_done = true; +device_unlock (&pdev->dev); + } +pci_cfg_access_unlock (pdev); + } + +if (!reset_done) +pr_warn ("%s: Unable to acquire locks for reset of %s\n" , + __func__,dev_name (&pdev->dev)); + }pci_restore_state (pdev); }