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

[Gary Guo]

On Thu Sep 3, 2026 at 4:22 PM BST, Danilo Krummrich wrote:
> 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.

Right, so despite that `Coherent` itself not having callbacks, IOMMU side can
have callback that associated with `Coherent` instance and thus require device
to be bound.

>
> 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. :)

No, I don't want this to be unsafe.

I just wonder if it could be made not unsound at all. I think in this case at
least it is possible with a revocation mechanism -- the `Coherent` itself is
just a piece of memory and does not reference other resoruces, so the
destructing it late does not matter (unlike registration). So at least it is
*possible* to close the hole.

> (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.)
>

For registration because it references other resources so I think we cannot
close the hole with changes internal to each subsystem.

Best,
Gary


> [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,
>  }