Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

[Danilo Krummrich <dakr@kernel.org>]

(For some reason, when replying to this mail, mutt removed Sima from To: and
instead switched Cc: to To:, hence resending.)

On Thu, Mar 06, 2025 at 04:21:51PM +0100, Simona Vetter wrote:
> On Thu, Mar 06, 2025 at 02:37:07PM +0100, Danilo Krummrich wrote:
> > On Wed, Mar 05, 2025 at 01:41:19PM -0400, Jason Gunthorpe wrote:
> > > On Mon, Feb 24, 2025 at 01:49:06PM +0200, Abdiel Janulgue wrote:
> > > 
> > > > +impl<T: AsBytes + FromBytes> Drop for CoherentAllocation<T> {
> > > > +    fn drop(&mut self) {
> > > > +        let size = self.count * core::mem::size_of::<T>();
> > > > +        // SAFETY: the device, cpu address, and the dma handle is valid due to the
> > > > +        // type invariants on `CoherentAllocation`.
> > > > +        unsafe {
> > > > +            bindings::dma_free_attrs(
> > > > +                self.dev.as_raw(),
> > > > +                size,
> > > > +                self.cpu_addr as _,
> > > > +                self.dma_handle,
> > > > +                self.dma_attrs.as_raw(),
> > > > +            )
> > > 
> > > I mentioned this in another thread..
> > > 
> > > There is an additional C API restriction here that the DMA API
> > > functions may only be called by a driver after probe() starts and
> > > before remove() completes. This applies to dma_free_attrs().
> > > 
> > > It is not enough that a refcount is held on device.
> > > 
> > > Otherwise the kernel may crash as the driver core allows resources
> > > used by the DMA API to be changed once the driver is removed.
> > > 
> > > See the related discussion here, with an example of what the crash can
> > > look like:
> > > 
> > > https://lore.kernel.org/lkml/8067f204-1380-4d37-8ffd-007fc6f26738@kernel.org/T/#m0c7dda0fb5981240879c5ca489176987d688844c
> > > 
> > >  > 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.
> > 
> > > 
> > > My rust is non-existent, but I did not see anything about this
> > > point.
> > 
> > Indeed, this needs to be fixed. It means that a CoherentAllocation also needs to
> > be embedded in a Devres container.
> > 
> > > 
> > > 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 do you mean with that? DMA memory allocations or "normal" memory
allocations?

The latter should never be in a Devres container. The Devres container should
only hold things that, for safety reasons, are not allowed to out-live device
/ driver unbind.

> But for dma mappings at runtime this will be too slow.

What exactly do you mean with "DMA mappings at runtime"? What to you think is
is slow in this aspect?