RE: [PATCH 1/1] iommu/vt-d: Avoid draining PRQ in sva mm release path

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Thursday, December 12, 2024 10:15 AM
> 
> When a PASID is used for SVA by a device, it's possible that the PASID
> entry is cleared before the device flushes all ongoing DMA requests and
> removes the SVA domain. This can occur when an exception happens and
> the
> process terminates before the device driver stops DMA and calls the
> iommu driver to unbind the PASID.
> 
> There's no need to drain the PRQ in the mm release path. Instead, the PRQ
> will be drained in the SVA unbind path.
> 
> Unfortunately, commit c43e1ccdebf2 ("iommu/vt-d: Drain PRQs when
> domain
> removed from RID") changed this behavior by unconditionally draining the
> PRQ in intel_pasid_tear_down_entry(). This can lead to a potential
> sleeping-in-atomic-context issue.
> 
> Smatch static checker warning:
> 
> 	drivers/iommu/intel/prq.c:95 intel_iommu_drain_pasid_prq()
> 	warn: sleeping in atomic context
> 
> To avoid this issue, prevent draining the PRQ in the SVA mm release path
> and restore the previous behavior.
> 
> Fixes: c43e1ccdebf2 ("iommu/vt-d: Drain PRQs when domain removed from
> RID")
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-
> 4a279dc88b49@stanley.mountain/
> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
>  drivers/iommu/intel/pasid.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 0f2a926d3bd5..5b7d85f1e143 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -265,7 +265,8 @@ void intel_pasid_tear_down_entry(struct
> intel_iommu *iommu, struct device *dev,
>  		iommu->flush.flush_iotlb(iommu, did, 0, 0,
> DMA_TLB_DSI_FLUSH);
> 
>  	devtlb_invalidation_with_pasid(iommu, dev, pasid);
> -	intel_iommu_drain_pasid_prq(dev, pasid);
> +	if (!fault_ignore)
> +		intel_iommu_drain_pasid_prq(dev, pasid);
>  }
> 

As a regression fix:

Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>

But I doubt whether it's working as expected. According to the
description and code, intel_pasid_tear_down_entry() is called
twice: the 1st in the mm release path and the 2nd in the unbind
path. PRQ draining is skipped in the former.

But intel_pasid_tear_down_entry() has a check at the beginning:

	pte = intel_pasid_get_entry(dev, pasid);
	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
		spin_unlock(&iommu->lock);
		return;
	}

The 1st invocation already clears the pasid entry with FPD set.

Then the 2nd invocation will return early instead of moving to
the point of PRQ draining.

Did I overlook anything?