> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Thursday, March 30, 2023 3:57 AM
>
> On Mon, 27 Mar 2023 02:40:43 -0700
> Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
>
[...]
> > +/*
> > + * device access via the fd opened by this function is blocked until
> > + * .open_device() is called successfully during BIND_IOMMUFD.
> > + */
> > +int vfio_device_fops_cdev_open(struct inode *inode, struct file *filep)
> > +{
> > + struct vfio_device *device = container_of(inode->i_cdev,
> > + struct vfio_device, cdev);
> > + struct vfio_device_file *df;
> > + int ret;
> > +
> > + if (!vfio_device_try_get_registration(device))
> > + return -ENODEV;
> > +
> > + df = vfio_allocate_device_file(device);
> > + if (IS_ERR(df)) {
> > + ret = PTR_ERR(df);
> > + goto err_put_registration;
> > + }
> > +
> > + filep->private_data = df;
> > +
> > + return 0;
> > +
> > +err_put_registration:
> > + vfio_device_put_registration(device);
> > + return ret;
> > +}
> > +
[...]
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 8e96aab27029..58fc3bb768f2 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -242,6 +242,7 @@ static int vfio_init_device(struct vfio_device *device, struct
> device *dev,
> > device->device.release = vfio_device_release;
> > device->device.class = vfio.device_class;
> > device->device.parent = device->dev;
> > + vfio_init_device_cdev(device);
> > return 0;
> >
> > out_uninit:
> > @@ -280,7 +281,7 @@ static int __vfio_register_dev(struct vfio_device *device,
> > if (ret)
> > goto err_out;
> >
> > - ret = device_add(&device->device);
> > + ret = vfio_device_add(device);
> > if (ret)
> > goto err_out;
> >
> > @@ -320,6 +321,12 @@ void vfio_unregister_group_dev(struct vfio_device *device)
> > bool interrupted = false;
> > long rc;
> >
> > + /* Prevent new device opened in the group path */
> > + vfio_device_group_unregister(device);
> > +
> > + /* Prevent new device opened in the cdev path */
> > + vfio_device_del(device);
> > +
> > vfio_device_put_registration(device);
> > rc = try_wait_for_completion(&device->comp);
> > while (rc <= 0) {
> > @@ -343,11 +350,6 @@ void vfio_unregister_group_dev(struct vfio_device *device)
> > }
> > }
> >
> > - vfio_device_group_unregister(device);
> > -
> > - /* Balances device_add in register path */
> > - device_del(&device->device);
> > -
>
> Why were these relocated? And additionally why was the comment
> regarding the balance operations dropped? The move seems unrelated to
> the patch here, so if it's actually advisable for some reason, it
> should be a separate patch. Thanks,
The reason for the relocation is to prevent new device which would result
in the device->refcount increasing. If the user keeps open device then the
device->refcount may keep increasing. Then the vfio_unregister_group_dev()
may be stuck here. This is rare, but possible.
By doing vfio_device_group_unregister(), the device is removed from the
group->device_list. Then user cannot open the device by VFIO_GROUP_GET_DEVICE_FD.
Hence it won't increase the device->refcount. I agree with you, this should
be done in a separate patch.
Same reason for relocating device_del(&device->device); User may keep
opening the cdev to increase the device->refcount. Then the
vfio_device_group_unregister() path would be stuck as well. But this
relocation needs to be done here since user cannot do it if without cdev.
Last, need to keep the balance comment as well even the sequence
it not strictly mirrored. will keep the comment.
> Alex
>
> > /* Balances vfio_device_set_group in register path */
> > vfio_device_remove_group(device);
> > }
> > @@ -555,7 +557,8 @@ static int vfio_device_fops_release(struct inode *inode,
> struct file *filep)
> > struct vfio_device_file *df = filep->private_data;
> > struct vfio_device *device = df->device;
> >
> > - vfio_device_group_close(df);
> > + if (df->group)
> > + vfio_device_group_close(df);
> >
> > vfio_device_put_registration(device);
> >
Thanks,
Yi Liu
