[RFC PATCH 1/6] driver core: add a bus notification to temporarily reject driver binding

[RFC PATCH 1/6] driver core: add a bus notification to temporarily reject driver binding

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

There are several requirements to temporarily reject device driver
binding. Possible usage cases as below:
1) We should avoid binding an unsafe driver to a device belonging to
   an active VFIO group, otherwise it will break the DMA isolation
   property of VFIO.
2) When hot-removing a PCI hierachy, we should avoid binding device
   drivers to PCI devices going to be removed during the window
   between unbinding of device driver and destroying of device nodes.
3) When hot-adding a PCI host bridge, we should temporarily disable
   driver binding before setting up corresponding IOMMU and IOAPIC.

We may add a flag into struct device to temporarily disable driver
binding as in this thread https://patchwork.kernel.org/patch/1535721/.

This patch proposes another solution to temporarily disable driver
binding by using bus notification mechanisms. It adds an notification
event to solicit if anybody has objections when binding a driver to a
device.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/base/dd.c      |   11 ++++++++++-
 include/linux/device.h |    8 +++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e3bbed8..43c8034 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -205,9 +205,18 @@ static int driver_sysfs_add(struct device *dev)
 {
 	int ret;
 
-	if (dev->bus)
+	if (dev->bus) {
+		/* Does anybody have objections to the driver binding? */
+		ret = blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
+				   BUS_NOTIFY_SOLICIT_BINDING, dev);
+		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK) {
+			dev_dbg(dev, "driver binding has been rejected by bus notification handler.\n");
+			return notifier_to_errno(ret);
+		}
+
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_BIND_DRIVER, dev);
+	}
 
 	ret = sysfs_create_link(&dev->driver->p->kobj, &dev->kobj,
 			  kobject_name(&dev->kobj));
