Re: [PATH v5 2/3] iommu/vt-d: debugfs: Create/remove debugfs file per {device, pasid}

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

On 11/13/24 05:22, Kees Bakker wrote:
> Op 13-10-2023 om 15:58 schreef Jingqi Liu:
>> Add a debugfs directory per pair of {device, pasid} if the mappings of
>> its page table are created and destroyed by the iommu_map/unmap()
>> interfaces. i.e. /sys/kernel/debug/iommu/intel/<device source id>/ 
>> <pasid>.
>> Create a debugfs file in the directory for users to dump the page
>> table corresponding to {device, pasid}. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:02.0/1/domain_translation_struct.
>> For the default domain without pasid, it creates a debugfs file in the
>> debugfs device directory for users to dump its page table. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:02.0/domain_translation_struct.
>>
>> When setting a domain to a PASID of device, create a debugfs file in
>> the pasid debugfs directory for users to dump the page table of the
>> specified pasid. Remove the debugfs device directory of the device
>> when releasing a device. e.g.
>> /sys/kernel/debug/iommu/intel/0000:00:01.0
>>
>> Signed-off-by: Jingqi Liu <Jingqi.liu@intel.com>
>> ---
>>   drivers/iommu/intel/debugfs.c | 53 +++++++++++++++++++++++++++++++----
>>   drivers/iommu/intel/iommu.c   |  7 +++++
>>   drivers/iommu/intel/iommu.h   | 14 +++++++++
>>   3 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/intel/debugfs.c b/drivers/iommu/intel/ 
>> debugfs.c
>> [...]
>> +/* Remove the device pasid debugfs directory. */
>> +void intel_iommu_debugfs_remove_dev_pasid(struct dev_pasid_info 
>> *dev_pasid)
>> +{
>> +    debugfs_remove_recursive(dev_pasid->debugfs_dentry);
>> +}
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> [...]
>> @@ -4710,6 +4713,7 @@ static void intel_iommu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid)
>>       spin_unlock_irqrestore(&dmar_domain->lock, flags);
>>       domain_detach_iommu(dmar_domain, iommu);
>> +    intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>>       kfree(dev_pasid);
>>   out_tear_down:
>>       intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>>
> 
> So, a call to intel_iommu_debugfs_remove_dev_pasid() was added.
> There is a potential problem that dev_pasid can be NULL.
> The diff doesn't show the whole context so let me give that here.
> Today that piece of the code looks like this
> 
>          list_for_each_entry(curr, &dmar_domain->dev_pasids, link_domain) {
>                  if (curr->dev == dev && curr->pasid == pasid) {
>                          list_del(&curr->link_domain);
>                          dev_pasid = curr;
>                          break;
>                  }
>          }
>          WARN_ON_ONCE(!dev_pasid);
>          spin_unlock_irqrestore(&dmar_domain->lock, flags);
> 
>          cache_tag_unassign_domain(dmar_domain, dev, pasid);
>          domain_detach_iommu(dmar_domain, iommu);
>          intel_iommu_debugfs_remove_dev_pasid(dev_pasid);
>          kfree(dev_pasid);
> 
> The for_each loop can exit without finding an entry.
> The WARN_ON_ONCE also suggests that there can be a NULL.
> After that the new function is called (see above) and it will
> dereference the NULL pointer.
> 
> Can you have a closer look?

It's already a kernel bug if dev_pasid is NULL when it reaches here. If
that happens, we should fix the bug, not avoid using the NULL pointer.

--
baolu