Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The `size()` method creates a shared reference to potentially mutating DMA memory, causing Undefined Behavior. - [Medium] `ContiguousBuffer` implementation for `KBox<T>` implicitly requires `T: Sized`, preventing the mapping of dynamically sized buffers. - [Low] Missing `#[inline]` annotations on small trait methods in an abstraction. --
--- Patch [1]: [PATCH 1/3] rust: dma: add ContiguousBuffer trait for streaming DMA storage --- commit 9463792a46877c9a52666dde64c8d1a545e696f7 Author: Maurice Hieronymus <[email protected]> rust: dma: add ContiguousBuffer trait for streaming DMA storage This commit introduces the ContiguousBuffer trait to describe backing storage that can be mapped via the streaming DMA API. It also implements this trait for KBox<T> to guarantee physically contiguous memory allocated via kmalloc(). > diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs > index 200def84fb69e..8a8af5ab7feb8 100644 > --- a/rust/kernel/dma.rs > +++ b/rust/kernel/dma.rs [ ... ] > +// SAFETY: `KBox` allocates via `kmalloc()`, which returns a single > physically contiguous, > +// DMA-safe region in the kernel's linear mapping. All three methods > describe that allocation. > +unsafe impl<T: FromBytes + AsBytes> ContiguousBuffer for KBox<T> { [Severity: Medium] Does this implementation unintentionally exclude dynamically sized buffers like KBox<[u8]>? Since T implicitly requires Sized here, it restricts the implementation to statically sized types, despite the ContiguousBuffer trait allowing type Data: ?Sized. Should this include the ?Sized bound? > + type Data = T; > + > + fn ptr(&mut self) -> *mut c_void { [Severity: Low] This isn't a bug, but according to the Rust subsystem guidelines, should small methods in abstractions, such as ptr(), size(), and data(), be annotated with #[inline]? > + let ptr = &raw mut **self; > + ptr.cast() > + } > + > + fn size(&self) -> usize { > + const { > + assert!( > + core::mem::size_of::<T>() > 0, > + "It doesn't make sense to map a ZST for DMA" > + ); > + } > + > + core::mem::size_of_val(&**self) [Severity: High] Does this expression create a shared reference to potentially mutating DMA memory? Calling core::mem::size_of_val(&**self) implicitly calls deref() and creates a shared reference (&T) to the underlying allocation. If the device is actively modifying the buffer during a DMA operation, could materializing this shared reference violate Rust's aliasing rules and cause Undefined Behavior? Since T is implicitly Sized in this implementation, can this just use core::mem::size_of::<T>() to evaluate the size purely at compile time without creating hazardous references? > + } > + > + fn data(&mut self) -> &mut Self::Data { > + self > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
