Skip to content

Commit

Permalink
vfio: Delete unbound_list
Browse files Browse the repository at this point in the history
An element is added to the group->unbound_list only during
vfio_unregister_group_dev(). In turn this is only ever called from a
driver core device_driver remove() function.

The element stays on the unbound_list until a
IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER is delivered which removes the list
element.

However IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER is really just the driver core
bus callback BUS_NOTIFY_UNBOUND_DRIVER.

BUS_NOTIFY_UNBOUND_DRIVER is generated in one place by the driver core
under the device_lock():

static void __device_release_driver(struct device *dev, struct device *parent)
{
[..]
		else if (drv->remove)
			drv->remove(dev);

			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
						     BUS_NOTIFY_UNBOUND_DRIVER,
						     dev);

There is only one place that reads the unbound_list - vfio_dev_viable():

	device_lock_assert(dev);
	list_for_each_entry(unbound, &group->unbound_list, unbound_next) {
		if (dev == unbound->dev) {

In short, we obtain the device_lock(), add then immediately remove the
unbound item. The only reader is done under the device_lock() and can thus
never see the item we added.

Delete the whole thing.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
  • Loading branch information
jgunthorpe committed Apr 26, 2021
1 parent fa6abb3 commit 45980bd
Showing 1 changed file with 2 additions and 68 deletions.
70 changes: 2 additions & 68 deletions drivers/vfio/vfio.c
Expand Up @@ -62,11 +62,6 @@ struct vfio_container {
bool noiommu;
};

struct vfio_unbound_dev {
struct device *dev;
struct list_head unbound_next;
};

struct vfio_group {
struct kref kref;
int minor;
Expand All @@ -79,8 +74,6 @@ struct vfio_group {
struct notifier_block nb;
struct list_head vfio_next;
struct list_head container_next;
struct list_head unbound_list;
struct mutex unbound_lock;
atomic_t opened;
wait_queue_head_t container_q;
bool noiommu;
Expand Down Expand Up @@ -304,8 +297,6 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
kref_init(&group->kref);
INIT_LIST_HEAD(&group->device_list);
mutex_init(&group->device_lock);
INIT_LIST_HEAD(&group->unbound_list);
mutex_init(&group->unbound_lock);
atomic_set(&group->container_users, 0);
atomic_set(&group->opened, 0);
init_waitqueue_head(&group->container_q);
Expand Down Expand Up @@ -371,18 +362,11 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
static void vfio_group_release(struct kref *kref)
{
struct vfio_group *group = container_of(kref, struct vfio_group, kref);
struct vfio_unbound_dev *unbound, *tmp;
struct iommu_group *iommu_group = group->iommu_group;

WARN_ON(!list_empty(&group->device_list));
WARN_ON(group->notifier.head);

list_for_each_entry_safe(unbound, tmp,
&group->unbound_list, unbound_next) {
list_del(&unbound->unbound_next);
kfree(unbound);
}

device_destroy(vfio.class, MKDEV(MAJOR(vfio.group_devt), group->minor));
list_del(&group->vfio_next);
vfio_free_group_minor(group->minor);
Expand Down Expand Up @@ -573,29 +557,17 @@ static bool vfio_dev_driver_allowed(struct device *dev,
*
* We use two methods to determine whether a device is bound to a vfio
* driver. The first is to test whether the device exists in the vfio
* group. The second is to test if the device exists on the group
* unbound_list, indicating it's in the middle of transitioning from
* a vfio driver to driver-less.
* group.
*/
static int vfio_dev_viable(struct device *dev, struct vfio_group *group)
{
struct vfio_device *device;
struct device_driver *drv = dev->driver;
struct vfio_unbound_dev *unbound;
int ret = -EINVAL;

device_lock_assert(dev);

mutex_lock(&group->unbound_lock);
list_for_each_entry(unbound, &group->unbound_list, unbound_next) {
if (dev == unbound->dev) {
ret = 0;
break;
}
}
mutex_unlock(&group->unbound_lock);

if (!ret || !drv || vfio_dev_driver_allowed(dev, drv))
if (!drv || vfio_dev_driver_allowed(dev, drv))
return 0;

device = vfio_group_get_device(group, dev);
Expand Down Expand Up @@ -646,7 +618,6 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
{
struct vfio_group *group = container_of(nb, struct vfio_group, nb);
struct device *dev = data;
struct vfio_unbound_dev *unbound;

/*
* Need to go through a group_lock lookup to get a reference or we
Expand Down Expand Up @@ -686,24 +657,6 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
case IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER:
dev_dbg(dev, "%s: group %d unbound from driver\n", __func__,
iommu_group_id(group->iommu_group));
/*
* XXX An unbound device in a live group is ok, but we'd
* really like to avoid the above BUG_ON by preventing other
* drivers from binding to it. Once that occurs, we have to
* stop the system to maintain isolation. At a minimum, we'd
* want a toggle to disable driver auto probe for this device.
*/

mutex_lock(&group->unbound_lock);
list_for_each_entry(unbound,
&group->unbound_list, unbound_next) {
if (dev == unbound->dev) {
list_del(&unbound->unbound_next);
kfree(unbound);
break;
}
}
mutex_unlock(&group->unbound_lock);
break;
}

Expand Down Expand Up @@ -837,29 +790,10 @@ static struct vfio_device *vfio_device_get_from_name(struct vfio_group *group,
void vfio_unregister_group_dev(struct vfio_device *device)
{
struct vfio_group *group = device->group;
struct vfio_unbound_dev *unbound;
unsigned int i = 0;
bool interrupted = false;
long rc;

/*
* When the device is removed from the group, the group suddenly
* becomes non-viable; the device has a driver (until the unbind
* completes), but it's not present in the group. This is bad news
* for any external users that need to re-acquire a group reference
* in order to match and release their existing reference. To
* solve this, we track such devices on the unbound_list to bridge
* the gap until they're fully unbound.
*/
unbound = kzalloc(sizeof(*unbound), GFP_KERNEL);
if (unbound) {
unbound->dev = device->dev;
mutex_lock(&group->unbound_lock);
list_add(&unbound->unbound_next, &group->unbound_list);
mutex_unlock(&group->unbound_lock);
}
WARN_ON(!unbound);

vfio_device_put(device);
rc = try_wait_for_completion(&device->comp);
while (rc <= 0) {
Expand Down

0 comments on commit 45980bd

Please sign in to comment.