Re: [PATCH 4/5] iommu: Swap the order of setting group->pasid_array and calling attach op of iommu drivers

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Feb 11, 2025 at 10:05:39PM -0800, Yi Liu wrote:
> The current implementation stores group->pasid_array before the underlying
> iommu driver has successfully set the new domain. This can lead to issues
> where PRIs are received on the new domain before the attach operation is
> completed. This patch swaps the order of operations to ensure that the
> domain is set in the iommu driver before updating group->pasid_array.

I think I would drop this note in a comment since it is subtle and
important..

> -	ret = xa_insert(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL);
> -	if (ret)
> +	if (xa_load(&group->pasid_array, pasid)) {
> +		ret = -EBUSY;
>  		goto out_unlock;
> +	}

Let's write it like this:

	ret = xa_insert(&group->pasid_array, pasid, XA_ZERO_ENTRY, GFP_KERNEL);
	if (ret)
		goto out_unlock;

	ret = __iommu_set_group_pasid(domain, group, pasid);
	if (ret) {
		xa_release(&group->pasid_array, pasid);
		goto out_unlock;
	}

	/*
	 * The xa_insert() above reserved the memory, and the group->mutex is
	 * held, this cannot fail. The new domain cannot be visible until the
	 * operation succeeds as we cannot tolerate PRIs becoming concurrently
	 * queued and then failing attach.
	 */
	ret = xa_err(
		xa_store(&group->pasid_array, pasid, pasid_entry, GFP_KERNEL));
	WARN_ON(ret);
	ret = 0;

> @@ -3510,19 +3518,26 @@ int iommu_attach_group_handle(struct iommu_domain *domain,
>  	pasid_entry = iommu_make_pasid_entry(domain, handle);
>  
>  	mutex_lock(&group->mutex);
> -	ret = xa_insert(&group->pasid_array,
> -			IOMMU_NO_PASID, pasid_entry, GFP_KERNEL);
> -	if (ret)
> +	if (xa_load(&group->pasid_array, IOMMU_NO_PASID)) {
> +		ret = -EBUSY;
>  		goto err_unlock;
> +	}

Same here

Jason