netdev - Re: [PATCH 3/5] iommu: Introduce the domain op enforce_cache

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: <20220405225739.GW2120790@nvidia.com>
Date:   Tue, 5 Apr 2022 19:57:39 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Alex Williamson <alex.williamson@...hat.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Lu Baolu <baolu.lu@...ux.intel.com>
Cc:     Christian Benvenuti <benve@...co.com>,
        Cornelia Huck <cohuck@...hat.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Gerald Schaefer <gerald.schaefer@...ux.ibm.com>,
        iommu@...ts.linux-foundation.org, Jason Wang <jasowang@...hat.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-rdma@...r.kernel.org,
        linux-s390@...r.kernel.org,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Nelson Escobar <neescoba@...co.com>, netdev@...r.kernel.org,
        Rob Clark <robdclark@...il.com>,
        Robin Murphy <robin.murphy@....com>,
        Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
        virtualization@...ts.linux-foundation.org,
        Will Deacon <will@...nel.org>, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 3/5] iommu: Introduce the domain op
 enforce_cache_coherency()

On Tue, Apr 05, 2022 at 01:50:36PM -0600, Alex Williamson wrote:
> >  
> > +static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
> > +{
> > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +
> > +	if (!dmar_domain->iommu_snooping)
> > +		return false;
> > +	dmar_domain->enforce_no_snoop = true;
> > +	return true;
> > +}
> 
> Don't we have issues if we try to set DMA_PTE_SNP on DMARs that don't
> support it, ie. reserved register bit set in pte faults?  

The way the Intel driver is setup that is not possible. Currently it
does:

 static bool intel_iommu_capable(enum iommu_cap cap)
 {
	if (cap == IOMMU_CAP_CACHE_COHERENCY)
		return domain_update_iommu_snooping(NULL);

Which is a global property unrelated to any device.

Thus either all devices and all domains support iommu_snooping, or
none do.

It is unclear because for some reason the driver recalculates this
almost constant value on every device attach..

> There's also a disconnect, maybe just in the naming or documentation,
> but if I call enforce_cache_coherency for a domain, that seems like the
> domain should retain those semantics regardless of how it's
> modified,

Right, this is how I would expect it to work.

> ie. "enforced".  For example, if I tried to perform the above operation,
> I should get a failure attaching the device that brings in the less
> capable DMAR because the domain has been set to enforce this
> feature.

We don't have any code causing a failure like this because no driver
needs it.

> Maybe this should be something like set_no_snoop_squashing with the
> above semantics, it needs to be re-applied whenever the domain:device
> composition changes?  Thanks,

If we get a real driver that needs non-uniformity here we can revisit
what to do. There are a couple of good options depending on exactly
what the HW behavior is.

Is it more clear if I fold in the below? It helps show that the
decision to use DMA_PTE_SNP is a global choice based on
domain_update_iommu_snooping():

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e5062461ab0640..fc789a9d955645 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -641,7 +641,6 @@ static unsigned long domain_super_pgsize_bitmap(struct dmar_domain *domain)
 static void domain_update_iommu_cap(struct dmar_domain *domain)
 {
 	domain_update_iommu_coherency(domain);
-	domain->iommu_snooping = domain_update_iommu_snooping(NULL);
 	domain->iommu_superpage = domain_update_iommu_superpage(domain, NULL);
 
 	/*
@@ -4283,7 +4282,6 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 	domain->agaw = width_to_agaw(adjust_width);
 
 	domain->iommu_coherency = false;
-	domain->iommu_snooping = false;
 	domain->iommu_superpage = 0;
 	domain->max_addr = 0;
 
@@ -4549,7 +4547,7 @@ static bool intel_iommu_enforce_cache_coherency(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
 
-	if (!dmar_domain->iommu_snooping)
+	if (!domain_update_iommu_snooping(NULL))
 		return false;
 	dmar_domain->enforce_no_snoop = true;
 	return true;
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 1f930c0c225d94..bc39f633efdf03 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -539,7 +539,6 @@ struct dmar_domain {
 
 	u8 has_iotlb_device: 1;
 	u8 iommu_coherency: 1;		/* indicate coherency of iommu access */
-	u8 iommu_snooping: 1;		/* indicate snooping control feature */
 	u8 enforce_no_snoop : 1;        /* Create IOPTEs with snoop control */
 
 	struct list_head devices;	/* all devices' list */