[PATCH 0/6] Fully lock the container members of struct vfio_group

[PATCH 0/6] Fully lock the container members of struct vfio_group

[Jason Gunthorpe]

The atomic based scheme for tracking the group->container and group->kvm
has two race conditions, simplify it by adding a rwsem to protect those
values and related and remove the atomics.

It is based on top of all the series thus far:
 - rc3
 - vfio_mdev_no_group
 - vfio_kvm_no_group
 - vfio_get_from_dev
 - iommu series

To make all this work removing alot of the container_users cases was
necessary, which was accomplished in prior series.

This is on github: https://github.com/jgunthorpe/linux/commits/vfio_group_locking

(which also has all 82 patches listed above)

This has been rebased far too many times, I've checked this version and so
has Nicolin, but it could use a careful 3rd look.

Jason Gunthorpe (6):
  vfio: Add missing locking for struct vfio_group::kvm
  vfio: Change struct vfio_group::opened from an atomic to bool
  vfio: Split up vfio_group_get_device_fd()
  vfio: Fully lock struct vfio_group::container
  vfio: Simplify the life cycle of the group FD
  vfio: Change struct vfio_group::container_users to a non-atomic int

 drivers/vfio/vfio.c | 273 +++++++++++++++++++++++++++-----------------
 1 file changed, 166 insertions(+), 107 deletions(-)


base-commit: e50abb9a762947bd0b9ffc73b7e28e5523f6eab8
-- 
2.36.0

[PATCH 1/6] vfio: Add missing locking for struct vfio_group::kvm

[Jason Gunthorpe]

Without locking userspace can trigger a UAF by racing
KVM_DEV_VFIO_GROUP_DEL with VFIO_GROUP_GET_DEVICE_FD:

              CPU1                               CPU2
					    ioctl(KVM_DEV_VFIO_GROUP_DEL)
 ioctl(VFIO_GROUP_GET_DEVICE_FD)
    vfio_group_get_device_fd
     open_device()
      intel_vgpu_open_device()
        vfio_register_notifier()
	 vfio_register_group_notifier()
	   blocking_notifier_call_chain(&group->notifier,
               VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);

					      set_kvm()
						group->kvm = NULL
					    close()
					     kfree(kvm)

             intel_vgpu_group_notifier()
                vdev->kvm = data
    [..]
        kvmgt_guest_init()
         kvm_get_kvm(info->kvm);
	    // UAF!

Add a simple rwsem in the group to protect the kvm while the notifier is
using it.

Note this doesn't fix the race internal to i915 where userspace can
trigger two VFIO_GROUP_NOTIFY_SET_KVM's before we reach kvmgt_guest_init()
and trigger this same UAF.

Fixes: ccd46dbae77d ("vfio: support notifier chain in vfio_group")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index e6ea3981bc7c4a..0477df3a50a3d6 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -77,6 +77,7 @@ struct vfio_group {
 	wait_queue_head_t		container_q;
 	enum vfio_group_type		type;
 	unsigned int			dev_counter;
+	struct rw_semaphore		group_rwsem;
 	struct kvm			*kvm;
 	struct blocking_notifier_head	notifier;
 };
@@ -361,6 +362,7 @@ static struct vfio_group *vfio_group_alloc(struct iommu_group *iommu_group,
 	group->cdev.owner = THIS_MODULE;
 
 	refcount_set(&group->users, 1);
+	init_rwsem(&group->group_rwsem);
 	INIT_LIST_HEAD(&group->device_list);
 	mutex_init(&group->device_lock);
 	init_waitqueue_head(&group->container_q);
@@ -1714,9 +1716,11 @@ void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
 	if (file->f_op != &vfio_group_fops)
 		return;
 
+	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);
 
@@ -2024,15 +2028,22 @@ static int vfio_register_group_notifier(struct vfio_group *group,
 		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 (!ret && set_kvm && group->kvm)
-		blocking_notifier_call_chain(&group->notifier,
-					VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
-	return ret;
+	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,
-- 
2.36.0

[PATCH 2/6] vfio: Change struct vfio_group::opened from an atomic to bool

[Jason Gunthorpe]

This is not a performance path, just use the group_rwsem to protect the
value.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 46 ++++++++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0477df3a50a3d6..a5584131648765 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -73,7 +73,7 @@ struct vfio_group {
 	struct mutex			device_lock;
 	struct list_head		vfio_next;
 	struct list_head		container_next;
-	atomic_t			opened;
+	bool				opened;
 	wait_queue_head_t		container_q;
 	enum vfio_group_type		type;
 	unsigned int			dev_counter;
@@ -1233,30 +1233,30 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 {
 	struct vfio_group *group =
 		container_of(inode->i_cdev, struct vfio_group, cdev);
-	int opened;
+	int ret;
+
+	down_write(&group->group_rwsem);
 
 	/* users can be zero if this races with vfio_group_put() */
-	if (!refcount_inc_not_zero(&group->users))
-		return -ENODEV;
+	if (!refcount_inc_not_zero(&group->users)) {
+		ret = -ENODEV;
+		goto err_unlock;
+	}
 
 	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO)) {
-		vfio_group_put(group);
-		return -EPERM;
+		ret = -EPERM;
+		goto err_put;
 	}
 
-	/* Do we need multiple instances of the group open?  Seems not. */
-	opened = atomic_cmpxchg(&group->opened, 0, 1);
-	if (opened) {
-		vfio_group_put(group);
-		return -EBUSY;
-	}
-
-	/* Is something still in use from a previous open? */
-	if (group->container) {
-		atomic_dec(&group->opened);
-		vfio_group_put(group);
-		return -EBUSY;
+	/*
+	 * Do we need multiple instances of the group open?  Seems not.
+	 * Is something still in use from a previous open?
+	 */
+	if (group->opened || group->container) {
+		ret = -EBUSY;
+		goto err_put;
 	}
+	group->opened = true;
 
 	/* Warn if previous user didn't cleanup and re-init to drop them */
 	if (WARN_ON(group->notifier.head))
@@ -1264,7 +1264,13 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 
 	filep->private_data = group;
 
+	up_write(&group->group_rwsem);
 	return 0;
+err_put:
+	vfio_group_put(group);
+err_unlock:
+	up_write(&group->group_rwsem);
+	return ret;
 }
 
 static int vfio_group_fops_release(struct inode *inode, struct file *filep)
@@ -1275,7 +1281,9 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 
 	vfio_group_try_dissolve_container(group);
 
-	atomic_dec(&group->opened);
+	down_write(&group->group_rwsem);
+	group->opened = false;
+	up_write(&group->group_rwsem);
 
 	vfio_group_put(group);
 
-- 
2.36.0

[PATCH 3/6] vfio: Split up vfio_group_get_device_fd()

[Jason Gunthorpe]

The split follows the pairing with the destroy functions:

 - vfio_group_get_device_fd() destroyed by close()

 - vfio_device_open() destroyed by vfio_device_fops_release()

 - vfio_device_assign_container() destroyed by
   vfio_group_try_dissolve_container()

The next patch will put a lock around vfio_device_assign_container().

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 89 +++++++++++++++++++++++++++++++--------------
 1 file changed, 62 insertions(+), 27 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a5584131648765..d8d14e528ab795 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1084,27 +1084,38 @@ static bool vfio_assert_device_open(struct vfio_device *device)
 	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
 }
 
-static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
+static int vfio_device_assign_container(struct vfio_device *device)
 {
-	struct vfio_device *device;
-	struct file *filep;
-	int fdno;
-	int ret = 0;
+	struct vfio_group *group = device->group;
 
 	if (0 == atomic_read(&group->container_users) ||
 	    !group->container->iommu_driver)
 		return -EINVAL;
 
-	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
-		return -EPERM;
+	if (group->type == VFIO_NO_IOMMU) {
+		if (!capable(CAP_SYS_RAWIO))
+			return -EPERM;
+		dev_warn(device->dev,
+			 "vfio-noiommu device opened by user (%s:%d)\n",
+			 current->comm, task_pid_nr(current));
+	}
 
-	device = vfio_device_get_from_name(group, buf);
-	if (IS_ERR(device))
-		return PTR_ERR(device);
+	atomic_inc(&group->container_users);
+	return 0;
+}
+
+static struct file *vfio_device_open(struct vfio_device *device)
+{
+	struct file *filep;
+	int ret;
+
+	ret = vfio_device_assign_container(device);
+	if (ret)
+		return ERR_PTR(ret);
 
 	if (!try_module_get(device->dev->driver->owner)) {
 		ret = -ENODEV;
-		goto err_device_put;
+		goto err_unassign_container;
 	}
 
 	mutex_lock(&device->dev_set->lock);
@@ -1120,15 +1131,11 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	 * We can't use anon_inode_getfd() because we need to modify
 	 * the f_mode flags directly to allow more than just ioctls
 	 */
-	fdno = ret = get_unused_fd_flags(O_CLOEXEC);
-	if (ret < 0)
-		goto err_close_device;
-
 	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
 				   device, O_RDWR);
 	if (IS_ERR(filep)) {
 		ret = PTR_ERR(filep);
-		goto err_fd;
+		goto err_close_device;
 	}
 
 	/*
@@ -1138,17 +1145,12 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	 */
 	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
 
-	atomic_inc(&group->container_users);
+	/*
+	 * On success the ref of device is moved to the file and
+	 * put in vfio_device_fops_release()
+	 */
+	return filep;
 
-	fd_install(fdno, filep);
-
-	if (group->type == VFIO_NO_IOMMU)
-		dev_warn(device->dev, "vfio-noiommu device opened by user "
-			 "(%s:%d)\n", current->comm, task_pid_nr(current));
-	return fdno;
-
-err_fd:
-	put_unused_fd(fdno);
 err_close_device:
 	mutex_lock(&device->dev_set->lock);
 	if (device->open_count == 1 && device->ops->close_device)
@@ -1157,7 +1159,40 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
 	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
 	module_put(device->dev->driver->owner);
-err_device_put:
+err_unassign_container:
+	vfio_group_try_dissolve_container(device->group);
+	return ERR_PTR(ret);
+}
+
+static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
+{
+	struct vfio_device *device;
+	struct file *filep;
+	int fdno;
+	int ret;
+
+	device = vfio_device_get_from_name(group, buf);
+	if (IS_ERR(device))
+		return PTR_ERR(device);
+
+	fdno = get_unused_fd_flags(O_CLOEXEC);
+	if (fdno < 0) {
+		ret = fdno;
+		goto err_put_device;
+	}
+
+	filep = vfio_device_open(device);
+	if (IS_ERR(filep)) {
+		ret = PTR_ERR(filep);
+		goto err_put_fdno;
+	}
+
+	fd_install(fdno, filep);
+	return fdno;
+
+err_put_fdno:
+	put_unused_fd(fdno);
+err_put_device:
 	vfio_device_put(device);
 	return ret;
 }
