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 <[email protected]> >> >> 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/[email protected]/ > > @@ -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, > }
