Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] StreamingCpuGuard unconditionally implements DerefMut, allowing safe Rust code to mutate DMA_FROM_DEVICE buffers. - [High] Safe Rust code can bypass dma_sync_single_for_device by leaking StreamingCpuGuard, causing a hardware data race. - [Medium] Panic in ContiguousBuffer::data() leaves the DMA buffer synced for the CPU without creating a guard. - [Low] Missing #[inline] annotations on small/forwarding methods in abstractions. --
--- Patch [2]: [PATCH 2/3] rust: dma: add abstraction for the single-buffer streaming DMA API --- commit 5deae1f6c89c43e53f36e40c370843d948aa3422 Author: Maurice Hieronymus <[email protected]> rust: dma: add abstraction for the single-buffer streaming DMA API Add Streaming, a safe abstraction around dma_map_single_attrs(). Between map and unmap the buffer belongs to the device, and the CPU may only access it in between a dma_sync_single_for_cpu() / dma_sync_single_for_device() pair. > diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs > index 8a8af5ab7feb8..cbaf30a2de864 100644 > --- a/rust/kernel/dma.rs > +++ b/rust/kernel/dma.rs > @@ -621,6 +630,389 @@ fn data(&mut self) -> &mut Self::Data { [ ... ] > + pub fn for_cpu(&mut self) -> StreamingCpuGuard<'_, C::Data> { > + let dev = self.dev; > + let dma_addr = self.dma_addr; > + let direction = self.direction; > + let size = self.container.size(); > + > + // SAFETY: By the type invariants, `dev` is bound and `dma_addr` > denotes a live mapping of > + // `size` bytes established with `direction`, which is the range > synced here. > + unsafe { > + bindings::dma_sync_single_for_cpu(dev.as_raw(), dma_addr, size, > direction.into()) > + }; > + > + // INVARIANT: The buffer is now owned by the CPU, and dropping the > guard hands it back. > + StreamingCpuGuard { > + data: self.container.data(), [Severity: Medium] If the custom ContiguousBuffer::data() implementation panics, does this leave the DMA buffer synced for the CPU without creating the guard? While the kernel currently uses panic=abort, this sequence mutates global state (the DMA cache mapping) before a potentially panicking safe trait method call, which could violate panic safety guarantees. Could data() be evaluated before calling dma_sync_single_for_cpu()? > + dev, > + dma_addr, > + size, > + direction, > + } > + } [ ... ] > + fn unmap(&mut self) { [Severity: Low] This isn't a bug, but according to the Rust kernel coding guidelines, functions in abstractions that are small or directly forward to a C binding should be annotated with #[inline]. Should this method have an #[inline] annotation? > + // SAFETY: By the type invariants, `self.dev` is bound and the > mapping is still live, with > + // exactly the address, size, direction and attributes it was > created with. Both callers > + // run this at most once, so the mapping cannot be torn down twice. > + unsafe { > + bindings::dma_unmap_single_attrs( > + self.dev.as_raw(), > + self.dma_addr, > + self.container.size(), > + self.direction.into(), > + self.dma_attrs.as_raw(), > + ) > + }; > + } [ ... ] > +impl<T: ?Sized> Drop for StreamingCpuGuard<'_, T> { > + fn drop(&mut self) { [Severity: High] Does relying on Drop to run dma_sync_single_for_device() allow safe Rust code to bypass the synchronization? If a driver calls for_cpu() and then passes the guard to core::mem::forget(), the mutable borrow ends without Drop ever being called. The driver could then safely call submit(), causing the device to interact with an unsynchronized buffer and breaking the explicit type invariant. > + // SAFETY: By the type invariants, `self.dev` is bound and > `self.dma_addr` denotes a live > + // mapping of `self.size` bytes established with `self.direction`, > which is the range > + // synced here. > + unsafe { > + bindings::dma_sync_single_for_device( > + self.dev.as_raw(), > + self.dma_addr, > + self.size, > + self.direction.into(), > + ) > + }; > + } > +} > + > +impl<T: ?Sized> Deref for StreamingCpuGuard<'_, T> { > + type Target = T; > + > + fn deref(&self) -> &Self::Target { [Severity: Low] This isn't a bug, but this small trait implementation in an abstraction might benefit from an #[inline] annotation to follow the Rust kernel coding guidelines. > + self.data > + } > +} > + > +impl<T: ?Sized> DerefMut for StreamingCpuGuard<'_, T> { > + fn deref_mut(&mut self) -> &mut Self::Target { [Severity: Low] This isn't a bug, but this trait implementation could also use an #[inline] annotation. [Severity: High] Does this unconditional DerefMut implementation permit safe code to mutate buffers mapped with DataDirection::FromDevice? According to the DMA API, memory mapped with DMA_FROM_DEVICE must be treated as read-only by the CPU to avoid dirtying cache lines, which could asynchronously evict and overwrite the device's DMA writes in physical memory. > + self.data > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
