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

Jason Gunthorpe
2022-04-13T14:03:12Z

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