-- 
2.36.0

[PATCH 4/6] vfio: Fully lock struct vfio_group::container

[Jason Gunthorpe]

This is necessary to avoid various user triggerable races, for instance
racing SET_CONTAINER/UNSET_CONTAINER:

                                  ioctl(VFIO_GROUP_SET_CONTAINER)
ioctl(VFIO_GROUP_UNSET_CONTAINER)
 vfio_group_unset_container
    int users = atomic_cmpxchg(&group->container_users, 1, 0);
    // users == 1 container_users == 0
    __vfio_group_unset_container(group);

                                    vfio_group_set_container()
	                              if (atomic_read(&group->container_users))
				        down_write(&container->group_lock);
				        group->container = container;
				        up_write(&container->group_lock);

      down_write(&container->group_lock);
      group->container = NULL;
      up_write(&container->group_lock);
      vfio_container_put(container);
      /* woops we leaked the original container  */

This can then go on to NULL pointer deref since container == 0 and
container_users == 1.

Wrap all touches of container, except those on a performance path with a
known open device, with the group_rwsem.

The only user of vfio_group_add_container_user() holds the user count for
a simple operation, change it to just hold the group_lock over the
operation and delete vfio_group_add_container_user(). Containers now only
gain a user when a device FD is opened.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 66 +++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index d8d14e528ab795..63f7fa872eae60 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -937,6 +937,8 @@ static void __vfio_group_unset_container(struct vfio_group *group)
 	struct vfio_container *container = group->container;
 	struct vfio_iommu_driver *driver;
 
+	lockdep_assert_held_write(&group->group_rwsem);
+
 	down_write(&container->group_lock);
 
 	driver = container->iommu_driver;
@@ -973,6 +975,8 @@ static int vfio_group_unset_container(struct vfio_group *group)
 {
 	int users = atomic_cmpxchg(&group->container_users, 1, 0);
 
+	lockdep_assert_held_write(&group->group_rwsem);
+
 	if (!users)
 		return -EINVAL;
 	if (users != 1)
@@ -991,8 +995,10 @@ static int vfio_group_unset_container(struct vfio_group *group)
  */
 static void vfio_group_try_dissolve_container(struct vfio_group *group)
 {
+	down_write(&group->group_rwsem);
 	if (0 == atomic_dec_if_positive(&group->container_users))
 		__vfio_group_unset_container(group);
+	up_write(&group->group_rwsem);
 }
 
 static int vfio_group_set_container(struct vfio_group *group, int container_fd)
@@ -1002,6 +1008,8 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 	struct vfio_iommu_driver *driver;
 	int ret = 0;
 
+	lockdep_assert_held_write(&group->group_rwsem);
+
 	if (atomic_read(&group->container_users))
 		return -EINVAL;
 
@@ -1059,23 +1067,6 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 	return ret;
 }
 
-static int vfio_group_add_container_user(struct vfio_group *group)
-{
-	if (!atomic_inc_not_zero(&group->container_users))
-		return -EINVAL;
-
-	if (group->type == VFIO_NO_IOMMU) {
-		atomic_dec(&group->container_users);
-		return -EPERM;
-	}
-	if (!group->container->iommu_driver) {
-		atomic_dec(&group->container_users);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static const struct file_operations vfio_device_fops;
 
 /* true if the vfio_device has open_device() called but not close_device() */
@@ -1088,6 +1079,8 @@ static int vfio_device_assign_container(struct vfio_device *device)
 {
 	struct vfio_group *group = device->group;
 
+	lockdep_assert_held_write(&group->group_rwsem);
+
 	if (0 == atomic_read(&group->container_users) ||
 	    !group->container->iommu_driver)
 		return -EINVAL;
@@ -1109,7 +1102,9 @@ static struct file *vfio_device_open(struct vfio_device *device)
 	struct file *filep;
 	int ret;
 
+	down_write(&device->group->group_rwsem);
 	ret = vfio_device_assign_container(device);
+	up_write(&device->group->group_rwsem);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -1219,11 +1214,13 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
 
 		status.flags = 0;
 
+		down_read(&group->group_rwsem);
 		if (group->container)
 			status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET |
 					VFIO_GROUP_FLAGS_VIABLE;
 		else if (!iommu_group_dma_owner_claimed(group->iommu_group))
 			status.flags |= VFIO_GROUP_FLAGS_VIABLE;
+		up_read(&group->group_rwsem);
 
 		if (copy_to_user((void __user *)arg, &status, minsz))
 			return -EFAULT;
@@ -1241,11 +1238,15 @@ static long vfio_group_fops_unl_ioctl(struct file *filep,
 		if (fd < 0)
 			return -EINVAL;
 
+		down_write(&group->group_rwsem);
 		ret = vfio_group_set_container(group, fd);
+		up_write(&group->group_rwsem);
 		break;
 	}
 	case VFIO_GROUP_UNSET_CONTAINER:
+		down_write(&group->group_rwsem);
 		ret = vfio_group_unset_container(group);
+		up_write(&group->group_rwsem);
 		break;
 	case VFIO_GROUP_GET_DEVICE_FD:
 	{
@@ -1731,15 +1732,19 @@ bool vfio_file_enforced_coherent(struct file *file)
 	if (file->f_op != &vfio_group_fops)
 		return true;
 
-	/*
-	 * Since the coherency state is determined only once a container is
-	 * attached the user must do so before they can prove they have
-	 * permission.
-	 */
-	if (vfio_group_add_container_user(group))
-		return true;
-	ret = vfio_ioctl_check_extension(group->container, VFIO_DMA_CC_IOMMU);
-	vfio_group_try_dissolve_container(group);
+	down_read(&group->group_rwsem);
+	if (group->container) {
+		ret = vfio_ioctl_check_extension(group->container,
+						 VFIO_DMA_CC_IOMMU);
+	} else {
+		/*
+		 * Since the coherency state is determined only once a container
+		 * is attached the user must do so before they can prove they
+		 * have permission.
+		 */
+		ret = true;
+	}
+	up_read(&group->group_rwsem);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
@@ -1932,6 +1937,7 @@ int vfio_pin_pages(struct vfio_device *device, unsigned long *user_pfn,
 	if (group->dev_counter > 1)
 		return -EINVAL;
 
+	/* group->container cannot change while a vfio device is open */
 	container = group->container;
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->pin_pages))
@@ -1967,6 +1973,7 @@ int vfio_unpin_pages(struct vfio_device *device, unsigned long *user_pfn,
 	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
 		return -E2BIG;
 
+	/* group->container cannot change while a vfio device is open */
 	container = device->group->container;
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->unpin_pages))
@@ -2006,6 +2013,7 @@ int vfio_dma_rw(struct vfio_device *device, dma_addr_t user_iova, void *data,
 	if (!data || len <= 0 || !vfio_assert_device_open(device))
 		return -EINVAL;
 
+	/* group->container cannot change while a vfio device is open */
 	container = device->group->container;
 	driver = container->iommu_driver;
 
@@ -2026,6 +2034,7 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
 	struct vfio_iommu_driver *driver;
 	int ret;
 
+	down_read(&group->group_rwsem);
 	container = group->container;
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->register_notifier))
@@ -2033,6 +2042,8 @@ static int vfio_register_iommu_notifier(struct vfio_group *group,
 						     events, nb);
 	else
 		ret = -ENOTTY;
+	up_read(&group->group_rwsem);
+
 	return ret;
 }
 
