Message-ID: <20250306160907.GF354511@nvidia.com>
Date: Thu, 6 Mar 2025 12:09:07 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>,
	Abdiel Janulgue <abdiel.janulgue@...il.com>, aliceryhl@...gle.com,
	robin.murphy@....com, daniel.almeida@...labora.com,
	rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>,
	Valentin Obst <kernel@...entinobst.de>,
	open list <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>,
	Marek Szyprowski <m.szyprowski@...sung.com>, airlied@...hat.com,
	"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

On Thu, Mar 06, 2025 at 04:21:51PM +0100, Simona Vetter wrote:
> > >  > a device with no driver bound should not be passed to the DMA API,
> > >  > much less a dead device that's already been removed from its parent
> > >  > bus.
> > 
> > Thanks for bringing this up!
> > 
> > I assume that's because of potential iommu mappings, the memory itself should
> > not be critical.

There is a lot of state tied to the struct device lifecycle that the
DMA API and iommu implicitly manages. It is not just iommu mappings.

It is incorrect to view the struct device as a simple refcount object
where holding the refcount means it is alive and safe to use. There
are three broad substates (No Driver, Driver Attached, Zombie) that
the struct device can be in that are relevant.

Technically it is unsafe and oopsable to call the allocation API as
well on a device that has no driver. This issue is also ignored in
these bindings and cannot be solved with revoke.

IOW I do not belive you can create bindings here that are truely safe
without also teaching rust to understand the concept of a scope
guaranteed to be within a probed driver's lifetime.

> > > Also note that any HW configured to do DMA must be halted before the
> > > free is allowed otherwise it is a UAF bug. It is worth mentioning that
> > > in the documentation.
> > 
> > Agreed, makes sense to document. For embedding the CoherentAllocation into
> > Devres this shouldn't be an issue, since a driver must stop operating the device
> > in remove() by definition.
>
> I think for basic driver allocations that you just need to run the device
> stuffing it all into devres is ok. 

What exactly will this revokable critical region protect?

The actual critical region extends into the HW itself, it is not
simple to model this with a pure SW construct of bracketing some
allocation. You need to bracket the *entire lifecycle* of the
dma_addr_t that has been returned and passed into HW, until the
dma_addr_t is removed from HW.

You cannot exit your critical region until the HW has ended it's DMA.

Is that possible? I think not, as least not as you imagine it with a
non-sleepable RCU.

Further forcing driver writers to make critical regions that do not
bracket the lifetime of the actual thing being protected (the
dma_addr_t) is a complete farce, IMHO.

All this does is confuse people about what is actually required to
write a correct driver and increases the chance of incorrect removal
sequencing.

My dislike of revoke has only increased as this discussion has gone
on, I think it should not be part of the generic Rust bidings at
all.

> But for dma mappings at runtime this will be too slow, so I guess
> we'll need subsystem specific abstractions which guarantee that all
> dma-api mappings have disappared when device removal finishes.

Yes, it will be way too slow.

I don't know about "subsystem level abstractions". That seems to be a
very big and invasive ask.

> sections. Similar for any other subsytem that shovely substantial amounts
> of data around. For some this might already be solved entirely at the C
> level, if the subsystem already tracks all buffers allocated to a device
> (media might work like that at least if you use videobuf helpers, but not
> sure).
> 
> So lots of good fun here, but I not unsurmountable.

I disagree. I think this is unsurmountable. The idea of fine grained
revoke is dead in my mind. If DRM really wants to attempt it, I think
that should be DRM's mess to figure out alone.

Revoke should not be part of the generic rust bindings and this idea
should not be exported to other subsystems.

We should be encouring drivers and subsystems to follow the RDMA model
for lifecycle which is proven to work.

Jason

Powered by blists - more mailing lists