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

[Alex Williamson <alex.williamson@redhat.com>]

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);
>