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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > 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
