Re: [PATCH 9/9] vfio: Remove calls to vfio_group_add_container_user()

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Apr 14, 2022 at 09:51:49AM -0400, Matthew Rosato wrote:
> On 4/12/22 11:53 AM, Jason Gunthorpe wrote:
> > When the open_device() op is called the container_users is incremented and
> > held incremented until close_device(). Thus, so long as drivers call
> > functions within their open_device()/close_device() region they do not
> > need to worry about the container_users.
> > 
> > These functions can all only be called between
> > open_device()/close_device():
> > 
> >    vfio_pin_pages()
> >    vfio_unpin_pages()
> >    vfio_dma_rw()
> >    vfio_register_notifier()
> >    vfio_unregister_notifier()
> > 
> > So eliminate the calls to vfio_group_add_container_user() and add a simple
> > WARN_ON to detect mis-use by drivers.
> > 
> 
> vfio_device_fops_release decrements dev->open_count immediately before
> calling dev->ops->close_device, which means we could enter close_device with
> a dev_count of 0.
> 
> Maybe vfio_device_fops_release should handle the same way as
> vfio_group_get_device_fd?
> 
> 	if (device->open_count == 1 && device->ops->close_device)
> 		device->ops->close_device(device);
> 	device->open_count--;

Yes, thanks alot! I have nothing to test these flows on...

It matches the ordering in the only other place to call close_device.

I folded this into the patch:

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index 0f735f9f206002..29761f0cf0a227 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -1551,8 +1551,9 @@ static int vfio_device_fops_release(struct inode *inode, struct file *filep)
 
 	mutex_lock(&device->dev_set->lock);
 	vfio_assert_device_open(device);
-	if (!--device->open_count && device->ops->close_device)
+	if (device->open_count == 1 && device->ops->close_device)
 		device->ops->close_device(device);
+	device->open_count--;
 	mutex_unlock(&device->dev_set->lock);
 
 	module_put(device->dev->driver->owner);

Jason