Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 5/19/22 2:33 PM, Matthew Rosato wrote:
Rather than relying on a notifier for associating the KVM with
the group, let's assume that the association has already been
made prior to device_open.  The first time a device is opened
associate the group KVM with the device.

This fixes a user-triggerable oops in GVT.

Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/gvt/gtt.c        |  4 +-
  drivers/gpu/drm/i915/gvt/gvt.h        |  3 -
  drivers/gpu/drm/i915/gvt/kvmgt.c      | 82 ++++++--------------------
  drivers/s390/crypto/vfio_ap_ops.c     | 35 ++---------
  drivers/s390/crypto/vfio_ap_private.h |  3 -
  drivers/vfio/vfio.c                   | 83 ++++++++++-----------------
  include/linux/vfio.h                  |  6 +-
  7 files changed, 57 insertions(+), 159 deletions(-)


diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e8914024f5b1..a7d2a95796d3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1284,25 +1284,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
  	}
  }
-static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
-				       unsigned long action, void *data)
-{
-	int notify_rc = NOTIFY_OK;
-	struct ap_matrix_mdev *matrix_mdev;
-
-	if (action != VFIO_GROUP_NOTIFY_SET_KVM)
-		return NOTIFY_OK;
-
-	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
-
-	if (!data)
-		vfio_ap_mdev_unset_kvm(matrix_mdev);
-	else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
-		notify_rc = NOTIFY_DONE;
-
-	return notify_rc;
-}
-
  static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
  {
  	struct device *dev;
@@ -1402,11 +1383,10 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
  	unsigned long events;
  	int ret;
- matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
-	events = VFIO_GROUP_NOTIFY_SET_KVM;
+	if (!vdev->kvm)
+		return -EINVAL;
- ret = vfio_register_notifier(vdev, VFIO_GROUP_NOTIFY, &events,
-				     &matrix_mdev->group_notifier);
+	ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
  	if (ret)
  		return ret;

I'm sorry I didn't see this with my last review, but maybe move the call
to vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm) after the successful
registration of the IOMMU notifier? This way you won't be plugging AP queues
into the guest only to remove them if the registration fails.

@@ -1415,12 +1395,11 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
  	ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY, &events,
  				     &matrix_mdev->iommu_notifier);
  	if (ret)
-		goto out_unregister_group;
+		goto err_kvm;
  	return 0;
-out_unregister_group:
-	vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
-				 &matrix_mdev->group_notifier);
+err_kvm:
+	vfio_ap_mdev_unset_kvm(matrix_mdev);
  	return ret;
  }
@@ -1431,8 +1410,6 @@ static void vfio_ap_mdev_close_device(struct vfio_device *vdev) vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY,
  				 &matrix_mdev->iommu_notifier);
-	vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
-				 &matrix_mdev->group_notifier);
  	vfio_ap_mdev_unset_kvm(matrix_mdev);
  }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 648fcaf8104a..a26efd804d0d 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -81,8 +81,6 @@ struct ap_matrix {
   * @node:	allows the ap_matrix_mdev struct to be added to a list
   * @matrix:	the adapters, usage domains and control domains assigned to the
   *		mediated matrix device.
- * @group_notifier: notifier block used for specifying callback function for
- *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
   * @iommu_notifier: notifier block used for specifying callback function for
   *		    handling the VFIO_IOMMU_NOTIFY_DMA_UNMAP even
   * @kvm:	the struct holding guest's state
@@ -94,7 +92,6 @@ struct ap_matrix_mdev {
  	struct vfio_device vdev;
  	struct list_head node;
  	struct ap_matrix matrix;
-	struct notifier_block group_notifier;
  	struct notifier_block iommu_notifier;
  	struct kvm *kvm;
  	crypto_hook pqap_hook;
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index cfcff7764403..831fc722e3f8 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1083,10 +1083,21 @@ static struct file *vfio_device_open(struct vfio_device *device)
mutex_lock(&device->dev_set->lock);
  	device->open_count++;
-	if (device->open_count == 1 && device->ops->open_device) {
-		ret = device->ops->open_device(device);
-		if (ret)
-			goto err_undo_count;
+	if (device->open_count == 1) {
+		/*
+		 * Here we pass the KVM pointer with the group under the read
+		 * lock.  If the device driver will use it, it must obtain a
+		 * reference and release it during close_device.
+		 */
+		down_read(&device->group->group_rwsem);
+		device->kvm = device->group->kvm;
+
+		if (device->ops->open_device) {
+			ret = device->ops->open_device(device);
+			if (ret)
+				goto err_undo_count;
+		}
+		up_read(&device->group->group_rwsem);
  	}
  	mutex_unlock(&device->dev_set->lock);
@@ -1119,10 +1130,14 @@ static struct file *vfio_device_open(struct vfio_device *device) err_close_device:
  	mutex_lock(&device->dev_set->lock);
+	down_read(&device->group->group_rwsem);
  	if (device->open_count == 1 && device->ops->close_device)
  		device->ops->close_device(device);
  err_undo_count:
  	device->open_count--;
+	if (device->open_count == 0 && device->kvm)
+		device->kvm = NULL;
+	up_read(&device->group->group_rwsem);
  	mutex_unlock(&device->dev_set->lock);
  	module_put(device->dev->driver->owner);
  err_unassign_container:
@@ -1315,9 +1330,13 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
mutex_lock(&device->dev_set->lock);
  	vfio_assert_device_open(device);
+	down_read(&device->group->group_rwsem);
  	if (device->open_count == 1 && device->ops->close_device)
  		device->ops->close_device(device);
+	up_read(&device->group->group_rwsem);
  	device->open_count--;
+	if (device->open_count == 0)
+		device->kvm = NULL;
  	mutex_unlock(&device->dev_set->lock);
module_put(device->dev->driver->owner);
@@ -1726,8 +1745,8 @@ EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
   * @file: VFIO group file
   * @kvm: KVM to link
   *
- * The kvm pointer will be forwarded to all the vfio_device's attached to the
- * VFIO file via the VFIO_GROUP_NOTIFY_SET_KVM notifier.
+ * When a VFIO device is first opened the KVM will be available in
+ * device->kvm if one was associated with the group.
   */
  void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
  {
@@ -1738,8 +1757,6 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
down_write(&group->group_rwsem);
  	group->kvm = kvm;
-	blocking_notifier_call_chain(&group->notifier,
-				     VFIO_GROUP_NOTIFY_SET_KVM, kvm);
  	up_write(&group->group_rwsem);
  }
  EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
@@ -2006,7 +2023,8 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
  	struct vfio_iommu_driver *driver;
  	int ret;
- down_read(&group->group_rwsem);
+	lockdep_assert_held_read(&group->group_rwsem);
+
  	container = group->container;
  	driver = container->iommu_driver;
  	if (likely(driver && driver->ops->register_notifier))
@@ -2014,7 +2032,6 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
  						     events, nb);
  	else
  		ret = -ENOTTY;
-	up_read(&group->group_rwsem);
return ret;
  }
@@ -2026,7 +2043,8 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
  	struct vfio_iommu_driver *driver;
  	int ret;
- down_read(&group->group_rwsem);
+	lockdep_assert_held_read(&group->group_rwsem);
+
  	container = group->container;
  	driver = container->iommu_driver;
  	if (likely(driver && driver->ops->unregister_notifier))
@@ -2034,47 +2052,10 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
  						       nb);
  	else
  		ret = -ENOTTY;
