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

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

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