diff --git a/include/linux/device.h b/include/linux/device.h
index 52a5f15..2cfe8f9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -165,9 +165,13 @@ extern int bus_register_notifier(struct bus_type *bus,
 extern int bus_unregister_notifier(struct bus_type *bus,
 				   struct notifier_block *nb);
 
-/* All 4 notifers below get called with the target struct device *
+/* All 7 notifers below get called with the target struct device *
  * as an argument. Note that those functions are likely to be called
  * with the device lock held in the core, so be careful.
+ *
+ * Note: return values of notification handler are ignored by the driver core
+ * except for BUS_SOLICIT_BINDING, so notication handler shouldn't return
+ * error code except for BUS_SOLICIT_BINDING.
  */
 #define BUS_NOTIFY_ADD_DEVICE		0x00000001 /* device added */
 #define BUS_NOTIFY_DEL_DEVICE		0x00000002 /* device removed */
@@ -178,6 +182,8 @@ extern int bus_unregister_notifier(struct bus_type *bus,
 						      unbound */
 #define BUS_NOTIFY_UNBOUND_DRIVER	0x00000006 /* driver is unbound
 						      from the device */
+#define BUS_NOTIFY_SOLICIT_BINDING	0x00000007 /* Any objections to the
+						      driver binding? */
 
 extern struct kset *bus_get_kset(struct bus_type *bus);
 extern struct klist *bus_get_device_klist(struct bus_type *bus);
-- 
1.7.9.5

[RFC PATCH 2/6] iommu: pass on BUS_NOTIFY_QUERY_BINDING to iommu group notifier clients

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Pass on BUS_NOTIFY_QUERY_BINDING event to iommu group notifier clients,
so notifier clients have a chance to reject device driver binding.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/iommu/iommu.c |   14 +++++++++-----
 include/linux/iommu.h |    1 +
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ddbdaca..7fbe055 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -497,6 +497,7 @@ static int add_iommu_group(struct device *dev, void *data)
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data)
 {
+	int ret = NOTIFY_DONE;
 	struct device *dev = data;
 	struct iommu_ops *ops = dev->bus->iommu_ops;
 	struct iommu_group *group;
@@ -512,7 +513,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	} else if (action == BUS_NOTIFY_DEL_DEVICE) {
 		if (ops->remove_device && dev->iommu_group) {
 			ops->remove_device(dev);
-			return 0;
+			return NOTIFY_DONE;
 		}
 	}
 
@@ -522,9 +523,12 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	 */
 	group = iommu_group_get(dev);
 	if (!group)
-		return 0;
+		return NOTIFY_DONE;
 
 	switch (action) {
+	case BUS_NOTIFY_SOLICIT_BINDING:
+		group_action = IOMMU_GROUP_NOTIFY_SOLICIT_BINDING;
+		break;
 	case BUS_NOTIFY_BIND_DRIVER:
 		group_action = IOMMU_GROUP_NOTIFY_BIND_DRIVER;
 		break;
@@ -540,11 +544,11 @@ static int iommu_bus_notifier(struct notifier_block *nb,
 	}
 
 	if (group_action)
-		blocking_notifier_call_chain(&group->notifier,
-					     group_action, dev);
+		ret = blocking_notifier_call_chain(&group->notifier,
+						   group_action, dev);
 
 	iommu_group_put(group);
-	return 0;
+	return ret;
 }
 
 static struct notifier_block iommu_bus_nb = {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 7e83370..366435b 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -109,6 +109,7 @@ struct iommu_ops {
 #define IOMMU_GROUP_NOTIFY_BOUND_DRIVER		4 /* Post Driver bind */
 #define IOMMU_GROUP_NOTIFY_UNBIND_DRIVER	5 /* Pre Driver unbind */
 #define IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER	6 /* Post Driver unbind */
+#define IOMMU_GROUP_NOTIFY_SOLICIT_BINDING	7 /* Check for objections */
 
 extern int bus_set_iommu(struct bus_type *bus, struct iommu_ops *ops);
 extern bool iommu_present(struct bus_type *bus);
-- 
1.7.9.5

[RFC PATCH 3/6] VFIO: unregister IOMMU notifier on error recovery path

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

On error recovery path in function vfio_create_group(), it should
unregister the IOMMU notifier for the new VFIO group. Otherwise it may
cause invalid memory access later when handling bus notifications.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/vfio/vfio.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 17830c9..3359ec2 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -191,6 +191,17 @@ static void vfio_container_put(struct vfio_container *container)
 	kref_put(&container->kref, vfio_container_release);
 }
 
+static void vfio_group_unlock_and_free(struct vfio_group *group)
+{
+	mutex_unlock(&vfio.group_lock);
+	/*
+	 * Unregister outside of lock.  A spurious callback is harmless now
+	 * that the group is no longer in vfio.group_list.
+	 */
+	iommu_group_unregister_notifier(group->iommu_group, &group->nb);
+	kfree(group);
+}
+
 /**
  * Group objects - create, release, get, put, search
  */
@@ -229,8 +240,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
 
 	minor = vfio_alloc_group_minor(group);
 	if (minor < 0) {
-		mutex_unlock(&vfio.group_lock);
-		kfree(group);
+		vfio_group_unlock_and_free(group);
 		return ERR_PTR(minor);
 	}
 
@@ -239,8 +249,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
 		if (tmp->iommu_group == iommu_group) {
 			vfio_group_get(tmp);
 			vfio_free_group_minor(minor);
-			mutex_unlock(&vfio.group_lock);
-			kfree(group);
+			vfio_group_unlock_and_free(group);
 			return tmp;
 		}
 	}
@@ -249,8 +258,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
 			    group, "%d", iommu_group_id(iommu_group));
 	if (IS_ERR(dev)) {
 		vfio_free_group_minor(minor);
-		mutex_unlock(&vfio.group_lock);
-		kfree(group);
+		vfio_group_unlock_and_free(group);
 		return (struct vfio_group *)dev; /* ERR_PTR */
 	}
 
@@ -274,16 +282,7 @@ static void vfio_group_release(struct kref *kref)
 	device_destroy(vfio.class, MKDEV(MAJOR(vfio.devt), group->minor));
 	list_del(&group->vfio_next);
 	vfio_free_group_minor(group->minor);
