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: <4d273959-5925-4df5-929a-88cd5c31748a@linux.intel.com>
Date: Wed, 7 Aug 2024 21:44:42 +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 20:17, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 02:41:39PM +0800, Baolu Lu wrote:
>> 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 get it, but you can't have ATS turned on at all if you can push the
> invalidations. So it seems like something is missing to enforce that
> with the identity domains.
> 
>>> 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.
> Not any more, that was removed in commit 06792d067989 ("iommu/vt-d:
> Cleanup use of iommu_flush_iotlb_psi()")

Yeah! How stupid I was! It's actually a dead code after the
implementation of cache tags.

> 
> This compiles, so you should do this instead:

Yes. I will cleanup this in a separated patch.

Thanks,
baolu