Re: [PATCH v3 3/9] iommu/vt-d: Let intel_pasid_tear_down_entry() return pasid entry

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/10/21 14:35, Yi Liu wrote:
> On 2024/10/21 14:13, Baolu Lu wrote:
>> On 2024/10/18 13:53, Yi Liu wrote:
>>> intel_pasid_tear_down_entry() finds the pasid entry and tears it down.
>>> There are paths that need to get the pasid entry, tear it down and
>>> re-configure it. Letting intel_pasid_tear_down_entry() return the pasid
>>> entry can avoid duplicate codes to get the pasid entry. No functional
>>> change is intended.
>>>
>>> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
>>> ---
>>>   drivers/iommu/intel/pasid.c | 11 ++++++++---
>>>   drivers/iommu/intel/pasid.h |  5 +++--
>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>> index 2898e7af2cf4..336f9425214c 100644
>>> --- a/drivers/iommu/intel/pasid.c
>>> +++ b/drivers/iommu/intel/pasid.c
>>> @@ -239,9 +239,12 @@ devtlb_invalidation_with_pasid(struct 
>>> intel_iommu *iommu,
>>>   /*
>>>    * Caller can request to drain PRQ in this helper if it hasn't done 
>>> so,
>>>    * e.g. in a path which doesn't follow remove_dev_pasid().
>>> + * Return the pasid entry pointer if the entry is found or NULL if no
>>> + * entry found.
>>>    */
>>> -void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct 
>>> device *dev,
>>> -                 u32 pasid, u32 flags)
>>> +struct pasid_entry *
>>> +intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device 
>>> *dev,
>>> +                u32 pasid, u32 flags)
>>>   {
>>>       struct pasid_entry *pte;
>>>       u16 did, pgtt;
>>> @@ -250,7 +253,7 @@ void intel_pasid_tear_down_entry(struct 
>>> intel_iommu *iommu, struct device *dev,
>>>       pte = intel_pasid_get_entry(dev, pasid);
>>>       if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
>>>           spin_unlock(&iommu->lock);
>>> -        return;
>>> +        goto out;
>>
>> The pasid table entry is protected by iommu->lock. It's  not reasonable
>> to return the pte pointer which is beyond the lock protected range.
> 
> Per my understanding, the iommu->lock protects the content of the entry,
> so the modifications to the entry need to hold it. While, it looks not
> necessary to protect the pasid entry pointer itself. The pasid table should
> exist during device probe and release. is it?

The pattern of the code that modifies a pasid table entry is,

	spin_lock(&iommu->lock);
	pte = intel_pasid_get_entry(dev, pasid);
	... modify the pasid table entry ...
	spin_unlock(&iommu->lock);

Returning the pte pointer to the caller introduces a potential race
condition. If the caller subsequently modifies the pte without re-
acquiring the spin lock, there's a risk of data corruption or
inconsistencies.

Thanks,
baolu