-
-	mutex_unlock(&vfio.group_lock);
-
-	/*
-	 * Unregister outside of lock.  A spurious callback is harmless now
-	 * that the group is no longer in vfio.group_list.
-	 */
-	iommu_group_unregister_notifier(group->iommu_group, &group->nb);
-
-	kfree(group);
+	vfio_group_unlock_and_free(group);
 }
 
 static void vfio_group_put(struct vfio_group *group)
-- 
1.7.9.5

[RFC PATCH 4/6] VFIO: reject binding driver to devices belonging to active VFIO groups

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

VFIO driver should reject binding unsafe drivers to devices belonging to
active VFIO groups, otherwise it will break the DMA isolation property
of VFIO groups. So hook IOMMU_GROUP_NOTIFY_QUERY_BINDING event to reject
unsafe device driver binding for active VFIO groups.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/vfio/pci/vfio_pci.c |   11 ++++-
 drivers/vfio/vfio.c         |  106 ++++++++++++++++++++++++++++++++++++-------
 include/linux/vfio.h        |    3 ++
 3 files changed, 102 insertions(+), 18 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 6968b72..e380fc1 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -538,6 +538,7 @@ static struct pci_driver vfio_pci_driver = {
 static void __exit vfio_pci_cleanup(void)
 {
 	pci_unregister_driver(&vfio_pci_driver);
+	vfio_unregister_device_driver(&vfio_pci_driver.driver);
 	vfio_pci_virqfd_exit();
 	vfio_pci_uninit_perm_bits();
 }
@@ -556,6 +557,10 @@ static int __init vfio_pci_init(void)
 	if (ret)
 		goto out_virqfd;
 
+	ret = vfio_register_device_driver(&vfio_pci_driver.driver);
+	if (ret)
+		goto out_vfio_drv;
+
 	/* Register and scan for devices */
 	ret = pci_register_driver(&vfio_pci_driver);
 	if (ret)
@@ -563,9 +568,11 @@ static int __init vfio_pci_init(void)
 
 	return 0;
 
-out_virqfd:
-	vfio_pci_virqfd_exit();
 out_driver:
+	vfio_unregister_device_driver(&vfio_pci_driver.driver);
+out_vfio_drv:
+	vfio_pci_virqfd_exit();
+out_virqfd:
 	vfio_pci_uninit_perm_bits();
 	return ret;
 }
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 3359ec2..02da980 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -39,6 +39,8 @@ static struct vfio {
 	struct class			*class;
 	struct list_head		iommu_drivers_list;
 	struct mutex			iommu_drivers_lock;
+	struct list_head		device_drivers_list;
+	struct mutex			device_drivers_lock;
 	struct list_head		group_list;
 	struct idr			group_idr;
 	struct mutex			group_lock;
@@ -54,6 +56,11 @@ struct vfio_iommu_driver {
 	struct list_head			vfio_next;
 };
 
+struct vfio_device_driver {
+	const struct device_driver		*drv;
+	struct list_head			vfio_next;
+};
+
 struct vfio_container {
 	struct kref			kref;
 	struct list_head		group_list;
@@ -135,6 +142,55 @@ void vfio_unregister_iommu_driver(const struct vfio_iommu_driver_ops *ops)
 EXPORT_SYMBOL_GPL(vfio_unregister_iommu_driver);
 
 /**
+ * VFIO device driver registration
+ */
+int vfio_register_device_driver(const struct device_driver *drv)
+{
+	struct vfio_device_driver *driver, *tmp;
+
+	driver = kzalloc(sizeof(*driver), GFP_KERNEL);
+	if (!driver)
+		return -ENOMEM;
+
+	driver->drv = drv;
+
+	mutex_lock(&vfio.device_drivers_lock);
+
+	/* Check for duplicates */
+	list_for_each_entry(tmp, &vfio.device_drivers_list, vfio_next) {
+		if (tmp->drv == drv) {
+			mutex_unlock(&vfio.device_drivers_lock);
+			kfree(driver);
+			return -EINVAL;
+		}
+	}
+
+	list_add(&driver->vfio_next, &vfio.device_drivers_list);
+
+	mutex_unlock(&vfio.device_drivers_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vfio_register_device_driver);
+
+void vfio_unregister_device_driver(const struct device_driver *drv)
+{
+	struct vfio_device_driver *driver;
+
+	mutex_lock(&vfio.device_drivers_lock);
+	list_for_each_entry(driver, &vfio.device_drivers_list, vfio_next) {
+		if (driver->drv == drv) {
+			list_del(&driver->vfio_next);
+			mutex_unlock(&vfio.device_drivers_lock);
+			kfree(driver);
+			return;
+		}
+	}
+	mutex_unlock(&vfio.device_drivers_lock);
+}
+EXPORT_SYMBOL_GPL(vfio_unregister_device_driver);
+
+/**
  * Group minor allocation/free - both called with vfio.group_lock held
  */
 static int vfio_alloc_group_minor(struct vfio_group *group)
@@ -463,17 +519,18 @@ static bool vfio_whitelisted_driver(struct device_driver *drv)
  */
 static int vfio_dev_viable(struct device *dev, void *data)
 {
-	struct vfio_group *group = data;
-	struct vfio_device *device;
+	struct vfio_device_driver *driver;
 
 	if (!dev->driver || vfio_whitelisted_driver(dev->driver))
 		return 0;
 
-	device = vfio_group_get_device(group, dev);
-	if (device) {
-		vfio_device_put(device);
-		return 0;
-	}
+	mutex_lock(&vfio.device_drivers_lock);
+	list_for_each_entry(driver, &vfio.device_drivers_list, vfio_next)
+		if (driver->drv == dev->driver) {
+			mutex_unlock(&vfio.device_drivers_lock);
+			return 0;
+		}
+	mutex_unlock(&vfio.device_drivers_lock);
 
 	return -EINVAL;
 }
@@ -496,7 +553,6 @@ static int vfio_group_nb_add_dev(struct vfio_group *group, struct device *dev)
 	if (!atomic_read(&group->container_users))
 		return 0;
 
-	/* TODO Prevent device auto probing */
 	WARN("Device %s added to live group %d!\n", dev_name(dev),
 	     iommu_group_id(group->iommu_group));
 
@@ -533,9 +589,28 @@ static int vfio_group_nb_verify(struct vfio_group *group, struct device *dev)
 	return vfio_dev_viable(dev, group);
 }
 
+static int vfio_group_nb_solicit_binding(struct vfio_group *group,
+					 struct device *dev)
+{
+	int ret;
+
+	/* Allow driver binding for idle groups */
+	if (!atomic_read(&group->container_users))
+		return 0;
+
+	ret = vfio_dev_viable(dev, group);
+	if (ret)
+		dev_info(dev,
+			 "VFIO rejects binding device in active group to unsafe driver %s!\n",
+			 dev_driver_string(dev));
+
+	return ret;
+}
+
 static int vfio_iommu_group_notifier(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
+	int ret = NOTIFY_DONE;
 	struct vfio_group *group = container_of(nb, struct vfio_group, nb);
 	struct device *dev = data;
 
@@ -556,6 +631,10 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
 	case IOMMU_GROUP_NOTIFY_DEL_DEVICE:
 		vfio_group_nb_del_dev(group, dev);
 		break;
+	case IOMMU_GROUP_NOTIFY_SOLICIT_BINDING:
+		if (vfio_group_nb_solicit_binding(group, dev))
+			ret = notifier_from_errno(-EBUSY);
+		break;
 	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
 		pr_debug("%s: Device %s, group %d binding to driver\n",
 			 __func__, dev_name(dev),
@@ -576,18 +655,11 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
 		pr_debug("%s: Device %s, group %d unbound from driver\n",
 			 __func__, dev_name(dev),
 			 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.
-		 */
 		break;
 	}
 
 	vfio_group_put(group);
-	return NOTIFY_OK;
+	return ret;
 }
 
 /**
@@ -1334,8 +1406,10 @@ static int __init vfio_init(void)
 	idr_init(&vfio.group_idr);
 	mutex_init(&vfio.group_lock);
 	mutex_init(&vfio.iommu_drivers_lock);
+	mutex_init(&vfio.device_drivers_lock);
 	INIT_LIST_HEAD(&vfio.group_list);
 	INIT_LIST_HEAD(&vfio.iommu_drivers_list);
+	INIT_LIST_HEAD(&vfio.device_drivers_list);
 	init_waitqueue_head(&vfio.release_q);
 
 	vfio.class = class_create(THIS_MODULE, "vfio");
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 0a4f180..4faa9b9 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -78,6 +78,9 @@ extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
 extern void vfio_unregister_iommu_driver(
 				const struct vfio_iommu_driver_ops *ops);
 
+extern int vfio_register_device_driver(const struct device_driver *drv);
+extern void vfio_unregister_device_driver(const struct device_driver *drv);
+
 /**
  * offsetofend(TYPE, MEMBER)
  *
-- 
1.7.9.5

[RFC PATCH 5/6] VFIO: simplify IOMMU group notification handler

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Now we have a way to reject binding unsafe drivers to devices belonging
to active VFIO groups, so we could simplify IOMMU group notification
handler to only handle IOMMU_GROUP_NOTIFY_SOLICIT_BINDING event.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/vfio/vfio.c |   90 ++++-----------------------------------------------
 1 file changed, 6 insertions(+), 84 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 02da980..18714b9 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -538,57 +538,6 @@ static int vfio_dev_viable(struct device *dev, void *data)
 /**
  * Async device support
  */
-static int vfio_group_nb_add_dev(struct vfio_group *group, struct device *dev)
-{
-	struct vfio_device *device;
-
-	/* Do we already know about it?  We shouldn't */
-	device = vfio_group_get_device(group, dev);
-	if (WARN_ON_ONCE(device)) {
-		vfio_device_put(device);
-		return 0;
-	}
-
-	/* Nothing to do for idle groups */
-	if (!atomic_read(&group->container_users))
-		return 0;
-
-	WARN("Device %s added to live group %d!\n", dev_name(dev),
-	     iommu_group_id(group->iommu_group));
-
-	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 */
-	if (!atomic_read(&group->container_users))
-		return 0;
-
-	return vfio_dev_viable(dev, group);
-}
-
 static int vfio_group_nb_solicit_binding(struct vfio_group *group,
 					 struct device *dev)
 {
@@ -614,6 +563,9 @@ 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;
 
+	if (action != IOMMU_GROUP_NOTIFY_SOLICIT_BINDING)
+		return NOTIFY_DONE;
+
 	/*
 	 * 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
@@ -624,41 +576,11 @@ static int vfio_iommu_group_notifier(struct notifier_block *nb,
 	if (WARN_ON(!group))
 		return NOTIFY_OK;
 
-	switch (action) {
-	case IOMMU_GROUP_NOTIFY_ADD_DEVICE:
-		vfio_group_nb_add_dev(group, dev);
-		break;
-	case IOMMU_GROUP_NOTIFY_DEL_DEVICE:
-		vfio_group_nb_del_dev(group, dev);
-		break;
-	case IOMMU_GROUP_NOTIFY_SOLICIT_BINDING:
-		if (vfio_group_nb_solicit_binding(group, dev))
-			ret = notifier_from_errno(-EBUSY);
-		break;
-	case IOMMU_GROUP_NOTIFY_BIND_DRIVER:
-		pr_debug("%s: Device %s, group %d binding to driver\n",
-			 __func__, dev_name(dev),
-			 iommu_group_id(group->iommu_group));
-		break;
-	case IOMMU_GROUP_NOTIFY_BOUND_DRIVER:
-		pr_debug("%s: Device %s, group %d bound to driver %s\n",
-			 __func__, dev_name(dev),
-			 iommu_group_id(group->iommu_group), dev->driver->name);
-		BUG_ON(vfio_group_nb_verify(group, dev));
-		break;
-	case IOMMU_GROUP_NOTIFY_UNBIND_DRIVER:
-		pr_debug("%s: Device %s, group %d unbinding from driver %s\n",
-			 __func__, dev_name(dev),
-			 iommu_group_id(group->iommu_group), dev->driver->name);
-		break;
-	case IOMMU_GROUP_NOTIFY_UNBOUND_DRIVER:
-		pr_debug("%s: Device %s, group %d unbound from driver\n",
-			 __func__, dev_name(dev),
-			 iommu_group_id(group->iommu_group));
-		break;
-	}
+	if (vfio_group_nb_solicit_binding(group, dev))
+		ret = notifier_from_errno(-EBUSY);
 
 	vfio_group_put(group);
+
 	return ret;
 }
 
-- 
1.7.9.5

[RFC PATCH 6/6] VFIO: use ACCESS_ONCE() to guard access to dev->driver

[Jiang Liu]

 From: Jiang Liu <jiang.liu@huawei.com>

Comments from dev_driver_string(),
/* dev->driver can change to NULL underneath us because of unbinding,
 * so be careful about accessing it.
 */

So use ACCESS_ONCE() to guard access to dev->driver field.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/vfio/vfio.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 18714b9..f158e42 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -520,8 +520,9 @@ static bool vfio_whitelisted_driver(struct device_driver *drv)
 static int vfio_dev_viable(struct device *dev, void *data)
 {
 	struct vfio_device_driver *driver;
+	struct device_driver *drv = ACCESS_ONCE(dev->driver);
 
-	if (!dev->driver || vfio_whitelisted_driver(dev->driver))
+	if (!drv || vfio_whitelisted_driver(drv))
 		return 0;
 
 	mutex_lock(&vfio.device_drivers_lock);
-- 
1.7.9.5

Re: [RFC PATCH 1/6] driver core: add a bus notification to temporarily reject driver binding

[Greg Kroah-Hartman]

On Sat, Nov 10, 2012 at 09:57:14PM +0800, Jiang Liu wrote:
>  From: Jiang Liu <jiang.liu@huawei.com>
> 
> There are several requirements to temporarily reject device driver
> binding. Possible usage cases as below:
> 1) We should avoid binding an unsafe driver to a device belonging to
>    an active VFIO group, otherwise it will break the DMA isolation
>    property of VFIO.
> 2) When hot-removing a PCI hierachy, we should avoid binding device
>    drivers to PCI devices going to be removed during the window
>    between unbinding of device driver and destroying of device nodes.
> 3) When hot-adding a PCI host bridge, we should temporarily disable
>    driver binding before setting up corresponding IOMMU and IOAPIC.
> 
> We may add a flag into struct device to temporarily disable driver
> binding as in this thread https://patchwork.kernel.org/patch/1535721/.

I totally do not understand.  The bus controls this, if it does not want
to bind a device to a driver, then don't do it.  It's really quite
simple to just block the probe callback the bus gets, right?  Why create
all of this extra, and confusing, interface instead?

> This patch proposes another solution to temporarily disable driver
> binding by using bus notification mechanisms. It adds an notification
> event to solicit if anybody has objections when binding a driver to a
> device.

Sorry, but no, don't do this, it's way more confusing.

greg k-h

Re: [RFC PATCH 1/6] driver core: add a bus notification to temporarily reject driver binding

[Jiang Liu]

On 11/11/2012 01:21 PM, Greg Kroah-Hartman wrote:
> On Sat, Nov 10, 2012 at 09:57:14PM +0800, Jiang Liu wrote:
>>  From: Jiang Liu <jiang.liu@huawei.com>
>>
>> There are several requirements to temporarily reject device driver
>> binding. Possible usage cases as below:
>> 1) We should avoid binding an unsafe driver to a device belonging to
>>    an active VFIO group, otherwise it will break the DMA isolation
>>    property of VFIO.
>> 2) When hot-removing a PCI hierachy, we should avoid binding device
>>    drivers to PCI devices going to be removed during the window
>>    between unbinding of device driver and destroying of device nodes.
>> 3) When hot-adding a PCI host bridge, we should temporarily disable
>>    driver binding before setting up corresponding IOMMU and IOAPIC.
>>
>> We may add a flag into struct device to temporarily disable driver
>> binding as in this thread https://patchwork.kernel.org/patch/1535721/.
> 
> I totally do not understand.  The bus controls this, if it does not want
> to bind a device to a driver, then don't do it.  It's really quite
> simple to just block the probe callback the bus gets, right?  Why create
> all of this extra, and confusing, interface instead?
Hi Greg,
	Thanks for your comments. 
	As you know, we already have an "drivers_autoprobe" flag for drivers,
