Re: [PATCH v6 01/14] iommu: Introduce a replace API for device pasid

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 14, 2025 at 08:10:41AM +0000, Tian, Kevin wrote:

> > > +	ret = __iommu_set_group_pasid(domain, group, pasid, curr-
> > >domain);
> > > +	if (ret)
> > > +		WARN_ON(handle != xa_store(&group->pasid_array, pasid,
> > > +					   curr, GFP_KERNEL));
> > 
> > I wonder about the ordering here, is it OK to have PRIs being
> > delivered to a domain that failed to attach? What cleans up that race
> > condition with domain free?
> > 
> > Should we replace the domain then set the xarray? (and same ordering
> > question for normal attach)
> 
> That makes sense to me.
> 
> But I don't think there is a problem with attach. xa_insert() will
> return error if an entry already exists. So there won't be any
> PRI being delivered at __iommu_set_group_pasid(), no matter
> it succeeds or not.

It has the same issue:

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

	ret = __iommu_set_group_pasid(domain, group, pasid);
        .. Concurrently a PRI event is pushed to the domain ..
	if (ret)
		xa_erase(&group->pasid_array, pasid);

        .. Now what? Who fences the PRI event thread before the caller
	   frees the domain ..?

We arranged things so that detatch would fence the PRI, if detach is
not called then there is no fence..

Jason