Re: [PATCH 4/4] rust: dma: tie Coherent and CoherentBox to the device's bound lifetime

[Danilo Krummrich]

On Thu Sep 3, 2026 at 3:20 PM CEST, Gary Guo wrote:
> The rust/kernel code looks good to me. Haven't checked nova part in detail, but
> it looks like a mechanical conversion, so would be fine if it builds.
>
> Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
>
> Sashiko points out that the `Coherent` could be leaked -- what's the implication
> when that happens? I think it's not going to be as problematic like
> registrations because coherent allocation carries no callbacks, so we probably
> don't need this to be unsafe, but I do wonder how'd DMA subsystem handle this.

The implication if leaked is the same as if it is kept alive past driver unbind,
which is why I changed the TODO comment accordingly in the hunk below.

If you look for a specific example, there's [1] for instance. So, it is
problematic, which is why I added the TODO comment back then.

But, we did accept this soundness hole from the get-go for both, keeping a
coherent allocation alive beyond driver unbind and for leaking it.

With this patch it is now impossible to keep it alive beyond driver unbind, so
switching to unsafe now would be a bit odd. :)

(The fact that we did accept this for coherent allocations is also one reason
why I was recently arguing that we can also make the forget() issue an accepted
soundness hole for registrations.)

[1] https://lore.kernel.org/all/6a7910da.9c11d2ce.289b96.00da.GAE@xxxxxxxxxx/

@@ -588,26 +587,20 @@ fn from(value: CoherentBox<T>) -> Self {
 ///   to an allocated region of coherent memory and `dma_addr` is the DMA address base of the
 ///   region.
 /// - The size in bytes of the allocation is equal to size information via pointer.
-// TODO
 //
-// DMA allocations potentially carry device resources (e.g.IOMMU mappings), hence for soundness
-// reasons DMA allocation would need to be embedded in a `Devres` container, in order to ensure
-// that device resources can never survive device unbind.
-//
-// However, it is neither desirable nor necessary to protect the allocated memory of the DMA
-// allocation from surviving device unbind; it would require RCU read side critical sections to
-// access the memory, which may require subsequent unnecessary copies.
-//
-// Hence, find a way to revoke the device resources of a `Coherent`, but not the
-// entire `Coherent` including the allocated memory itself.
-pub struct Coherent<T: KnownSize + ?Sized> {
-    dev: ARef<device::Device>,
+// The lifetime parameter ties DMA allocations to the device's bound scope, ensuring they are freed
+// before the device is unbound under normal circumstances. However, if a `Coherent` is leaked (e.g.
+// via `mem::forget`), device resources such as IOMMU mappings will not be released.  Making all
+// constructors `unsafe` to prevent this is considered too restrictive for the common case; this
+// soundness hole is accepted for now.
+pub struct Coherent<'a, T: KnownSize + ?Sized> {
+    dev: &'a device::Device<Bound>,
     dma_addr: DmaAddress,
     cpu_addr: NonNull<T>,
     dma_attrs: Attrs,
 }