we are trying to provide a similar mechanism for devices.
	But I'm not sure whether we could block the probe callback. For PCI
host bridge hotplug, that will effectively block the PCI host bridge hotplug
thread. For VFIO case, its goal is to reject binding unsafe drivers to PCI
devices belonging to active VFIO group, so it doesn't make sense to block
the driver probing thread too. So we are trying to return error code instead
of blocking in really_probe().
	Thanks!
	Gerry

> 
>> This patch proposes another solution to temporarily disable driver
>> binding by using bus notification mechanisms. It adds an notification
>> event to solicit if anybody has objections when binding a driver to a
>> device.
> 
> Sorry, but no, don't do this, it's way more confusing.
> 
> greg k-h
> 

Re: [RFC PATCH 3/6] VFIO: unregister IOMMU notifier on error recovery path

[Alex Williamson]

On Sat, 2012-11-10 at 21:57 +0800, Jiang Liu wrote:
>  From: Jiang Liu <jiang.liu@huawei.com>
> 
> On error recovery path in function vfio_create_group(), it should
> unregister the IOMMU notifier for the new VFIO group. Otherwise it may
> cause invalid memory access later when handling bus notifications.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/vfio/vfio.c |   31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)

This patch and patch 6/6 looks like good vfio fixes regardless of how we
tackle the driver binding problem.  Please submit them separately.
Thanks for the patches, I look forward to a solution here.  Thanks,