-	up_read(&group->group_rwsem);
return ret;
  }
-static int vfio_register_group_notifier(struct vfio_group *group,
-					unsigned long *events,
-					struct notifier_block *nb)
-{
-	int ret;
-	bool set_kvm = false;
-
-	if (*events & VFIO_GROUP_NOTIFY_SET_KVM)
-		set_kvm = true;
-
-	/* clear known events */
-	*events &= ~VFIO_GROUP_NOTIFY_SET_KVM;
-
-	/* refuse to continue if still events remaining */
-	if (*events)
-		return -EINVAL;
-
-	ret = blocking_notifier_chain_register(&group->notifier, nb);
-	if (ret)
-		return ret;
-
-	/*
-	 * The attaching of kvm and vfio_group might already happen, so
-	 * here we replay once upon registration.
-	 */
-	if (set_kvm) {
-		down_read(&group->group_rwsem);
-		if (group->kvm)
-			blocking_notifier_call_chain(&group->notifier,
-						     VFIO_GROUP_NOTIFY_SET_KVM,
-						     group->kvm);
-		up_read(&group->group_rwsem);
-	}
-	return 0;
-}
-
  int vfio_register_notifier(struct vfio_device *device,
  			   enum vfio_notify_type type, unsigned long *events,
  			   struct notifier_block *nb)
@@ -2090,9 +2071,6 @@ int vfio_register_notifier(struct vfio_device *device,
  	case VFIO_IOMMU_NOTIFY:
  		ret = vfio_register_iommu_notifier(group, events, nb);
  		break;
-	case VFIO_GROUP_NOTIFY:
-		ret = vfio_register_group_notifier(group, events, nb);
-		break;
  	default:
  		ret = -EINVAL;
  	}
@@ -2114,9 +2092,6 @@ int vfio_unregister_notifier(struct vfio_device *device,
  	case VFIO_IOMMU_NOTIFY:
  		ret = vfio_unregister_iommu_notifier(group, nb);
  		break;
-	case VFIO_GROUP_NOTIFY:
-		ret = blocking_notifier_chain_unregister(&group->notifier, nb);
-		break;
  	default:
  		ret = -EINVAL;
  	}
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 45b287826ce6..aa888cc51757 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -36,6 +36,8 @@ struct vfio_device {
  	struct vfio_device_set *dev_set;
  	struct list_head dev_set_list;
  	unsigned int migration_flags;
+	/* Driver must reference the kvm during open_device or never touch it */
+	struct kvm *kvm;
/* Members below here are private, not for driver use */
  	refcount_t refcount;
@@ -155,15 +157,11 @@ extern int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova,
  /* each type has independent events */
  enum vfio_notify_type {
  	VFIO_IOMMU_NOTIFY = 0,
-	VFIO_GROUP_NOTIFY = 1,
  };
/* events for VFIO_IOMMU_NOTIFY */
  #define VFIO_IOMMU_NOTIFY_DMA_UNMAP	BIT(0)
-/* events for VFIO_GROUP_NOTIFY */
-#define VFIO_GROUP_NOTIFY_SET_KVM	BIT(0)
-
  extern int vfio_register_notifier(struct vfio_device *device,
  				  enum vfio_notify_type type,
  				  unsigned long *required_events,




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux