[v3,3/6] iommu: Same critical region for device release and removal
Commit Message
In a non-driver context, it is crucial to ensure the consistency of a
device's iommu ops. Otherwise, it may result in a situation where a
device is released but it's iommu ops are still used.
Put the ops->release_device and __iommu_group_remove_device() in a some
group->mutext critical region, so that, as long as group->mutex is held
and the device is in its group's device list, its iommu ops are always
consistent. Add check of group ownership if the released device is the
last one.
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
Comments
On Mon, Mar 06, 2023 at 10:58:01AM +0800, Lu Baolu wrote:
> In a non-driver context, it is crucial to ensure the consistency of a
> device's iommu ops. Otherwise, it may result in a situation where a
> device is released but it's iommu ops are still used.
>
> Put the ops->release_device and __iommu_group_remove_device() in a some
> group->mutext critical region, so that, as long as group->mutex is held
> and the device is in its group's device list, its iommu ops are always
> consistent. Add check of group ownership if the released device is the
> last one.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++++--
> 1 file changed, 28 insertions(+), 2 deletions(-)
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bd9b293e07a8..0bcd9625090d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -507,18 +507,44 @@ static void __iommu_group_release_device(struct iommu_group *group,
>
> void iommu_release_device(struct device *dev)
> {
> + struct iommu_group *group = dev->iommu_group;
> + struct group_device *device;
> const struct iommu_ops *ops;
>
> - if (!dev->iommu)
> + if (!dev->iommu || !group)
> return;
>
> iommu_device_unlink(dev->iommu->iommu_dev, dev);
>
> + mutex_lock(&group->mutex);
> + device = __iommu_group_remove_device(group, dev);
> +
> + /*
> + * If the group has become empty then ownership must have been released,
> + * and the current domain must be set back to NULL or the default
> + * domain.
> + */
> + if (list_empty(&group->devices))
> + WARN_ON(group->owner_cnt ||
> + group->domain != group->default_domain);
> +
> + /*
> + * release_device() must stop using any attached domain on the device.
> + * If there are still other devices in the group they are not effected
> + * by this callback.
> + *
> + * The IOMMU driver must set the device to either an identity or
> + * blocking translation and stop using any domain pointer, as it is
> + * going to be freed.
> + */
> ops = dev_iommu_ops(dev);
> if (ops->release_device)
> ops->release_device(dev);
> + mutex_unlock(&group->mutex);
IMHO it is best to be locked like this
But for this series, if you run into problems with ARM and
release_device I think we could still safely unlock group->mutex
before calling this?
It would still be OK because the iommu_group_first_dev() will either
return NULL so iommu_group_store_type() wills top, or it will block
the ultimate call to release here which invalidate's ops.
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
@@ -507,18 +507,44 @@ static void __iommu_group_release_device(struct iommu_group *group,
void iommu_release_device(struct device *dev)
{
+ struct iommu_group *group = dev->iommu_group;
+ struct group_device *device;
const struct iommu_ops *ops;
- if (!dev->iommu)
+ if (!dev->iommu || !group)
return;
iommu_device_unlink(dev->iommu->iommu_dev, dev);
+ mutex_lock(&group->mutex);
+ device = __iommu_group_remove_device(group, dev);
+
+ /*
+ * If the group has become empty then ownership must have been released,
+ * and the current domain must be set back to NULL or the default
+ * domain.
+ */
+ if (list_empty(&group->devices))
+ WARN_ON(group->owner_cnt ||
+ group->domain != group->default_domain);
+
+ /*
+ * release_device() must stop using any attached domain on the device.
+ * If there are still other devices in the group they are not effected
+ * by this callback.
+ *
+ * The IOMMU driver must set the device to either an identity or
+ * blocking translation and stop using any domain pointer, as it is
+ * going to be freed.
+ */
ops = dev_iommu_ops(dev);
if (ops->release_device)
ops->release_device(dev);
+ mutex_unlock(&group->mutex);
+
+ if (device)
+ __iommu_group_release_device(group, device);
- iommu_group_remove_device(dev);
module_put(ops->owner);
dev_iommu_free(dev);
}