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: <389082e3-f4c3-4e84-a2d0-629612eed305@arm.com>
Date:   Fri, 27 Jan 2023 11:42:27 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Jacob Pan <jacob.jun.pan@...ux.intel.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
Subject: Re: [PATCH v2 5/8] iommu: Switch __iommu_domain_alloc() to device ops

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)

Thanks,
Robin.

> 
>> +	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
> 
> Thanks,
> 
> Jacob