linux-kernel - Re: [PATCH v2 5/8] iommu: Switch __iommu_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: <20230127083245.1a0bc8e1@jacob-builder>
Date:   Fri, 27 Jan 2023 08:32:45 -0800
From:   Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     joro@...tes.org, will@...nel.org, iommu@...ts.linux.dev,
        linux-kernel@...r.kernel.org, hch@....de, jgg@...dia.com,
        baolu.lu@...ux.intel.com, jacob.jun.pan@...ux.intel.com
Subject: Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device
 ops

Hi Robin,

On Fri, 27 Jan 2023 11:42:27 +0000, Robin Murphy <robin.murphy@....com>
wrote:

> On 2023-01-26 23:22, Jacob Pan wrote:
> > Hi Robin,
> > 
> > On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@....com>
> > wrote:
> >   
> >>   
> >> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> >> +{
> >> +	struct device **alloc_dev = data;
> >> +
> >> +	if (!dev_iommu_ops_valid(dev))
> >> +		return 0;
> >> +
> >> +	WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) !=
> >> dev_iommu_ops(*alloc_dev),
> >> +		"Multiple IOMMU drivers present, which the public
> >> IOMMU API can't fully support yet. You may still need to disable one
> >> or more to get the expected result here, sorry!\n"); +
> >> +	*alloc_dev = dev;
> >> +	return 0;
> >> +}
> >> +
> >>   struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
> >>   {
> >> -	return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> >> +	struct device *dev = NULL;
> >> +
> >> +	/* We always check the whole bus, so the return value isn't
> >> useful */
> >> +	bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
> >> +	if (!dev)
> >> +		return NULL;  
> > Since __iommu_domain_alloc_dev() will always return 0,
> > bus_for_each_dev() will never breakout until the whole dev list is
> > iterated over. If so, would dev only record the last one? i.e. prior
> > results get overwritten.  Maybe a misunderstood the logic.  
> 
> Yes, as the comment points out, the intent is to walk the whole bus to 
> check it for consistency. Beyond that, we just need *a* device with 
> IOMMU ops; it doesn't matter at all which one it is. It happens to be 
> the last one off the list because that's what fell out of writing the 
> fewest lines of code.
> 
> (You could argue that there's no need to repeat the full walk if the 
> WARN_ONCE has already fired, but I'd rather keep the behaviour simple 
> and consistent - this is only meant to be a short-term solution, and 
> it's not a performance-critical path)
That make sense now, thank you for the explanation.


Jacob