linux-kernel - Re: [PATCH v4 1/2] iommu/s390: Fix race with release

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: <31269d99-5ffc-7060-2af2-ce1f5bc33de2@arm.com>
Date:   Thu, 1 Sep 2022 18:00:51 +0100
From:   Robin Murphy <robin.murphy@....com>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Niklas Schnelle <schnelle@...ux.ibm.com>,
        Pierre Morel <pmorel@...ux.ibm.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>, iommu@...ts.linux.dev,
        linux-s390@...r.kernel.org, borntraeger@...ux.ibm.com,
        hca@...ux.ibm.com, gor@...ux.ibm.com,
        gerald.schaefer@...ux.ibm.com, agordeev@...ux.ibm.com,
        svens@...ux.ibm.com, joro@...tes.org, will@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/2] iommu/s390: Fix race with release_device ops

On 2022-09-01 16:49, Jason Gunthorpe wrote:
> On Thu, Sep 01, 2022 at 04:03:36PM +0100, Robin Murphy wrote:
>> On 2022-09-01 15:34, Jason Gunthorpe wrote:
>>> On Thu, Sep 01, 2022 at 03:29:16PM +0100, Robin Murphy wrote:
>>>
>>>> Right, the next step would be to bridge that gap to iommu-dma to dump the
>>>> flush queue when IOVA allocation failure implies we've reached the
>>>> "rollover" point, and perhaps not use the timer at all. By that point a
>>>> dedicated domain type, or at least some definite internal flag, for this
>>>> alternate behaviour seems like the logical way to go.
>>>
>>> At least on this direction, I've been thinking it would be nice to
>>> replace the domain type _FQ with a flag inside the domain, maybe the
>>> ops, saying how the domain wants the common DMA API to operate. If it
>>> wants FQ mode or other tuning parameters
>>
>> Compare the not-necessarily-obvious matrix of "strict" and "passthrough"
>> command-line parameters with the nice understandable kconfig and sysfs
>> controls for a reminder of why I moved things *from* that paradigm in the
>> first place ;)
> 
> I'm looking at it from a code perspective, where the drivers don't
> seem to actually care about DMA_FQ. eg search for it in the drivers
> and you mostly see:
> 
> 	    (type != IOMMU_DOMAIN_DMA && type != IOMMU_DOMAIN_DMA_FQ))
> 
> The exception is domain_alloc which fails in cases where the domain
> doesn't 'support' FQ.
> 
> But support FQ or not can be cast as a simple capability flag in the
> domain. We don't need a whole type for the driver to communicate it.

Right, since the rest of DMA domain setup got streamlined into the core 
code this one remaining vestige of the old world order is ripe for 
cleanup, I've just had bigger fish to fry. Or as the case may be, bigger 
chunks of repetitive boilerplate to remove from elsewhere in the drivers :)

> The strictness level belongs completely in the core code, it shouldn't
> leak into the driver.

It's already not about drivers having any influence on strictness, it's 
about there being good reason to treat these as distinct domain types 
through the core code, and as things stand those domain types are 
exposed to drivers, so drivers need to not freak out at seeing them. 
Indeed Any driver can "support" IOMMU_DOMAIN_DMA now, so if you've got 
time to come up with a way of making that completely transparent to 
drivers then please go ahead, though IIRC there were one or two cases 
where folks explicitly *didn't* want it being used, so those might need 
proper IOMMU_DOMAIN_IDENTITY support and a def_domain_type override adding.

The thing with IOMMU_DOMAIN_DMA_FQ is that drivers *do* need to somehow 
indicate that they implement the relevant optimisations in their unmap 
path, otherwise using a flush queue is objectively worse in every 
respect than not using a flush queue. Given the status quo, rejecting 
the domain type at allocation was by far the simplest and most obvious 
way to achieve that. Once again, please do propose moving it to a more 
explicit "I can support deferred unmap" driver capability if you'd like, 
otherwise I'll get there eventually.

Thanks,
Robin.