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
