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

[Jason Gunthorpe]

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@xxxxxxxxxx/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.

My rust is non-existent, but I did not see anything about this
point.

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.

Jason