Re: [Intel-gfx] [PATCH 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, May 17, 2022 at 02:08:51PM -0400, 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 also fixes a user triggerable oops in gvt
 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>

You can add my signed-off-by as well

> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index cfcff7764403..c5d421eda275 100644
> +++ b/drivers/vfio/vfio.c
> @@ -10,6 +10,7 @@
>   * Author: Tom Lyon, pugs@cisco.com
>   */
>  
> +#include "linux/kvm_host.h"

This is the wrong format of include (editor automation, sigh)

> @@ -1083,6 +1084,13 @@ static struct file *vfio_device_open(struct vfio_device *device)
>  
>  	mutex_lock(&device->dev_set->lock);
>  	device->open_count++;
> +	down_write(&device->group->group_rwsem);

Read I suppose

> +	if (device->open_count == 1 && device->group->kvm) {
> +		device->kvm = device->group->kvm;
> +		kvm_get_kvm(device->kvm);
> +	}
> +	up_write(&device->group->group_rwsem);

Yeah, this is OK, not very elegant though

I was looking at this - but it could come later too:

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 8e11d9119418be..c5d8dfe8314108 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -10,7 +10,6 @@
  * Author: Tom Lyon, pugs@cisco.com
  */
 
-#include "linux/kvm_host.h"
 #include <linux/cdev.h>
 #include <linux/compat.h>
 #include <linux/device.h>
@@ -33,6 +32,7 @@
 #include <linux/vfio.h>
 #include <linux/wait.h>
 #include <linux/sched/signal.h>
+#include <linux/kvm_host.h>
 #include "vfio.h"
 
 #define DRIVER_VERSION	"0.3"
@@ -1059,93 +1059,71 @@ static int vfio_device_assign_container(struct vfio_device *device)
 
 static void vfio_device_unassign_container(struct vfio_device *device)
 {
-	down_write(&device->group->group_rwsem);
+	lockdep_assert_held(&device->group->group_rwsem);
+
 	WARN_ON(device->group->container_users <= 1);
 	device->group->container_users--;
 	fput(device->group->opened_file);
-	up_write(&device->group->group_rwsem);
 }
 
-static struct file *vfio_device_open(struct vfio_device *device)
+static int vfio_device_open(struct vfio_device *device)
 {
-	struct file *filep;
 	int ret;
 
+	lockdep_assert_held(&device->dev_set->lock);
+
+	if (!try_module_get(device->dev->driver->owner))
+		return -ENODEV;
+
 	down_write(&device->group->group_rwsem);
 	ret = vfio_device_assign_container(device);
-	if (ret) {
-		up_write(&device->group->group_rwsem);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto err_unlock;
 
 	if (device->ops->flags & VFIO_DEVICE_NEEDS_KVM)
 	{
-		if (!device->group->kvm) {
-			up_write(&device->group->group_rwsem);
+		if (!device->group->kvm)
 			goto err_unassign_container;
-		}
 		device->kvm = device->group->kvm;
 		kvm_get_kvm(device->kvm);
 	}
 	up_write(&device->group->group_rwsem);
 
-	if (!try_module_get(device->dev->driver->owner)) {
-		ret = -ENODEV;
-		goto err_put_kvm;
-	}
-
-	mutex_lock(&device->dev_set->lock);
-	device->open_count++;
-	if (device->open_count == 1 && device->ops->open_device) {
+	if (device->ops->open_device) {
 		ret = device->ops->open_device(device);
 		if (ret)
-			goto err_undo_count;
+			goto err_put_kvm;
 	}
-	mutex_unlock(&device->dev_set->lock);
+	return 0;
 
-	/*
-	 * We can't use anon_inode_getfd() because we need to modify
-	 * the f_mode flags directly to allow more than just ioctls
-	 */
-	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
-				   device, O_RDWR);
-	if (IS_ERR(filep)) {
-		ret = PTR_ERR(filep);
-		goto err_close_device;
+err_put_kvm:
+	if (device->kvm) {
+		kvm_put_kvm(device->kvm);
+		device->kvm = NULL;
 	}
+	down_write(&device->group->group_rwsem);
+err_unassign_container:
+	vfio_device_unassign_container(device);
+err_unlock:
+	up_write(&device->group->group_rwsem);
+	module_put(device->dev->driver->owner);
+	return ret;
+}
 
-	/*
-	 * TODO: add an anon_inode interface to do this.
-	 * Appears to be missing by lack of need rather than
-	 * explicitly prevented.  Now there's need.
-	 */
-	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
-
-	if (device->group->type == VFIO_NO_IOMMU)
-		dev_warn(device->dev, "vfio-noiommu device opened by user "
-			 "(%s:%d)\n", current->comm, task_pid_nr(current));
-	/*
-	 * On success the ref of device is moved to the file and
-	 * put in vfio_device_fops_release()
-	 */
-	return filep;
+static void vfio_device_close(struct vfio_device *device)
+{
+	lockdep_assert_held(&device->dev_set->lock);
 
-err_close_device:
-	mutex_lock(&device->dev_set->lock);
-	if (device->open_count == 1 && device->ops->close_device)
+	if (device->ops->close_device)
 		device->ops->close_device(device);
-err_undo_count:
-	device->open_count--;
-	mutex_unlock(&device->dev_set->lock);
-	module_put(device->dev->driver->owner);
-err_put_kvm:
 	if (device->kvm) {
 		kvm_put_kvm(device->kvm);
 		device->kvm = NULL;
 	}
-err_unassign_container:
+	down_write(&device->group->group_rwsem);
 	vfio_device_unassign_container(device);
-	return ERR_PTR(ret);
+	up_write(&device->group->group_rwsem);
+	module_put(device->dev->driver->owner);
 }
 
 static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
@@ -1159,23 +1137,61 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *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;
+	mutex_lock(&device->dev_set->lock);
+	device->open_count++;
+	if (device->open_count == 1) {
+		ret = vfio_device_open(device);
+		if (ret) {
+			device->open_count--;
+			mutex_unlock(&device->dev_set->lock);
+			goto err_put_device;
+		}
 	}
+	mutex_unlock(&device->dev_set->lock);
 
-	filep = vfio_device_open(device);
+	/*
+	 * We can't use anon_inode_getfd() because we need to modify
+	 * the f_mode flags directly to allow more than just ioctls
+	 */
+	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops, device,
+				   O_RDWR);
 	if (IS_ERR(filep)) {
 		ret = PTR_ERR(filep);
-		goto err_put_fdno;
+		goto err_close_device;
+	}
+
+	/*
+	 * TODO: add an anon_inode interface to do this.
+	 * Appears to be missing by lack of need rather than
+	 * explicitly prevented.  Now there's need.
+	 */
+	filep->f_mode |= (FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE);
+
+	fdno = get_unused_fd_flags(O_CLOEXEC);
+	if (fdno < 0) {
+		ret = fdno;
+		goto err_put_file;
 	}
 
+	if (device->group->type == VFIO_NO_IOMMU)
+		dev_warn(device->dev, "vfio-noiommu device opened by user "
+			 "(%s:%d)\n", current->comm, task_pid_nr(current));
+
+	/*
+	 * On success the ref of device is moved to the file and put in
+	 * vfio_device_fops_release().
+	 */
 	fd_install(fdno, filep);
 	return fdno;
 
-err_put_fdno:
-	put_unused_fd(fdno);
+err_put_file:
+	fput(filep);
+err_close_device:
+	mutex_lock(&device->dev_set->lock);
+	if (device->open_count == 1)
+		vfio_device_close(device);
+	device->open_count--;
+	mutex_unlock(&device->dev_set->lock);
 err_put_device:
 	vfio_device_put(device);
 	return ret;
@@ -1333,19 +1349,11 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
-	if (device->open_count == 1 && device->ops->close_device)
-		device->ops->close_device(device);
+	if (device->open_count == 1)
+		vfio_device_close(device);
 	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
 
-	module_put(device->dev->driver->owner);
-
-	if (device->kvm) {
-		kvm_put_kvm(device->kvm);
-		device->kvm = NULL;
-	}
-	vfio_device_unassign_container(device);
-
 	vfio_device_put(device);
 
 	return 0;