| Message ID | 6-v5-cd1be8dd9c71+3fa-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers | expand |
Hi Jason, On Tue, Feb 06, 2024 at 11:12:43AM -0400, Jason Gunthorpe wrote: > The BTM support wants to be able to change the ASID of any smmu_domain. > When it goes to do this it holds the arm_smmu_asid_lock and iterates over > the target domain's devices list. > > During attach of a S1 domain we must ensure that the devices list and > CD are in sync, otherwise we could miss CD updates or a parallel CD update > could push an out of date CD. > > This is pretty complicated, and almost works today because > arm_smmu_detach_dev() removes the master from the linked list before > working on the CD entries, preventing parallel update of the CD. > > However, it does have an issue where the CD can remain programed while the > domain appears to be unattached. arm_smmu_share_asid() will then not clear > any CD entriess and install its own CD entry with the same ASID > concurrently. This creates a small race window where the IOMMU can see two > ASIDs pointing to different translations. > > Solve this by wrapping most of the attach flow in the > arm_smmu_asid_lock. This locks more than strictly needed to prepare for > the next patch which will reorganize the order of the linked list, STE and > CD changes. > > Move arm_smmu_detach_dev() till after we have initialized the domain so > the lock can be held for less time. > This seems a bit theoretical to me also it requires mis-programming as the master will issue DMA in detach, but as this is not a hot path, I don’t think overlocking will is a problem. Reviewed-by: Mostafa Saleh <smostafa@google.com> > Reviewed-by: Michael Shavit <mshavit@google.com> > Reviewed-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Tested-by: Moritz Fischer <moritzf@google.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 22 ++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > index 417b2c877ff311..1229545ae6db4e 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2639,8 +2639,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > return -EBUSY; > } > > - arm_smmu_detach_dev(master); > - > mutex_lock(&smmu_domain->init_mutex); > > if (!smmu_domain->smmu) { > @@ -2655,6 +2653,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (ret) > return ret; > > + /* > + * Prevent arm_smmu_share_asid() from trying to change the ASID > + * of either the old or new domain while we are working on it. > + * This allows the STE and the smmu_domain->devices list to > + * be inconsistent during this routine. > + */ > + mutex_lock(&arm_smmu_asid_lock); > + > + arm_smmu_detach_dev(master); > + > master->domain = smmu_domain; > > /* > @@ -2680,13 +2688,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > } > > - /* > - * Prevent SVA from concurrently modifying the CD or writing to > - * the CD entry > - */ > - mutex_lock(&arm_smmu_asid_lock); > ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); > - mutex_unlock(&arm_smmu_asid_lock); > if (ret) { > master->domain = NULL; > goto out_list_del; > @@ -2696,13 +2698,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > arm_smmu_install_ste_for_dev(master); > > arm_smmu_enable_ats(master); > - return 0; > + goto out_unlock; > > out_list_del: > spin_lock_irqsave(&smmu_domain->devices_lock, flags); > list_del(&master->domain_head); > spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > > +out_unlock: > + mutex_unlock(&arm_smmu_asid_lock); > return ret; > } > > -- > 2.43.0 >
On Tue, Feb 13, 2024 at 03:38:57PM +0000, Mostafa Saleh wrote: > This seems a bit theoretical to me also it requires mis-programming as the master > will issue DMA in detach iommufd allows VFIO userspace to do this workload as an attack on the kernel. Thanks, Jason
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 417b2c877ff311..1229545ae6db4e 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2639,8 +2639,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) return -EBUSY; } - arm_smmu_detach_dev(master); - mutex_lock(&smmu_domain->init_mutex); if (!smmu_domain->smmu) { @@ -2655,6 +2653,16 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) if (ret) return ret; + /* + * Prevent arm_smmu_share_asid() from trying to change the ASID + * of either the old or new domain while we are working on it. + * This allows the STE and the smmu_domain->devices list to + * be inconsistent during this routine. + */ + mutex_lock(&arm_smmu_asid_lock); + + arm_smmu_detach_dev(master); + master->domain = smmu_domain; /* @@ -2680,13 +2688,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } } - /* - * Prevent SVA from concurrently modifying the CD or writing to - * the CD entry - */ - mutex_lock(&arm_smmu_asid_lock); ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd); - mutex_unlock(&arm_smmu_asid_lock); if (ret) { master->domain = NULL; goto out_list_del; @@ -2696,13 +2698,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) arm_smmu_install_ste_for_dev(master); arm_smmu_enable_ats(master); - return 0; + goto out_unlock; out_list_del: spin_lock_irqsave(&smmu_domain->devices_lock, flags); list_del(&master->domain_head); spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); +out_unlock: + mutex_unlock(&arm_smmu_asid_lock); return ret; }