Alex

> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 17830c9..3359ec2 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -191,6 +191,17 @@ static void vfio_container_put(struct vfio_container *container)
>  	kref_put(&container->kref, vfio_container_release);
>  }
>  
> +static void vfio_group_unlock_and_free(struct vfio_group *group)
> +{
> +	mutex_unlock(&vfio.group_lock);
> +	/*
> +	 * Unregister outside of lock.  A spurious callback is harmless now
> +	 * that the group is no longer in vfio.group_list.
> +	 */
> +	iommu_group_unregister_notifier(group->iommu_group, &group->nb);
> +	kfree(group);
> +}
> +
>  /**
>   * Group objects - create, release, get, put, search
>   */
> @@ -229,8 +240,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
>  
>  	minor = vfio_alloc_group_minor(group);
>  	if (minor < 0) {
> -		mutex_unlock(&vfio.group_lock);
> -		kfree(group);
> +		vfio_group_unlock_and_free(group);
>  		return ERR_PTR(minor);
>  	}
>  
> @@ -239,8 +249,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
>  		if (tmp->iommu_group == iommu_group) {
>  			vfio_group_get(tmp);
>  			vfio_free_group_minor(minor);
> -			mutex_unlock(&vfio.group_lock);
> -			kfree(group);
> +			vfio_group_unlock_and_free(group);
>  			return tmp;
>  		}
>  	}
> @@ -249,8 +258,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
>  			    group, "%d", iommu_group_id(iommu_group));
>  	if (IS_ERR(dev)) {
>  		vfio_free_group_minor(minor);
> -		mutex_unlock(&vfio.group_lock);
> -		kfree(group);
> +		vfio_group_unlock_and_free(group);
>  		return (struct vfio_group *)dev; /* ERR_PTR */
>  	}
>  
> @@ -274,16 +282,7 @@ static void vfio_group_release(struct kref *kref)
>  	device_destroy(vfio.class, MKDEV(MAJOR(vfio.devt), group->minor));
>  	list_del(&group->vfio_next);
>  	vfio_free_group_minor(group->minor);
> -
> -	mutex_unlock(&vfio.group_lock);
> -
> -	/*
> -	 * Unregister outside of lock.  A spurious callback is harmless now
> -	 * that the group is no longer in vfio.group_list.
> -	 */
> -	iommu_group_unregister_notifier(group->iommu_group, &group->nb);
> -
> -	kfree(group);
> +	vfio_group_unlock_and_free(group);
>  }
>  
>  static void vfio_group_put(struct vfio_group *group)