@@ -2043,6 +2054,7 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
 	struct vfio_iommu_driver *driver;
 	int ret;
 
+	down_read(&group->group_rwsem);
 	container = group->container;
 	driver = container->iommu_driver;
 	if (likely(driver && driver->ops->unregister_notifier))
@@ -2050,6 +2062,8 @@ static int vfio_unregister_iommu_notifier(struct vfio_group *group,
 						       nb);
 	else
 		ret = -ENOTTY;
+	up_read(&group->group_rwsem);
+
 	return ret;
 }
 
-- 
2.36.0

[PATCH 5/6] vfio: Simplify the life cycle of the group FD

[Jason Gunthorpe]

Once userspace opens a group FD it is prevented from opening another
instance of that same group FD until all the prior group FDs and users of
the container are done.

The first is done trivially by checking the group->owned during group FD
open.

However, things get a little weird if userspace creates a device FD and
then closes the group FD. The group FD still cannot be re-opened, but this
time it is because the group->container is still set and container_users
is elevated by the device FD.

Due to this mismatched lifecycle we have the
vfio_group_try_dissolve_container() which tries to auto-free a container
after the group FD is closed but the device FD remains open.

Instead have the device FD hold onto a reference to the single group
FD. This directly prevents vfio_group_fops_release() from being called
when any device FD exists and makes the lifecycle model more
understandable.

vfio_group_try_dissolve_container() is removed as the only place a
container is auto-deleted is during vfio_group_fops_release(). At this
point the container_users is either 1 or 0 since all device FDs must be
closed.

Change group->owner to group->singleton_filep which points to the single
struct file * that is open for the group. If the group->singleton_filep is
NULL then group->container == NULL.

If all device FDs have closed then the group's notifier list must be
empty.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 49 +++++++++++++++++++--------------------------
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 63f7fa872eae60..94ab415190011d 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -73,12 +73,12 @@ struct vfio_group {
 	struct mutex			device_lock;
 	struct list_head		vfio_next;
 	struct list_head		container_next;
-	bool				opened;
 	wait_queue_head_t		container_q;
 	enum vfio_group_type		type;
 	unsigned int			dev_counter;
 	struct rw_semaphore		group_rwsem;
 	struct kvm			*kvm;
+	struct file			*singleton_file;
 	struct blocking_notifier_head	notifier;
 };
 
@@ -987,20 +987,6 @@ static int vfio_group_unset_container(struct vfio_group *group)
 	return 0;
 }
 
-/*
- * When removing container users, anything that removes the last user
- * implicitly removes the group from the container.  That is, if the
- * group file descriptor is closed, as well as any device file descriptors,
- * the group is free.
- */
-static void vfio_group_try_dissolve_container(struct vfio_group *group)
-{
-	down_write(&group->group_rwsem);
-	if (0 == atomic_dec_if_positive(&group->container_users))
-		__vfio_group_unset_container(group);
-	up_write(&group->group_rwsem);
-}
-
 static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 {
 	struct fd f;
@@ -1093,10 +1079,19 @@ static int vfio_device_assign_container(struct vfio_device *device)
 			 current->comm, task_pid_nr(current));
 	}
 
+	get_file(group->singleton_file);
 	atomic_inc(&group->container_users);
 	return 0;
 }
 
+static void vfio_device_unassign_container(struct vfio_device *device)
+{
+	down_write(&device->group->group_rwsem);
+	atomic_dec(&device->group->container_users);
+	fput(device->group->singleton_file);
+	up_write(&device->group->group_rwsem);
+}
+
 static struct file *vfio_device_open(struct vfio_device *device)
 {
 	struct file *filep;
@@ -1155,7 +1150,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
 	mutex_unlock(&device->dev_set->lock);
 	module_put(device->dev->driver->owner);
 err_unassign_container:
-	vfio_group_try_dissolve_container(device->group);
+	vfio_device_unassign_container(device);
 	return ERR_PTR(ret);
 }
 
@@ -1286,18 +1281,12 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
 
 	/*
 	 * Do we need multiple instances of the group open?  Seems not.
-	 * Is something still in use from a previous open?
 	 */
-	if (group->opened || group->container) {
+	if (group->singleton_file) {
 		ret = -EBUSY;
 		goto err_put;
 	}
-	group->opened = true;
-
-	/* Warn if previous user didn't cleanup and re-init to drop them */
-	if (WARN_ON(group->notifier.head))
-		BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
-
+	group->singleton_file = filep;
 	filep->private_data = group;
 
 	up_write(&group->group_rwsem);
@@ -1315,10 +1304,14 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 
 	filep->private_data = NULL;
 
-	vfio_group_try_dissolve_container(group);
-
 	down_write(&group->group_rwsem);
-	group->opened = false;
+	/* All device FDs must be released before the group fd releases. */
+	WARN_ON(group->notifier.head);
+	if (group->container) {
+		WARN_ON(atomic_read(&group->container_users) != 1);
+		__vfio_group_unset_container(group);
+	}
+	group->singleton_file = NULL;
 	up_write(&group->group_rwsem);
 
 	vfio_group_put(group);
@@ -1350,7 +1343,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	module_put(device->dev->driver->owner);
 
-	vfio_group_try_dissolve_container(device->group);
+	vfio_device_unassign_container(device);
 
 	vfio_device_put(device);
 
-- 
2.36.0

[PATCH 6/6] vfio: Change struct vfio_group::container_users to a non-atomic int

[Jason Gunthorpe]

Now that everything is fully locked there is no need for container_users
to remain as an atomic, change it to an unsigned int.

Use 'if (group->container)' as the test to determine if the container is
present or not instead of using container_users.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/vfio/vfio.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 94ab415190011d..5c9f56d05f9dfa 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -66,7 +66,7 @@ struct vfio_group {
 	struct device 			dev;
 	struct cdev			cdev;
 	refcount_t			users;
-	atomic_t			container_users;
+	unsigned int			container_users;
 	struct iommu_group		*iommu_group;
 	struct vfio_container		*container;
 	struct list_head		device_list;
@@ -431,7 +431,7 @@ static void vfio_group_put(struct vfio_group *group)
 	 * properly hold the group reference.
 	 */
 	WARN_ON(!list_empty(&group->device_list));
-	WARN_ON(atomic_read(&group->container_users));
+	WARN_ON(group->container || group->container_users);
 	WARN_ON(group->notifier.head);
 
 	list_del(&group->vfio_next);
@@ -949,6 +949,7 @@ static void __vfio_group_unset_container(struct vfio_group *group)
 	iommu_group_release_dma_owner(group->iommu_group);
 
 	group->container = NULL;
+	group->container_users = 0;
 	wake_up(&group->container_q);
 	list_del(&group->container_next);
 
@@ -973,17 +974,13 @@ static void __vfio_group_unset_container(struct vfio_group *group)
  */
 static int vfio_group_unset_container(struct vfio_group *group)
 {
-	int users = atomic_cmpxchg(&group->container_users, 1, 0);
-
 	lockdep_assert_held_write(&group->group_rwsem);
 
-	if (!users)
+	if (!group->container)
 		return -EINVAL;
-	if (users != 1)
+	if (group->container_users != 1)
 		return -EBUSY;
-
 	__vfio_group_unset_container(group);
-
 	return 0;
 }
 
@@ -996,7 +993,7 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 
 	lockdep_assert_held_write(&group->group_rwsem);
 
-	if (atomic_read(&group->container_users))
+	if (group->container || WARN_ON(group->container_users))
 		return -EINVAL;
 
 	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
@@ -1040,12 +1037,12 @@ static int vfio_group_set_container(struct vfio_group *group, int container_fd)
 	}
 
 	group->container = container;
+	group->container_users = 1;
 	container->noiommu = (group->type == VFIO_NO_IOMMU);
 	list_add(&group->container_next, &container->group_list);
 
 	/* Get a reference on the container and mark a user within the group */
 	vfio_container_get(container);
-	atomic_inc(&group->container_users);
 
 unlock_out:
 	up_write(&container->group_lock);
@@ -1067,8 +1064,8 @@ static int vfio_device_assign_container(struct vfio_device *device)
 
 	lockdep_assert_held_write(&group->group_rwsem);
 
-	if (0 == atomic_read(&group->container_users) ||
-	    !group->container->iommu_driver)
+	if (!group->container || !group->container->iommu_driver ||
+	    WARN_ON(!group->container_users))
 		return -EINVAL;
 
 	if (group->type == VFIO_NO_IOMMU) {
@@ -1080,14 +1077,15 @@ static int vfio_device_assign_container(struct vfio_device *device)
 	}
 
 	get_file(group->singleton_file);
-	atomic_inc(&group->container_users);
+	group->container_users++;
 	return 0;
 }
 
 static void vfio_device_unassign_container(struct vfio_device *device)
 {
 	down_write(&device->group->group_rwsem);
-	atomic_dec(&device->group->container_users);
+	WARN_ON(device->group->container_users <= 1);
+	device->group->container_users--;
 	fput(device->group->singleton_file);
 	up_write(&device->group->group_rwsem);
 }
