Re: [PATCH v9 02/21] iommu: Wrap pasid_array entry creation and setting

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Mar 13, 2025 at 05:35:13AM -0700, Yi Liu wrote:
> The IOMMU core does not mandate that callers must always provide a new
> handle, allowing for the possibility of handle reuse. In the replace
> path, the existing handle can be reused. To facilitate this, the core
> must ensure that the pasid_array entry is made or updated under xa_lock
> to prevent race conditions with callers of
> iommu_attach_handle_get().

I don't think that helps, the access to handle->domain is done unlocked:

static struct iommu_attach_handle *find_fault_handler(struct device *dev,
						     struct iopf_fault *evt)
{
[..]
	if (!attach_handle->domain->iopf_handler)
		return NULL;

And so on. So even with this locking change the domain value is unstable.

The driver still has to fence the iopf queue to flush out the domain
references during replace.

We decided the instability of fault delivery during replace is fine,
as it is logically OK for either domain to receive the fault.

What is problematic here is the repeated references to handle->domain
in the fault path without locking during handle reuse. It should be
using READ_ONCE(attach_handle->domain) and it should happen only once.

> Additionally, this operation should be performed only after the underlying
> IOMMU driver has successfully set the domain. This precaution is necessary
> to prevent forwarding PRIs to the new domain before it is fully prepared.

This can't work, we need to change the xarray, then have the driver
do the fencing to flush out the old xarray value from the fault path.

Otherwise the old handle and domain is still floating out there after
replace/attach returns which will UAF the domain pointer.

Jason