Re: [PATCH v3 3/6] iommu: Same critical region for device release and removal

[Jason Gunthorpe]

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@xxxxxxxxxx>
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>

Jason