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

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

[Lu Baolu]

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@linaro.org>
Closes: https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-4a279dc88b49@stanley.mountain/
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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);
 }
 
 /*
-- 
2.43.0

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

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> 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@linaro.org>
> Closes: https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-
> 4a279dc88b49@stanley.mountain/
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  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@intel.com>

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?

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

[Baolu Lu]

On 2024/12/12 14:18, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> 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@linaro.org>
>> Closes:https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-
>> 4a279dc88b49@stanley.mountain/
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   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@intel.com>
> 
> 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.

You are right.

We need to clear the Fault Processing Disabled bit in the unbind path if
it is set and drain the PRQ. What about below code?

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 5b7d85f1e143..45bd1b689674 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -244,11 +244,25 @@ void intel_pasid_tear_down_entry(struct 
intel_iommu *iommu, struct device *dev,

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

+	if (!pasid_pte_is_present(pte)) {
+		if (!pasid_pte_is_fault_disabled(pte)) {
+			WARN_ON(READ_ONCE(pte->val[0]) != 0);
+			spin_unlock(&iommu->lock);
+			return;
+		}
+
+		pasid_clear_entry(pe);
+		spin_unlock(&iommu->lock);
+		intel_iommu_drain_pasid_prq(dev, pasid);
+
+		return;
+	}
+
  	did = pasid_get_domain_id(pte);
  	pgtt = pasid_pte_get_pgtt(pte);
  	intel_pasid_clear_entry(dev, pasid, fault_ignore);
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 082f4fe20216..668d8ece6b14 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -73,6 +73,12 @@ static inline bool pasid_pte_is_present(struct 
pasid_entry *pte)
  	return READ_ONCE(pte->val[0]) & PASID_PTE_PRESENT;
  }

+/* Get FPD(Fault Processing Disable) bit of a PASID table entry */
+static inline bool pasid_pte_is_fault_disabled(struct pasid_entry *pte)
+{
+	return READ_ONCE(pte->val[0]) & PASID_PTE_FPD;
+}
+
  /* Get PGTT field of a PASID table entry */
  static inline u16 pasid_pte_get_pgtt(struct pasid_entry *pte)
  {

Thanks,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, December 12, 2024 2:58 PM
> 
> On 2024/12/12 14:18, Tian, Kevin wrote:
> >> From: Lu Baolu<baolu.lu@linux.intel.com>
> >> 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@linaro.org>
> >> Closes:https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-
> >> 4a279dc88b49@stanley.mountain/
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   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@intel.com>
> >
> > 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.
> 
> You are right.
> 
> We need to clear the Fault Processing Disabled bit in the unbind path if
> it is set and drain the PRQ. What about below code?
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 5b7d85f1e143..45bd1b689674 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -244,11 +244,25 @@ void intel_pasid_tear_down_entry(struct
> intel_iommu *iommu, struct device *dev,
> 
>   	spin_lock(&iommu->lock);
>   	pte = intel_pasid_get_entry(dev, pasid);
> -	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
> +	if (WARN_ON(!pte)) {
>   		spin_unlock(&iommu->lock);
>   		return;
>   	}
> 
> +	if (!pasid_pte_is_present(pte)) {

add a comment here. otherwise looks good

> +		if (!pasid_pte_is_fault_disabled(pte)) {
> +			WARN_ON(READ_ONCE(pte->val[0]) != 0);
> +			spin_unlock(&iommu->lock);
> +			return;
> +		}
> +
> +		pasid_clear_entry(pe);
> +		spin_unlock(&iommu->lock);
> +		intel_iommu_drain_pasid_prq(dev, pasid);
> +
> +		return;
> +	}
> +
>   	did = pasid_get_domain_id(pte);
>   	pgtt = pasid_pte_get_pgtt(pte);
>   	intel_pasid_clear_entry(dev, pasid, fault_ignore);
> diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
> index 082f4fe20216..668d8ece6b14 100644
> --- a/drivers/iommu/intel/pasid.h
> +++ b/drivers/iommu/intel/pasid.h
> @@ -73,6 +73,12 @@ static inline bool pasid_pte_is_present(struct
> pasid_entry *pte)
>   	return READ_ONCE(pte->val[0]) & PASID_PTE_PRESENT;
>   }
> 
> +/* Get FPD(Fault Processing Disable) bit of a PASID table entry */
> +static inline bool pasid_pte_is_fault_disabled(struct pasid_entry *pte)
> +{
> +	return READ_ONCE(pte->val[0]) & PASID_PTE_FPD;
> +}
> +
>   /* Get PGTT field of a PASID table entry */
>   static inline u16 pasid_pte_get_pgtt(struct pasid_entry *pte)
>   {
> 
> Thanks,
> baolu