Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 9 Mar 2023 21:08:55 -0400 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v3 3/6] iommu: Same critical region for device release and removal |
| |
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
| ![\](/images/icornerr.gif) |