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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Jan 15, 2025 at 04:43:41AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Tuesday, January 14, 2025 9:46 PM
> > 
> > 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..
> > 
> 
> Though I'm fine to change the order in attach too as it looks more
> reasonable logically, I'm trying to understand the actual impact of
> the original order (e.g. is the change worth of a Fix tag?)
> 
> If there is no detach happened before then it's the 1st attach to
> a faultable domain and PRI will be enabled right before this function
> hence no fence required.
> 
> Would a sane device trigger PRI in this window?

I would say this is not a "sane" scenario, this is a theoretical race
triggerable by the device. Perhaps a VFIO user can force the device to
trigger this race and exploit the kernel.

> If it's a detach-then-attach flow, detach will do the fence anyway
> before the attach.

The issue is the error, once we do the xa_insert() then any faults will
get routed to our domain and the fault path threads will hold pointers
to the domain. Once the xa_insert() is done we must flush the fault
path threads before allowing the domain to be freed.

If __iommu_set_group_pasid() fails then we do an xa_erase() but
nothing will flush the fault threads.

Jason