@@ -1308,7 +1306,7 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
 	/* All device FDs must be released before the group fd releases. */
 	WARN_ON(group->notifier.head);
 	if (group->container) {
-		WARN_ON(atomic_read(&group->container_users) != 1);
+		WARN_ON(group->container_users != 1);
 		__vfio_group_unset_container(group);
 	}
 	group->singleton_file = NULL;
-- 
2.36.0

Re: [PATCH 3/6] vfio: Split up vfio_group_get_device_fd()

[Alex Williamson]

On Thu,  5 May 2022 21:25:03 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> The split follows the pairing with the destroy functions:
> 
>  - vfio_group_get_device_fd() destroyed by close()
> 
>  - vfio_device_open() destroyed by vfio_device_fops_release()
> 
>  - vfio_device_assign_container() destroyed by
>    vfio_group_try_dissolve_container()
> 
> The next patch will put a lock around vfio_device_assign_container().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 89 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 62 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index a5584131648765..d8d14e528ab795 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1084,27 +1084,38 @@ static bool vfio_assert_device_open(struct vfio_device *device)
>  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
>  }
>  
> -static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> +static int vfio_device_assign_container(struct vfio_device *device)
>  {
> -	struct vfio_device *device;
> -	struct file *filep;
> -	int fdno;
> -	int ret = 0;
> +	struct vfio_group *group = device->group;
>  
>  	if (0 == atomic_read(&group->container_users) ||
>  	    !group->container->iommu_driver)
>  		return -EINVAL;
>  
> -	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
> -		return -EPERM;
> +	if (group->type == VFIO_NO_IOMMU) {
> +		if (!capable(CAP_SYS_RAWIO))
> +			return -EPERM;
> +		dev_warn(device->dev,
> +			 "vfio-noiommu device opened by user (%s:%d)\n",
> +			 current->comm, task_pid_nr(current));

I don't see why this was moved.  It was previously ordered such that we
would not emit a warning unless the device is actually opened.  Now
there are various error cases that could make this a false warning.
Thanks,

Alex

> +	}
>  
> -	device = vfio_device_get_from_name(group, buf);
> -	if (IS_ERR(device))
> -		return PTR_ERR(device);
> +	atomic_inc(&group->container_users);
> +	return 0;
> +}
> +
> +static struct file *vfio_device_open(struct vfio_device *device)
> +{
> +	struct file *filep;
> +	int ret;
> +
> +	ret = vfio_device_assign_container(device);
> +	if (ret)
> +		return ERR_PTR(ret);
>  
>  	if (!try_module_get(device->dev->driver->owner)) {
>  		ret = -ENODEV;
> -		goto err_device_put;
> +		goto err_unassign_container;
>  	}
>  
>  	mutex_lock(&device->dev_set->lock);
> @@ -1120,15 +1131,11 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  	 * We can't use anon_inode_getfd() because we need to modify
>  	 * the f_mode flags directly to allow more than just ioctls
>  	 */
> -	fdno = ret = get_unused_fd_flags(O_CLOEXEC);
> -	if (ret < 0)
> -		goto err_close_device;
> -
>  	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
>  				   device, O_RDWR);
>  	if (IS_ERR(filep)) {
>  		ret = PTR_ERR(filep);
> -		goto err_fd;
> +		goto err_close_device;
>  	}
>  
>  	/*
> @@ -1138,17 +1145,12 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  	 */
>  	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
>  
> -	atomic_inc(&group->container_users);
> +	/*
> +	 * On success the ref of device is moved to the file and
> +	 * put in vfio_device_fops_release()
> +	 */
> +	return filep;
>  
> -	fd_install(fdno, filep);
> -
> -	if (group->type == VFIO_NO_IOMMU)
> -		dev_warn(device->dev, "vfio-noiommu device opened by user "
> -			 "(%s:%d)\n", current->comm, task_pid_nr(current));
> -	return fdno;
> -
> -err_fd:
> -	put_unused_fd(fdno);
>  err_close_device:
>  	mutex_lock(&device->dev_set->lock);
>  	if (device->open_count == 1 && device->ops->close_device)
> @@ -1157,7 +1159,40 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
>  	device->open_count--;
>  	mutex_unlock(&device->dev_set->lock);
>  	module_put(device->dev->driver->owner);
> -err_device_put:
> +err_unassign_container:
> +	vfio_group_try_dissolve_container(device->group);
> +	return ERR_PTR(ret);
> +}
> +
> +static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> +{
> +	struct vfio_device *device;
> +	struct file *filep;
> +	int fdno;
> +	int ret;
> +
> +	device = vfio_device_get_from_name(group, buf);
> +	if (IS_ERR(device))
> +		return PTR_ERR(device);
> +
> +	fdno = get_unused_fd_flags(O_CLOEXEC);
> +	if (fdno < 0) {
> +		ret = fdno;
> +		goto err_put_device;
> +	}
> +
> +	filep = vfio_device_open(device);
> +	if (IS_ERR(filep)) {
> +		ret = PTR_ERR(filep);
> +		goto err_put_fdno;
> +	}
> +
> +	fd_install(fdno, filep);
> +	return fdno;
> +
> +err_put_fdno:
> +	put_unused_fd(fdno);
> +err_put_device:
>  	vfio_device_put(device);
>  	return ret;
>  }

Re: [PATCH 5/6] vfio: Simplify the life cycle of the group FD

[Alex Williamson]

On Thu,  5 May 2022 21:25:05 -0300
Jason Gunthorpe <jgg@nvidia.com> wrote:

