netdev - Re: [PATCH v2 04/10] iommu/dma: Use the gfp parameter in __iommu_dma_alloc

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: <Y8r7ujD8BVgWiIH/@nvidia.com>
Date:   Fri, 20 Jan 2023 16:38:18 -0400
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        Kevin Tian <kevin.tian@...el.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        ath10k@...ts.infradead.org, ath11k@...ts.infradead.org,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        dri-devel@...ts.freedesktop.org, iommu@...ts.linux.dev,
        kvm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-remoteproc@...r.kernel.org,
        linux-s390@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-tegra@...r.kernel.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, nouveau@...ts.freedesktop.org,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v2 04/10] iommu/dma: Use the gfp parameter in
 __iommu_dma_alloc_noncontiguous()

On Fri, Jan 20, 2023 at 07:28:19PM +0000, Robin Murphy wrote:
> On 2023-01-18 18:00, Jason Gunthorpe wrote:
> > Change the sg_alloc_table_from_pages() allocation that was hardwired to
> > GFP_KERNEL to use the gfp parameter like the other allocations in this
> > function.
> > 
> > Auditing says this is never called from an atomic context, so it is safe
> > as is, but reads wrong.
> 
> I think the point may have been that the sgtable metadata is a
> logically-distinct allocation from the buffer pages themselves. Much like
> the allocation of the pages array itself further down in
> __iommu_dma_alloc_pages().

That makes sense, and it is a good reason to mask off the allocation
policy flags from the gfp.

On the other hand it also makes sense to continue to pass in things
like NOWAIT|NOWARN to all the allocations. Even to the iommu driver.

So I'd prefer to change this to mask and make all the following calls
consistently use the input gfp

> I'd say the more confusing thing about this particular context is why we're
> using iommu_map_sg_atomic() further down - that seems to have been an
> oversight in 781ca2de89ba, since this particular path has never supported
> being called in atomic context.

Huh. I had fixed that in v1, this patch was supposed to have that
hunk, that was the main point of making this patch actually..

> Overall I'm starting to wonder if it might not be better to stick a "use
> GFP_KERNEL_ACCOUNT if you allocate" flag in the domain for any level of the
> API internals to pick up as appropriate, rather than propagate per-call gfp
> flags everywhere. 

We might get to something like that, but it requires more parts that
are not ready yet. Most likely this would take the form of some kind
of 'this is an iommufd created domain' indication. This happens
naturally as part of the nesting patches.

Right now I want to get people to start testing with this because the
charge from the IOPTEs is far and away the largest memory draw.  Parts
like fixing the iommu drivers to actually use gfp are necessary to
make it work.

If we flip the two places using KERNEL_ACCOUNT to something else later
it doesn't really matter. I think the removal of the two _atomic
wrappers is still appropriate stand-alone.

> As it stands we're still missing potential pagetable and other
> domain-related allocations by drivers in .attach_dev and even (in

Yes, I plan to get to those when we add an alloc_domain_iommufd() or
whatever op. The driver will know the calling context and can set the
gfp flags for any allocations under alloc_domain under that time.

Then we can go and figure out if there are other allocations and if
all or only some drivers need a flag - eg at attach time. Though this
is less worrying because you can only scale attach up to num_pasids *
num open vfios.

iommufd will let userspace create and populate an unlimited number of
iommu_domains, so everything linked to an unattached iommu_domain
should be charged.

> probably-shouldn't-really-happen cases) .unmap_pages...

Gah, unmap_pages isn't allow to fail. There is no way to recover from
this. iommufd will spew a warn and then have a small race where
userspace can UAF kernel memory.

I'd call such a driver implementation broken. Why would you need to do
this?? :(

Thanks,
Jason