linux-kernel - Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity domain

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9aa9e93e-915e-4c6d-9adb-b7d5facdf3bc@linux.intel.com>
Date: Wed, 7 Aug 2024 14:41:39 +0800
From: Baolu Lu <baolu.lu@...ux.intel.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: baolu.lu@...ux.intel.com, Joerg Roedel <joro@...tes.org>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 Kevin Tian <kevin.tian@...el.com>, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/7] iommu/vt-d: Prepare for global static identity
 domain

On 2024/8/7 1:12, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 10:39:38AM +0800, Lu Baolu wrote:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index c019fb3b3e78..f37c8c3cba3c 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -1270,6 +1270,9 @@ void domain_update_iotlb(struct dmar_domain *domain)
>>   	bool has_iotlb_device = false;
>>   	unsigned long flags;
>>   
>> +	if (!domain)
>> +		return;
>> +
> This seems really strange, maybe wrong..
> 
> The only callers that could take advantage are
> iommu_enable_pci_caps()/iommu_disable_pci_caps()

Yes.

When the PCI ATS status changes, the domain attached to the device
should have its domain->has_iotlb_device flag updated.

The global static identity domain is a dummy domain without a
corresponding dmar_domain structure. Consequently, the device's
info->domain will be NULL. This is why a check is necessary.

I might need to make this check explicit with an additional change.

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f37c8c3cba3c..d59e9ac223ba 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1270,9 +1270,6 @@ void domain_update_iotlb(struct dmar_domain *domain)
         bool has_iotlb_device = false;
         unsigned long flags;

-       if (!domain)
-               return;
-
         spin_lock_irqsave(&domain->lock, flags);
         list_for_each_entry(info, &domain->devices, link) {
                 if (info->ats_enabled) {
@@ -1330,7 +1327,8 @@ static void iommu_enable_pci_caps(struct 
device_domain_info *info)
         if (info->ats_supported && pci_ats_page_aligned(pdev) &&
             !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
                 info->ats_enabled = 1;
-               domain_update_iotlb(info->domain);
+               if (info->domain)
+                       domain_update_iotlb(info->domain);
         }
  }

@@ -1346,7 +1344,8 @@ static void iommu_disable_pci_caps(struct 
device_domain_info *info)
         if (info->ats_enabled) {
                 pci_disable_ats(pdev);
                 info->ats_enabled = 0;
-               domain_update_iotlb(info->domain);
+               if (info->domain)
+                       domain_update_iotlb(info->domain);
         }

         if (info->pasid_enabled) {

> 
> But if they are mucking with ATS then the ATC flushes should not be
> done wrong!
> 
> So I looked at this and, uh, who even reads domain->has_iotlb_device ?

The has_iotlb_device flag indicates whether a domain is attached to
devices with ATS enabled. If a domain lacks this flag, no device TBLs
need to be invalidated during unmap operations. This optimization avoids
unnecessary looping through all attached devices.

> So I'd just delete  domain->has_iotlb_device and domain_update_iotlb()
> as well.

Thanks,
baolu