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

[Jason Gunthorpe]

On Wed, Apr 13, 2022 at 08:11:05AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 12, 2022 at 12:53:36PM -0300, Jason Gunthorpe wrote:
> > +   if (WARN_ON(!READ_ONCE(vdev->open_count)))
> > +           return -EINVAL;
> 
> I think all the WARN_ON()s in this patch need to be WARN_ON_ONCE,
> otherwise there will be too many backtraces to be useful if a driver
> ever gets the API wrong.

Sure, I added a wrapper to make that have less overhead and merged it
with the other 'driver is calling this correctly' checks:

@@ -1330,6 +1330,12 @@ static int vfio_group_add_container_user(struct 
vfio_group *group)
 
 static const struct file_operations vfio_device_fops;
 
+/* true if the vfio_device has open_device() called but not close_device() */
+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)
 {
        struct vfio_device *device;
@@ -1544,6 +1550,7 @@ static int vfio_device_fops_release(struct inode *inode, 
struct file *filep)
        struct vfio_device *device = filep->private_data;
 
        mutex_lock(&device->dev_set->lock);
+       vfio_assert_device_open(device);
        if (!--device->open_count && device->ops->close_device)
                device->ops->close_device(device);
        mutex_unlock(&device->dev_set->lock);
@@ -2112,7 +2119,7 @@ int vfio_pin_pages(struct vfio_device *vdev, unsigned 
long *user_pfn, int npage,
        struct vfio_iommu_driver *driver;
        int ret;
 
-       if (!user_pfn || !phys_pfn || !npage)
+       if (!user_pfn || !phys_pfn || !npage || !vfio_assert_device_open(vdev))
                return -EINVAL;
 
        if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
@@ -2121,9 +2128,6 @@ int vfio_pin_pages(struct vfio_device *vdev, unsigned 
long *user_pfn, int npage,
        if (group->dev_counter > 1)
                return -EINVAL;
 
-       if (WARN_ON(!READ_ONCE(vdev->open_count)))
-               return -EINVAL;
-
        container = group->container;
        driver = container->iommu_driver;
        if (likely(driver && driver->ops->pin_pages))
@@ -2153,15 +2157,12 @@ int vfio_unpin_pages(struct vfio_device *vdev, unsigned 
long *user_pfn,
        struct vfio_iommu_driver *driver;
        int ret;
 
-       if (!user_pfn || !npage)
+       if (!user_pfn || !npage || !vfio_assert_device_open(vdev))
                return -EINVAL;
 
        if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
                return -E2BIG;
 
-       if (WARN_ON(!READ_ONCE(vdev->open_count)))
-               return -EINVAL;
-
        container = vdev->group->container;
        driver = container->iommu_driver;
        if (likely(driver && driver->ops->unpin_pages))
@@ -2198,10 +2199,7 @@ int vfio_dma_rw(struct vfio_device *vdev, dma_addr_t 
user_iova,
        struct vfio_iommu_driver *driver;
        int ret = 0;
 
-       if (!data || len <= 0)
-               return -EINVAL;
-
-       if (WARN_ON(!READ_ONCE(vdev->open_count)))
+       if (!data || len <= 0 || !vfio_assert_device_open(vdev))
                return -EINVAL;
 
        container = vdev->group->container;
@@ -2294,10 +2292,7 @@ int vfio_register_notifier(struct vfio_device *dev, enum 
vfio_notify_type type,
        struct vfio_group *group = dev->group;
        int ret;
 
-       if (!nb || !events || (*events == 0))
-               return -EINVAL;
-
-       if (WARN_ON(!READ_ONCE(dev->open_count)))
+       if (!nb || !events || (*events == 0) || !vfio_assert_device_open(dev))
                return -EINVAL;
 
        switch (type) {
@@ -2321,10 +2316,7 @@ int vfio_unregister_notifier(struct vfio_device *dev,
        struct vfio_group *group = dev->group;
        int ret;
 
-       if (!nb)
-               return -EINVAL;
-
-       if (WARN_ON(!READ_ONCE(dev->open_count)))
+       if (!nb || !vfio_assert_device_open(dev))
                return -EINVAL;
 
        switch (type) {

Thanks,
Jason