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