linux-kernel - Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init

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]

Date:   Mon, 9 Aug 2021 15:47:32 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Will Deacon <will@...nel.org>
Cc:     joro@...tes.org, linux-kernel@...r.kernel.org,
        dianders@...omium.org, iommu@...ts.linux-foundation.org,
        rajatja@...gle.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 24/25] iommu/dma: Factor out flush queue init

On 2021-08-09 13:52, Will Deacon wrote:
> On Wed, Aug 04, 2021 at 06:15:52PM +0100, Robin Murphy wrote:
>> Factor out flush queue setup from the initial domain init so that we
>> can potentially trigger it from sysfs later on in a domain's lifetime.
>>
>> Reviewed-by: Lu Baolu <baolu.lu@...ux.intel.com>
>> Reviewed-by: John Garry <john.garry@...wei.com>
>> Signed-off-by: Robin Murphy <robin.murphy@....com>
>> ---
>>   drivers/iommu/dma-iommu.c | 30 ++++++++++++++++++++----------
>>   include/linux/dma-iommu.h |  9 ++++++---
>>   2 files changed, 26 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 2e19505dddf9..f51b8dc99ac6 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -310,6 +310,25 @@ static bool dev_is_untrusted(struct device *dev)
>>   	return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
>>   }
>>   
>> +int iommu_dma_init_fq(struct iommu_domain *domain)
>> +{
>> +	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>> +
>> +	if (domain->type != IOMMU_DOMAIN_DMA_FQ)
>> +		return -EINVAL;
>> +	if (cookie->fq_domain)
>> +		return 0;
>> +
>> +	if (init_iova_flush_queue(&cookie->iovad, iommu_dma_flush_iotlb_all,
>> +				  iommu_dma_entry_dtor)) {
>> +		pr_warn("iova flush queue initialization failed\n");
>> +		domain->type = IOMMU_DOMAIN_DMA;
>> +		return -ENODEV;
> 
> I do find this a bit odd: we assert that the caller has set domain->type
> to IOMMU_DOMAIN_DMA_FQ but then on failure we reset it to IOMMU_DOMAIN_DMA
> here. I think it would be less error-prone if the setting of domain->type
> was handled in the same function.

On reflection I think I agree. For some reason I settled on the idea of 
doing this to make the callers simpler, but it turns out that unpicking 
it to flow logically is in fact a +4/-5 diff essentially just moving all 
the same statements to different places, and that's before I update 
comments since that theoretical race between the sysfs and DMA ops paths 
only exists because of sysfs having to dance around the type check here...

I'll send v4 later today or possibly tomorrow, but not in such a hurry 
that I skimp on the build-testing this time!

Cheers,
Robin.