Re: [PATCH v4 01/27] iommu/arm-smmu-v3: Check that the RID domain is S1 in SVA

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 30, 2024 at 05:11:36PM +0000, Shameerali Kolothum Thodi wrote:
> > 
> > > > +	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
> > > > +		return -ENODEV;
> > >
> > > I think we need to do the above checks in
> > arm_smmu_sva_remove_dev_pasid()
> > > as well.
> > 
> > The core won't call that unless a PASID is already attached, meaning
> > we already passed the above check in bind. So it shouldn't need to be
> > duplicated.
> 
> I think it does in the error path.  See __iommu_set_group_pasid() call.
> I haven't tested what happens if that returns error because of identity 
> domain and then __iommu_remove_group_pasid() is called.

You are correct, but this is a problem in the core code :(

Driver should not have to deal with this unbalance. A set/remove
pairing should be enforced by the core.

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3481,15 +3481,24 @@ EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
                                   struct iommu_group *group, ioasid_t pasid)
 {
+       struct group_device *last_gdev;
        struct group_device *device;
        int ret = 0;
 
        for_each_group_device(group, device) {
                ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
                if (ret)
-                       break;
+                       goto err_revert;
        }
+       return 0;
 
+err_revert:
+       last_gdev = device;
+       for_each_group_device(group, device) {
+               if (device == last_gdev)
+                       break;
+               dev_iommu_ops(device->dev)->remove_dev_pasid(device->dev, pasid);
+       }
        return ret;
 }
 
@@ -3538,10 +3547,8 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
        }
 
        ret = __iommu_set_group_pasid(domain, group, pasid);
-       if (ret) {
-               __iommu_remove_group_pasid(group, pasid);
+       if (ret)
                xa_erase(&group->pasid_array, pasid);
-       }
 out_unlock:
        mutex_unlock(&group->mutex);
        return ret;

> So as an exported function, still think it is better to check for domain
> type before accessing smmu_domain there.

After part 2 the remove path doesn't touch the RID so I prefer to
leave it in part 1 and it will be fully safe in part 2. It is just
more code that part 2 has to remove.

Thanks,
Jason