> Once userspace opens a group FD it is prevented from opening another
> instance of that same group FD until all the prior group FDs and users of
> the container are done.
> 
> The first is done trivially by checking the group->owned during group FD
> open.
> 
> However, things get a little weird if userspace creates a device FD and
> then closes the group FD. The group FD still cannot be re-opened, but this
> time it is because the group->container is still set and container_users
> is elevated by the device FD.
> 
> Due to this mismatched lifecycle we have the
> vfio_group_try_dissolve_container() which tries to auto-free a container
> after the group FD is closed but the device FD remains open.
> 
> Instead have the device FD hold onto a reference to the single group
> FD. This directly prevents vfio_group_fops_release() from being called
> when any device FD exists and makes the lifecycle model more
> understandable.
> 
> vfio_group_try_dissolve_container() is removed as the only place a
> container is auto-deleted is during vfio_group_fops_release(). At this
> point the container_users is either 1 or 0 since all device FDs must be
> closed.
> 
> Change group->owner to group->singleton_filep which points to the single
> struct file * that is open for the group. If the group->singleton_filep is
> NULL then group->container == NULL.
> 
> If all device FDs have closed then the group's notifier list must be
> empty.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/vfio/vfio.c | 49 +++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 63f7fa872eae60..94ab415190011d 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -73,12 +73,12 @@ struct vfio_group {
>  	struct mutex			device_lock;
>  	struct list_head		vfio_next;
>  	struct list_head		container_next;
> -	bool				opened;
>  	wait_queue_head_t		container_q;
>  	enum vfio_group_type		type;
>  	unsigned int			dev_counter;
>  	struct rw_semaphore		group_rwsem;
>  	struct kvm			*kvm;
> +	struct file			*singleton_file;

I'm not really a fan of this name, if we have a single struct file
pointer on the group, it's necessarily singleton.  Maybe just
"opened_file"?

>  	struct blocking_notifier_head	notifier;
>  };
>  
> @@ -987,20 +987,6 @@ static int vfio_group_unset_container(struct vfio_group *group)
>  	return 0;
>  }
>  
> -/*
> - * When removing container users, anything that removes the last user
> - * implicitly removes the group from the container.  That is, if the
> - * group file descriptor is closed, as well as any device file descriptors,
> - * the group is free.
> - */
> -static void vfio_group_try_dissolve_container(struct vfio_group *group)
> -{
> -	down_write(&group->group_rwsem);
> -	if (0 == atomic_dec_if_positive(&group->container_users))
> -		__vfio_group_unset_container(group);
> -	up_write(&group->group_rwsem);
> -}
> -
>  static int vfio_group_set_container(struct vfio_group *group, int container_fd)
>  {
>  	struct fd f;
> @@ -1093,10 +1079,19 @@ static int vfio_device_assign_container(struct vfio_device *device)
>  			 current->comm, task_pid_nr(current));
>  	}
>  
> +	get_file(group->singleton_file);
>  	atomic_inc(&group->container_users);
>  	return 0;
>  }
>  
> +static void vfio_device_unassign_container(struct vfio_device *device)
> +{
> +	down_write(&device->group->group_rwsem);
> +	atomic_dec(&device->group->container_users);
> +	fput(device->group->singleton_file);
> +	up_write(&device->group->group_rwsem);
> +}
> +
>  static struct file *vfio_device_open(struct vfio_device *device)
>  {
>  	struct file *filep;
> @@ -1155,7 +1150,7 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  	mutex_unlock(&device->dev_set->lock);
>  	module_put(device->dev->driver->owner);
>  err_unassign_container:
> -	vfio_group_try_dissolve_container(device->group);
> +	vfio_device_unassign_container(device);
>  	return ERR_PTR(ret);
>  }
>  
> @@ -1286,18 +1281,12 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep)
>  
>  	/*
>  	 * Do we need multiple instances of the group open?  Seems not.
> -	 * Is something still in use from a previous open?
>  	 */
> -	if (group->opened || group->container) {
> +	if (group->singleton_file) {
>  		ret = -EBUSY;
>  		goto err_put;
>  	}
> -	group->opened = true;
> -
> -	/* Warn if previous user didn't cleanup and re-init to drop them */
> -	if (WARN_ON(group->notifier.head))
> -		BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
> -
> +	group->singleton_file = filep;
>  	filep->private_data = group;
>  
>  	up_write(&group->group_rwsem);
> @@ -1315,10 +1304,14 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
>  
>  	filep->private_data = NULL;
>  
> -	vfio_group_try_dissolve_container(group);
> -
>  	down_write(&group->group_rwsem);
> -	group->opened = false;
> +	/* All device FDs must be released before the group fd releases. */

This sounds more like a user directive as it's phrased, maybe something
like:

	/*
	 * Device FDs hold a group file reference, therefore the group
	 * release is only called when there are no open devices.
	 */

Thanks,
Alex

> +	WARN_ON(group->notifier.head);
> +	if (group->container) {
> +		WARN_ON(atomic_read(&group->container_users) != 1);
> +		__vfio_group_unset_container(group);
> +	}
> +	group->singleton_file = NULL;
>  	up_write(&group->group_rwsem);
>  
>  	vfio_group_put(group);
> @@ -1350,7 +1343,7 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
>  
>  	module_put(device->dev->driver->owner);
>  
> -	vfio_group_try_dissolve_container(device->group);
> +	vfio_device_unassign_container(device);
>  
>  	vfio_device_put(device);
>  

Re: [PATCH 3/6] vfio: Split up vfio_group_get_device_fd()

[Jason Gunthorpe]

On Tue, May 10, 2022 at 01:59:56PM -0600, Alex Williamson wrote:
> On Thu,  5 May 2022 21:25:03 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > The split follows the pairing with the destroy functions:
> > 
> >  - vfio_group_get_device_fd() destroyed by close()
> > 
> >  - vfio_device_open() destroyed by vfio_device_fops_release()
> > 
> >  - vfio_device_assign_container() destroyed by
> >    vfio_group_try_dissolve_container()
> > 
> > The next patch will put a lock around vfio_device_assign_container().
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 89 +++++++++++++++++++++++++++++++--------------
> >  1 file changed, 62 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index a5584131648765..d8d14e528ab795 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -1084,27 +1084,38 @@ static bool vfio_assert_device_open(struct vfio_device *device)
> >  	return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
> >  }
> >  
> > -static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
> > +static int vfio_device_assign_container(struct vfio_device *device)
> >  {
> > -	struct vfio_device *device;
> > -	struct file *filep;
> > -	int fdno;
> > -	int ret = 0;
> > +	struct vfio_group *group = device->group;
> >  
> >  	if (0 == atomic_read(&group->container_users) ||
> >  	    !group->container->iommu_driver)
> >  		return -EINVAL;
> >  
> > -	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
> > -		return -EPERM;
> > +	if (group->type == VFIO_NO_IOMMU) {
> > +		if (!capable(CAP_SYS_RAWIO))
> > +			return -EPERM;
> > +		dev_warn(device->dev,
> > +			 "vfio-noiommu device opened by user (%s:%d)\n",
> > +			 current->comm, task_pid_nr(current));
> 
> I don't see why this was moved.  It was previously ordered such that we
> would not emit a warning unless the device is actually opened.  Now
> there are various error cases that could make this a false warning.

I have another patch that moves all the container code into another
file and then optionally doesn't compile it - this is one of the
functions that gets moved.

When container support is disabled things like group->type get ifdef'd
away too so leaving this behind creates some mess and breaks up the
modularity.

I don't think it is worth suppressing an unlikely false message to
break the modularity - at the end someone did try to open and use a
device that is dangerous - it is not completely false.

Jason

Re: [PATCH 5/6] vfio: Simplify the life cycle of the group FD

[Jason Gunthorpe]

On Tue, May 10, 2022 at 01:59:59PM -0600, Alex Williamson wrote:
> On Thu,  5 May 2022 21:25:05 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > Once userspace opens a group FD it is prevented from opening another
> > instance of that same group FD until all the prior group FDs and users of
> > the container are done.
> > 
> > The first is done trivially by checking the group->owned during group FD
> > open.
> > 
> > However, things get a little weird if userspace creates a device FD and
> > then closes the group FD. The group FD still cannot be re-opened, but this
> > time it is because the group->container is still set and container_users
> > is elevated by the device FD.
> > 
> > Due to this mismatched lifecycle we have the
> > vfio_group_try_dissolve_container() which tries to auto-free a container
> > after the group FD is closed but the device FD remains open.
> > 
> > Instead have the device FD hold onto a reference to the single group
> > FD. This directly prevents vfio_group_fops_release() from being called
> > when any device FD exists and makes the lifecycle model more
> > understandable.
> > 
> > vfio_group_try_dissolve_container() is removed as the only place a
> > container is auto-deleted is during vfio_group_fops_release(). At this
> > point the container_users is either 1 or 0 since all device FDs must be
> > closed.
> > 
> > Change group->owner to group->singleton_filep which points to the single
> > struct file * that is open for the group. If the group->singleton_filep is
> > NULL then group->container == NULL.
> > 
> > If all device FDs have closed then the group's notifier list must be
> > empty.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/vfio.c | 49 +++++++++++++++++++--------------------------
> >  1 file changed, 21 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > index 63f7fa872eae60..94ab415190011d 100644
> > +++ b/drivers/vfio/vfio.c
> > @@ -73,12 +73,12 @@ struct vfio_group {
> >  	struct mutex			device_lock;
> >  	struct list_head		vfio_next;
> >  	struct list_head		container_next;
> > -	bool				opened;
> >  	wait_queue_head_t		container_q;
> >  	enum vfio_group_type		type;
> >  	unsigned int			dev_counter;
> >  	struct rw_semaphore		group_rwsem;
> >  	struct kvm			*kvm;
> > +	struct file			*singleton_file;
> 
> I'm not really a fan of this name, if we have a single struct file
> pointer on the group, it's necessarily singleton.  Maybe just
> "opened_file"?

Sure

> > @@ -1315,10 +1304,14 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep)
> >  
> >  	filep->private_data = NULL;
> >  
> > -	vfio_group_try_dissolve_container(group);
> > -
> >  	down_write(&group->group_rwsem);
> > -	group->opened = false;
> > +	/* All device FDs must be released before the group fd releases. */
> 
> This sounds more like a user directive as it's phrased, maybe something
> like:
> 
> 	/*
> 	 * Device FDs hold a group file reference, therefore the group
> 	 * release is only called when there are no open devices.
> 	 */

OK

What do you want to do with this series?

As-posted it requires the iommu series, and Joerg has vanished again
so I don't know what will happen there.

However, it doesn't require it, there are just some textual conflicts.

It does need the KVM series though, which I expect we will just go
ahead with unacked. Oh well.

Do you want to still try to get it in, or just give up for this cycle?
If yes, which base should I use :)

Thanks,
Jason

RE: [PATCH 1/6] vfio: Add missing locking for struct vfio_group::kvm

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> Without locking userspace can trigger a UAF by racing
> KVM_DEV_VFIO_GROUP_DEL with VFIO_GROUP_GET_DEVICE_FD:
> 
>               CPU1                               CPU2
> 					    ioctl(KVM_DEV_VFIO_GROUP_DEL)
>  ioctl(VFIO_GROUP_GET_DEVICE_FD)
>     vfio_group_get_device_fd
>      open_device()
>       intel_vgpu_open_device()
>         vfio_register_notifier()
> 	 vfio_register_group_notifier()
> 	   blocking_notifier_call_chain(&group->notifier,
>                VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
> 
> 					      set_kvm()
> 						group->kvm = NULL
> 					    close()
> 					     kfree(kvm)
> 
>              intel_vgpu_group_notifier()
>                 vdev->kvm = data
>     [..]
>         kvmgt_guest_init()
>          kvm_get_kvm(info->kvm);
> 	    // UAF!
> 

this doesn't match the latest code since kvmgt_guest_init() has
been removed.

With the new code UAF will occur in an earlier place:

	ret = -ESRCH;
	if (!vgpu->kvm || vgpu->kvm->mm != current->mm) {
		gvt_vgpu_err("KVM is required to use Intel vGPU\n");
		goto undo_register;
	}

	...
	kvm_get_kvm(vgpu->kvm);

> Add a simple rwsem in the group to protect the kvm while the notifier is
> using it.
> 
> Note this doesn't fix the race internal to i915 where userspace can
> trigger two VFIO_GROUP_NOTIFY_SET_KVM's before we reach
> kvmgt_guest_init()
> and trigger this same UAF.
> 
> Fixes: ccd46dbae77d ("vfio: support notifier chain in vfio_group")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

with above flow updated:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index e6ea3981bc7c4a..0477df3a50a3d6 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -77,6 +77,7 @@ struct vfio_group {
>  	wait_queue_head_t		container_q;
>  	enum vfio_group_type		type;
>  	unsigned int			dev_counter;
> +	struct rw_semaphore		group_rwsem;
>  	struct kvm			*kvm;
>  	struct blocking_notifier_head	notifier;
>  };
> @@ -361,6 +362,7 @@ static struct vfio_group *vfio_group_alloc(struct
> iommu_group *iommu_group,
>  	group->cdev.owner = THIS_MODULE;
> 
>  	refcount_set(&group->users, 1);
> +	init_rwsem(&group->group_rwsem);
>  	INIT_LIST_HEAD(&group->device_list);
>  	mutex_init(&group->device_lock);
>  	init_waitqueue_head(&group->container_q);
> @@ -1714,9 +1716,11 @@ void vfio_file_set_kvm(struct file *file, struct kvm
> *kvm)
>  	if (file->f_op != &vfio_group_fops)
>  		return;
> 
> +	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);
> 
> @@ -2024,15 +2028,22 @@ static int vfio_register_group_notifier(struct
> vfio_group *group,
>  		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 (!ret && set_kvm && group->kvm)
> -		blocking_notifier_call_chain(&group->notifier,
> -					VFIO_GROUP_NOTIFY_SET_KVM,
> group->kvm);
> -	return ret;
> +	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,
> --
> 2.36.0

RE: [PATCH 2/6] vfio: Change struct vfio_group::opened from an atomic to bool

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> This is not a performance path, just use the group_rwsem to protect the
> value.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> +	/*
> +	 * Do we need multiple instances of the group open?  Seems not.
> +	 * Is something still in use from a previous open?
> +	 */
> +	if (group->opened || group->container) {
> +		ret = -EBUSY;
> +		goto err_put;
>  	}

I was about to suggest that probably above two checks can be
split so each has its own comment, but then noted the 2nd one
is soon removed by patch05. So it's just fine. 😊

RE: [PATCH 3/6] vfio: Split up vfio_group_get_device_fd()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> The split follows the pairing with the destroy functions:
> 
>  - vfio_group_get_device_fd() destroyed by close()
> 
>  - vfio_device_open() destroyed by vfio_device_fops_release()
> 
>  - vfio_device_assign_container() destroyed by
>    vfio_group_try_dissolve_container()
> 
> The next patch will put a lock around vfio_device_assign_container().
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> -	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
> -		return -EPERM;
> +	if (group->type == VFIO_NO_IOMMU) {
> +		if (!capable(CAP_SYS_RAWIO))
> +			return -EPERM;
> +		dev_warn(device->dev,
> +			 "vfio-noiommu device opened by user (%s:%d)\n",
> +			 current->comm, task_pid_nr(current));
> +	}

after this move is 'opened' the right status? Probably clearer to
say that the user is attempting to open a vfio-noiommu device
at this point...

RE: [PATCH 4/6] vfio: Fully lock struct vfio_group::container

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> This is necessary to avoid various user triggerable races, for instance
> racing SET_CONTAINER/UNSET_CONTAINER:
> 
>                                   ioctl(VFIO_GROUP_SET_CONTAINER)
> ioctl(VFIO_GROUP_UNSET_CONTAINER)
>  vfio_group_unset_container
>     int users = atomic_cmpxchg(&group->container_users, 1, 0);
>     // users == 1 container_users == 0
>     __vfio_group_unset_container(group);
> 
>                                     vfio_group_set_container()
> 	                              if (atomic_read(&group->container_users))

	                       if (!atomic_read(...))

> 				        down_write(&container->group_lock);
> 				        group->container = container;
> 				        up_write(&container->group_lock);
> 

It'd be clearer to add below step here:

       container = group->container;

otherwise if this assignment is done before above race then the
original container is not leaked.

>       down_write(&container->group_lock);
>       group->container = NULL;
>       up_write(&container->group_lock);
>       vfio_container_put(container);
>       /* woops we leaked the original container  */
> 
> This can then go on to NULL pointer deref since container == 0 and
> container_users == 1.
> 
> Wrap all touches of container, except those on a performance path with a
> known open device, with the group_rwsem.
> 
> The only user of vfio_group_add_container_user() holds the user count for
> a simple operation, change it to just hold the group_lock over the
> operation and delete vfio_group_add_container_user(). Containers now only
> gain a user when a device FD is opened.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c | 66 +++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index d8d14e528ab795..63f7fa872eae60 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -937,6 +937,8 @@ static void __vfio_group_unset_container(struct
> vfio_group *group)
>  	struct vfio_container *container = group->container;
>  	struct vfio_iommu_driver *driver;
> 
> +	lockdep_assert_held_write(&group->group_rwsem);
> +
>  	down_write(&container->group_lock);
> 
>  	driver = container->iommu_driver;
> @@ -973,6 +975,8 @@ static int vfio_group_unset_container(struct
> vfio_group *group)
>  {
>  	int users = atomic_cmpxchg(&group->container_users, 1, 0);
> 
> +	lockdep_assert_held_write(&group->group_rwsem);
> +
>  	if (!users)
>  		return -EINVAL;
>  	if (users != 1)
> @@ -991,8 +995,10 @@ static int vfio_group_unset_container(struct
> vfio_group *group)
>   */
>  static void vfio_group_try_dissolve_container(struct vfio_group *group)
>  {
> +	down_write(&group->group_rwsem);
>  	if (0 == atomic_dec_if_positive(&group->container_users))
>  		__vfio_group_unset_container(group);
> +	up_write(&group->group_rwsem);
>  }
> 
>  static int vfio_group_set_container(struct vfio_group *group, int
> container_fd)
> @@ -1002,6 +1008,8 @@ static int vfio_group_set_container(struct
> vfio_group *group, int container_fd)
>  	struct vfio_iommu_driver *driver;
>  	int ret = 0;
> 
> +	lockdep_assert_held_write(&group->group_rwsem);
> +
>  	if (atomic_read(&group->container_users))
>  		return -EINVAL;
> 
> @@ -1059,23 +1067,6 @@ static int vfio_group_set_container(struct
> vfio_group *group, int container_fd)
>  	return ret;
>  }
> 
> -static int vfio_group_add_container_user(struct vfio_group *group)
> -{
> -	if (!atomic_inc_not_zero(&group->container_users))
> -		return -EINVAL;
> -
> -	if (group->type == VFIO_NO_IOMMU) {
> -		atomic_dec(&group->container_users);
> -		return -EPERM;
> -	}
> -	if (!group->container->iommu_driver) {
> -		atomic_dec(&group->container_users);
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
>  static const struct file_operations vfio_device_fops;
> 
>  /* true if the vfio_device has open_device() called but not close_device() */
> @@ -1088,6 +1079,8 @@ static int vfio_device_assign_container(struct
> vfio_device *device)
>  {
>  	struct vfio_group *group = device->group;
> 
> +	lockdep_assert_held_write(&group->group_rwsem);
> +
>  	if (0 == atomic_read(&group->container_users) ||
>  	    !group->container->iommu_driver)
>  		return -EINVAL;
> @@ -1109,7 +1102,9 @@ static struct file *vfio_device_open(struct
> vfio_device *device)
>  	struct file *filep;
>  	int ret;
> 
> +	down_write(&device->group->group_rwsem);
>  	ret = vfio_device_assign_container(device);
> +	up_write(&device->group->group_rwsem);
>  	if (ret)
>  		return ERR_PTR(ret);
> 
> @@ -1219,11 +1214,13 @@ static long vfio_group_fops_unl_ioctl(struct file
> *filep,
> 
>  		status.flags = 0;
> 
> +		down_read(&group->group_rwsem);
>  		if (group->container)
>  			status.flags |= VFIO_GROUP_FLAGS_CONTAINER_SET
> |
>  					VFIO_GROUP_FLAGS_VIABLE;
>  		else if (!iommu_group_dma_owner_claimed(group-
> >iommu_group))
>  			status.flags |= VFIO_GROUP_FLAGS_VIABLE;
> +		up_read(&group->group_rwsem);
> 
>  		if (copy_to_user((void __user *)arg, &status, minsz))
>  			return -EFAULT;
> @@ -1241,11 +1238,15 @@ static long vfio_group_fops_unl_ioctl(struct file
> *filep,
>  		if (fd < 0)
>  			return -EINVAL;
> 
> +		down_write(&group->group_rwsem);
>  		ret = vfio_group_set_container(group, fd);
> +		up_write(&group->group_rwsem);
>  		break;
>  	}
>  	case VFIO_GROUP_UNSET_CONTAINER:
> +		down_write(&group->group_rwsem);
>  		ret = vfio_group_unset_container(group);
> +		up_write(&group->group_rwsem);
>  		break;
>  	case VFIO_GROUP_GET_DEVICE_FD:
>  	{
> @@ -1731,15 +1732,19 @@ bool vfio_file_enforced_coherent(struct file *file)
>  	if (file->f_op != &vfio_group_fops)
>  		return true;
> 
> -	/*
> -	 * Since the coherency state is determined only once a container is
> -	 * attached the user must do so before they can prove they have
> -	 * permission.
> -	 */
> -	if (vfio_group_add_container_user(group))
> -		return true;
> -	ret = vfio_ioctl_check_extension(group->container,
> VFIO_DMA_CC_IOMMU);
> -	vfio_group_try_dissolve_container(group);
> +	down_read(&group->group_rwsem);
> +	if (group->container) {
> +		ret = vfio_ioctl_check_extension(group->container,
> +						 VFIO_DMA_CC_IOMMU);
> +	} else {
> +		/*
> +		 * Since the coherency state is determined only once a
> container
> +		 * is attached the user must do so before they can prove they
> +		 * have permission.
> +		 */
> +		ret = true;
> +	}
> +	up_read(&group->group_rwsem);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> @@ -1932,6 +1937,7 @@ int vfio_pin_pages(struct vfio_device *device,
> unsigned long *user_pfn,
>  	if (group->dev_counter > 1)
>  		return -EINVAL;
> 
> +	/* group->container cannot change while a vfio device is open */
>  	container = group->container;
>  	driver = container->iommu_driver;
>  	if (likely(driver && driver->ops->pin_pages))
> @@ -1967,6 +1973,7 @@ int vfio_unpin_pages(struct vfio_device *device,
> unsigned long *user_pfn,
>  	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
>  		return -E2BIG;
> 
> +	/* group->container cannot change while a vfio device is open */
>  	container = device->group->container;
>  	driver = container->iommu_driver;
>  	if (likely(driver && driver->ops->unpin_pages))
> @@ -2006,6 +2013,7 @@ int vfio_dma_rw(struct vfio_device *device,
> dma_addr_t user_iova, void *data,
>  	if (!data || len <= 0 || !vfio_assert_device_open(device))
>  		return -EINVAL;
> 
> +	/* group->container cannot change while a vfio device is open */
>  	container = device->group->container;
>  	driver = container->iommu_driver;
> 
> @@ -2026,6 +2034,7 @@ static int vfio_register_iommu_notifier(struct
> vfio_group *group,
>  	struct vfio_iommu_driver *driver;
>  	int ret;
> 
> +	down_read(&group->group_rwsem);
>  	container = group->container;
>  	driver = container->iommu_driver;
>  	if (likely(driver && driver->ops->register_notifier))
> @@ -2033,6 +2042,8 @@ static int vfio_register_iommu_notifier(struct
> vfio_group *group,
>  						     events, nb);
>  	else
>  		ret = -ENOTTY;
> +	up_read(&group->group_rwsem);
> +
>  	return ret;
>  }
> 
> @@ -2043,6 +2054,7 @@ static int vfio_unregister_iommu_notifier(struct
> vfio_group *group,
>  	struct vfio_iommu_driver *driver;
>  	int ret;
> 
> +	down_read(&group->group_rwsem);
>  	container = group->container;
>  	driver = container->iommu_driver;
>  	if (likely(driver && driver->ops->unregister_notifier))
> @@ -2050,6 +2062,8 @@ static int vfio_unregister_iommu_notifier(struct
> vfio_group *group,
>  						       nb);
>  	else
>  		ret = -ENOTTY;
> +	up_read(&group->group_rwsem);
> +
>  	return ret;
>  }
> 
> --
> 2.36.0

RE: [PATCH 5/6] vfio: Simplify the life cycle of the group FD

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> Once userspace opens a group FD it is prevented from opening another
> instance of that same group FD until all the prior group FDs and users of
> the container are done.
> 
> The first is done trivially by checking the group->owned during group FD

s/owned/opened/

> open.
> 
> However, things get a little weird if userspace creates a device FD and
> then closes the group FD. The group FD still cannot be re-opened, but this
> time it is because the group->container is still set and container_users
> is elevated by the device FD.
> 
> Due to this mismatched lifecycle we have the
> vfio_group_try_dissolve_container() which tries to auto-free a container
> after the group FD is closed but the device FD remains open.
> 
> Instead have the device FD hold onto a reference to the single group
> FD. This directly prevents vfio_group_fops_release() from being called
> when any device FD exists and makes the lifecycle model more
> understandable.
> 
> vfio_group_try_dissolve_container() is removed as the only place a
> container is auto-deleted is during vfio_group_fops_release(). At this
> point the container_users is either 1 or 0 since all device FDs must be
> closed.
> 
> Change group->owner to group->singleton_filep which points to the single

s/owner/opened/

> struct file * that is open for the group. If the group->singleton_filep is
> NULL then group->container == NULL.
> 
> If all device FDs have closed then the group's notifier list must be
> empty.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

With the name change suggested by Alex:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c | 49 +++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 63f7fa872eae60..94ab415190011d 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -73,12 +73,12 @@ struct vfio_group {
>  	struct mutex			device_lock;
>  	struct list_head		vfio_next;
>  	struct list_head		container_next;
> -	bool				opened;
>  	wait_queue_head_t		container_q;
>  	enum vfio_group_type		type;
>  	unsigned int			dev_counter;
>  	struct rw_semaphore		group_rwsem;
>  	struct kvm			*kvm;
> +	struct file			*singleton_file;
>  	struct blocking_notifier_head	notifier;
>  };
> 
> @@ -987,20 +987,6 @@ static int vfio_group_unset_container(struct
> vfio_group *group)
>  	return 0;
>  }
> 
> -/*
> - * When removing container users, anything that removes the last user
> - * implicitly removes the group from the container.  That is, if the
> - * group file descriptor is closed, as well as any device file descriptors,
> - * the group is free.
> - */
> -static void vfio_group_try_dissolve_container(struct vfio_group *group)
> -{
> -	down_write(&group->group_rwsem);
> -	if (0 == atomic_dec_if_positive(&group->container_users))
> -		__vfio_group_unset_container(group);
> -	up_write(&group->group_rwsem);
> -}
> -
>  static int vfio_group_set_container(struct vfio_group *group, int
> container_fd)
>  {
>  	struct fd f;
> @@ -1093,10 +1079,19 @@ static int vfio_device_assign_container(struct
> vfio_device *device)
>  			 current->comm, task_pid_nr(current));
>  	}
> 
> +	get_file(group->singleton_file);
>  	atomic_inc(&group->container_users);
>  	return 0;
>  }
> 
> +static void vfio_device_unassign_container(struct vfio_device *device)
> +{
> +	down_write(&device->group->group_rwsem);
> +	atomic_dec(&device->group->container_users);
> +	fput(device->group->singleton_file);
> +	up_write(&device->group->group_rwsem);
> +}
> +
>  static struct file *vfio_device_open(struct vfio_device *device)
>  {
>  	struct file *filep;
> @@ -1155,7 +1150,7 @@ static struct file *vfio_device_open(struct
> vfio_device *device)
>  	mutex_unlock(&device->dev_set->lock);
>  	module_put(device->dev->driver->owner);
>  err_unassign_container:
> -	vfio_group_try_dissolve_container(device->group);
> +	vfio_device_unassign_container(device);
>  	return ERR_PTR(ret);
>  }
> 
> @@ -1286,18 +1281,12 @@ static int vfio_group_fops_open(struct inode
> *inode, struct file *filep)
> 
>  	/*
>  	 * Do we need multiple instances of the group open?  Seems not.
> -	 * Is something still in use from a previous open?
>  	 */
> -	if (group->opened || group->container) {
> +	if (group->singleton_file) {
>  		ret = -EBUSY;
>  		goto err_put;
>  	}
> -	group->opened = true;
> -
> -	/* Warn if previous user didn't cleanup and re-init to drop them */
> -	if (WARN_ON(group->notifier.head))
> -		BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
> -
> +	group->singleton_file = filep;
>  	filep->private_data = group;
> 
>  	up_write(&group->group_rwsem);
> @@ -1315,10 +1304,14 @@ static int vfio_group_fops_release(struct inode
> *inode, struct file *filep)
> 
>  	filep->private_data = NULL;
> 
> -	vfio_group_try_dissolve_container(group);
> -
>  	down_write(&group->group_rwsem);
> -	group->opened = false;
> +	/* All device FDs must be released before the group fd releases. */
> +	WARN_ON(group->notifier.head);
> +	if (group->container) {
> +		WARN_ON(atomic_read(&group->container_users) != 1);
> +		__vfio_group_unset_container(group);
> +	}
> +	group->singleton_file = NULL;
>  	up_write(&group->group_rwsem);
> 
>  	vfio_group_put(group);
> @@ -1350,7 +1343,7 @@ static int vfio_device_fops_release(struct inode
> *inode, struct file *filep)
> 
>  	module_put(device->dev->driver->owner);
> 
> -	vfio_group_try_dissolve_container(device->group);
> +	vfio_device_unassign_container(device);
> 
>  	vfio_device_put(device);
> 
> --
> 2.36.0

RE: [PATCH 6/6] vfio: Change struct vfio_group::container_users to a non-atomic int

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Friday, May 6, 2022 8:25 AM
> 
> Now that everything is fully locked there is no need for container_users
> to remain as an atomic, change it to an unsigned int.
> 
> Use 'if (group->container)' as the test to determine if the container is
> present or not instead of using container_users.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> ---
>  drivers/vfio/vfio.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 94ab415190011d..5c9f56d05f9dfa 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -66,7 +66,7 @@ struct vfio_group {
>  	struct device 			dev;
>  	struct cdev			cdev;
>  	refcount_t			users;
> -	atomic_t			container_users;
> +	unsigned int			container_users;
>  	struct iommu_group		*iommu_group;
>  	struct vfio_container		*container;
>  	struct list_head		device_list;
> @@ -431,7 +431,7 @@ static void vfio_group_put(struct vfio_group *group)
>  	 * properly hold the group reference.
>  	 */
>  	WARN_ON(!list_empty(&group->device_list));
> -	WARN_ON(atomic_read(&group->container_users));
> +	WARN_ON(group->container || group->container_users);
>  	WARN_ON(group->notifier.head);
> 
>  	list_del(&group->vfio_next);
> @@ -949,6 +949,7 @@ static void __vfio_group_unset_container(struct
> vfio_group *group)
>  	iommu_group_release_dma_owner(group->iommu_group);
> 
>  	group->container = NULL;
> +	group->container_users = 0;
>  	wake_up(&group->container_q);
>  	list_del(&group->container_next);
> 
> @@ -973,17 +974,13 @@ static void __vfio_group_unset_container(struct
> vfio_group *group)
>   */
>  static int vfio_group_unset_container(struct vfio_group *group)
>  {
> -	int users = atomic_cmpxchg(&group->container_users, 1, 0);
> -
>  	lockdep_assert_held_write(&group->group_rwsem);
> 
> -	if (!users)
> +	if (!group->container)
>  		return -EINVAL;
> -	if (users != 1)
> +	if (group->container_users != 1)
>  		return -EBUSY;
> -
>  	__vfio_group_unset_container(group);
> -
>  	return 0;
>  }
> 
> @@ -996,7 +993,7 @@ static int vfio_group_set_container(struct vfio_group
> *group, int container_fd)
> 
>  	lockdep_assert_held_write(&group->group_rwsem);
> 
> -	if (atomic_read(&group->container_users))
> +	if (group->container || WARN_ON(group->container_users))
>  		return -EINVAL;
> 
>  	if (group->type == VFIO_NO_IOMMU && !capable(CAP_SYS_RAWIO))
> @@ -1040,12 +1037,12 @@ static int vfio_group_set_container(struct
> vfio_group *group, int container_fd)
>  	}
> 
>  	group->container = container;
> +	group->container_users = 1;
>  	container->noiommu = (group->type == VFIO_NO_IOMMU);
>  	list_add(&group->container_next, &container->group_list);
> 
>  	/* Get a reference on the container and mark a user within the group
> */
>  	vfio_container_get(container);
> -	atomic_inc(&group->container_users);
> 
>  unlock_out:
>  	up_write(&container->group_lock);
> @@ -1067,8 +1064,8 @@ static int vfio_device_assign_container(struct
> vfio_device *device)
> 
>  	lockdep_assert_held_write(&group->group_rwsem);
> 
> -	if (0 == atomic_read(&group->container_users) ||
> -	    !group->container->iommu_driver)
> +	if (!group->container || !group->container->iommu_driver ||
> +	    WARN_ON(!group->container_users))
>  		return -EINVAL;
> 
>  	if (group->type == VFIO_NO_IOMMU) {
> @@ -1080,14 +1077,15 @@ static int vfio_device_assign_container(struct
> vfio_device *device)
>  	}
> 
>  	get_file(group->singleton_file);
> -	atomic_inc(&group->container_users);
> +	group->container_users++;
>  	return 0;
>  }
> 
>  static void vfio_device_unassign_container(struct vfio_device *device)
>  {
>  	down_write(&device->group->group_rwsem);
> -	atomic_dec(&device->group->container_users);
> +	WARN_ON(device->group->container_users <= 1);
> +	device->group->container_users--;
>  	fput(device->group->singleton_file);
>  	up_write(&device->group->group_rwsem);
>  }
> @@ -1308,7 +1306,7 @@ static int vfio_group_fops_release(struct inode
> *inode, struct file *filep)
>  	/* All device FDs must be released before the group fd releases. */
>  	WARN_ON(group->notifier.head);
>  	if (group->container) {
> -		WARN_ON(atomic_read(&group->container_users) != 1);
> +		WARN_ON(group->container_users != 1);
>  		__vfio_group_unset_container(group);
>  	}
>  	group->singleton_file = NULL;
> --
> 2.36.0

Re: [PATCH 1/6] vfio: Add missing locking for struct vfio_group::kvm

[Jason Gunthorpe]

On Fri, May 13, 2022 at 09:08:06AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, May 6, 2022 8:25 AM
> > 
> > Without locking userspace can trigger a UAF by racing
> > KVM_DEV_VFIO_GROUP_DEL with VFIO_GROUP_GET_DEVICE_FD:
> > 
> >               CPU1                               CPU2
> > 					    ioctl(KVM_DEV_VFIO_GROUP_DEL)
> >  ioctl(VFIO_GROUP_GET_DEVICE_FD)
> >     vfio_group_get_device_fd
> >      open_device()
> >       intel_vgpu_open_device()
> >         vfio_register_notifier()
> > 	 vfio_register_group_notifier()
> > 	   blocking_notifier_call_chain(&group->notifier,
> >                VFIO_GROUP_NOTIFY_SET_KVM, group->kvm);
> > 
> > 					      set_kvm()
> > 						group->kvm = NULL
> > 					    close()
> > 					     kfree(kvm)
> > 
> >              intel_vgpu_group_notifier()
> >                 vdev->kvm = data
> >     [..]
> >         kvmgt_guest_init()
> >          kvm_get_kvm(info->kvm);
> > 	    // UAF!
> > 
> 
> this doesn't match the latest code since kvmgt_guest_init() has
> been removed.
> 
> With the new code UAF will occur in an earlier place:
> 
> 	ret = -ESRCH;
> 	if (!vgpu->kvm || vgpu->kvm->mm != current->mm) {
> 		gvt_vgpu_err("KVM is required to use Intel vGPU\n");
> 		goto undo_register;
> 	}
> 
> 	...
> 	kvm_get_kvm(vgpu->kvm);

Right thanks

I just dropped the kvmgt_guest_init() since it is all in the same
function now.

This needs fixing as Christoph suggested by making the KVM some
integral part of the VFIO so it can't just randomly disappear under a
VFIO driver that requires it.

Jason

Re: [PATCH 4/6] vfio: Fully lock struct vfio_group::container

[Jason Gunthorpe]

On Fri, May 13, 2022 at 09:53:05AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Friday, May 6, 2022 8:25 AM
> > 
> > This is necessary to avoid various user triggerable races, for instance
> > racing SET_CONTAINER/UNSET_CONTAINER:
> > 
> >                                   ioctl(VFIO_GROUP_SET_CONTAINER)
> > ioctl(VFIO_GROUP_UNSET_CONTAINER)
> >  vfio_group_unset_container
> >     int users = atomic_cmpxchg(&group->container_users, 1, 0);
> >     // users == 1 container_users == 0
> >     __vfio_group_unset_container(group);
> > 
> >                                     vfio_group_set_container()
> > 	                              if (atomic_read(&group->container_users))
> 
> 	                       if (!atomic_read(...))
> 
> > 				        down_write(&container->group_lock);
> > 				        group->container = container;
> > 				        up_write(&container->group_lock);
> > 
> 
> It'd be clearer to add below step here:
> 
>        container = group->container;
> 
> otherwise if this assignment is done before above race then the
> original container is not leaked.

Not quite, it is the assignment to NULL that is trouble:

> >       down_write(&container->group_lock);
> >       group->container = NULL;
> >       up_write(&container->group_lock);

Either we leaked the original container or we leaked the new
container, and now the atomics are out of sync with the state of
group->container.

But sure, it is a touch clearer to focus only one scenario, I picked
the one where group->container is read early and we leak the new
container.

Thanks,
Jason