linux-kernel - Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from 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: <20220523110158.3382b5fd@jacob-builder>
Date:   Mon, 23 May 2022 11:01:58 -0700
From:   Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>
Cc:     "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Jean-Philippe Brucker <jean-philippe@...aro.com>,
        "Lu Baolu" <baolu.lu@...ux.intel.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        "vkoul@...nel.org" <vkoul@...nel.org>,
        "robin.murphy@....com" <robin.murphy@....com>,
        "will@...nel.org" <will@...nel.org>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        Eric Auger <eric.auger@...hat.com>,
        jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from
 domain

Hi Kevin,

On Mon, 23 May 2022 09:14:04 +0000, "Tian, Kevin" <kevin.tian@...el.com>
wrote:

> > From: Tian, Kevin
> > Sent: Monday, May 23, 2022 3:55 PM
> >   
> > > From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
> > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > > iommu_domain *domain)
> > > +{
> > > +	struct iommu_domain *tdomain;
> > > +	struct iommu_group *group;
> > > +	unsigned long index;
> > > +	ioasid_t pasid = INVALID_IOASID;
> > > +
> > > +	group = iommu_group_get(dev);
> > > +	if (!group)
> > > +		return pasid;
> > > +
> > > +	xa_for_each(&group->pasid_array, index, tdomain) {
> > > +		if (domain == tdomain) {
> > > +			pasid = index;
> > > +			break;
> > > +		}
> > > +	}  
> > 
> > Don't we need to acquire the group lock here?
> > 
pasid_array is under RCU read lock so it is protected though may have stale
data. It also used in atomic context for TLB flush, cannot take the
group mutex. If the caller does detach_dev_pasid while doing TLB flush, it
could result in extra flush but harmless.

> > Btw the intention of this function is a bit confusing. Patch01 already
> > stores the pasid under domain hence it's redundant to get it
> > indirectly from xarray index. You could simply introduce a flag bit
> > (e.g. dma_pasid_enabled) in device_domain_info and then directly
> > use domain->dma_pasid once the flag is true.
> >   
> 
> Just saw your discussion with Jason about v3. While it makes sense
> to not specialize DMA domain in iommu driver, the use of this function
> should only be that when the call chain doesn't pass down a pasid
> value e.g. when doing cache invalidation for domain map/unmap. If
> the upper interface already carries a pasid e.g. in detach_dev_pasid()
> iommu driver can simply verify that the corresponding pasid xarray 
> entry points to the specified domain instead of using this function to
> loop xarray and then verify the returned pasid (as done in patch03/04).
Excellent point, I could just use xa_load(pasid) to compare the domain
instead of loop through xa.
I will add another helper.

bool iommu_is_pasid_domain_attached(struct device *dev, struct iommu_domain *domain, ioasid_t pasid)
{
	struct iommu_group *group;
	bool ret = false;

	group = iommu_group_get(dev);
	if (WARN_ON(!group))
		return false;

	if (domain == xa_load(&group->pasid_array, pasid))
		ret = true;

	iommu_group_put(group);

	return ret;
}

Thanks,

Jacob