Re: [PATCH v5 06/17] iommu/arm-smmu-v3: Hold arm_smmu_asid_lock during all of attach_dev

Mostafa Saleh
2024-02-13T15:39:17Z

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