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

[Jason Gunthorpe <jgg@nvidia.com>]

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