Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `CoherentView::as_mut` and `Coherent::as_mut` return mutable references without requiring uniqueness in their safety contracts, leading to potential UB. - [Medium] Missing `?Sized` bound on `CoherentView` implementation block. --
commit 0b0ac87fb8d6dae9b55b873481974c835edfff5a Author: Gary Guo <[email protected]> rust: io: implement a view type for Coherent This commit implements a CoherentView type as a view of Coherent. It contains both CPU and DMA pointers and delegates most of the Io implementation to SysMemBackend. > diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs > index 200def84fb69e..970a667b9be28 100644 > --- a/rust/kernel/dma.rs > +++ b/rust/kernel/dma.rs [ ... ] > @@ -1133,6 +1140,132 @@ unsafe impl Send for CoherentHandle {} > // plain `Copy` values. > unsafe impl Sync for CoherentHandle {} > > +/// View type for `Coherent`. > +/// > +/// This is same as [`SysMem`] but with additional information that allows > handing out a DMA handle. > +pub struct CoherentView<'a, T: ?Sized> { > + cpu_addr: SysMem<'a, T>, > + dma_handle: DmaAddress, > +} > + > +impl<T: ?Sized> Copy for CoherentView<'_, T> {} > +impl<T: ?Sized> Clone for CoherentView<'_, T> { > + #[inline] > + fn clone(&self) -> Self { > + *self > + } > +} > + > +impl<'a, T> CoherentView<'a, T> { [Severity: Medium] Since generic type parameters implicitly require Sized in Rust, this impl block omits the ?Sized bound on T. Does this prevent dynamically-sized views (like CoherentView<'_, [u8]>) from using the methods defined here (as_sys_mem, dma_handle, as_ref, as_mut)? > + /// Erase the DMA handle information and obtain a [`SysMem`] view of the > same memory region. > + #[inline] > + pub fn as_sys_mem(self) -> SysMem<'a, T> { [ ... ] > + /// Returns a mutable reference to the data in the region. > + /// > + /// # Safety > + /// > + /// * Callers must ensure that the device does not read/write to/from > memory while the returned > + /// slice is live. > + /// * Callers must ensure that this call does not race with a read or > write to the same region > + /// while the returned slice is live. > + #[inline] > + pub unsafe fn as_mut(self) -> &'a mut T { [Severity: High] Since CoherentView implements Copy, a caller could copy the view and call as_mut(self) multiple times to yield multiple overlapping &'a mut T references to the exact same memory. Does this same issue apply to Coherent::as_mut when returning a mutable reference from a shared borrow? Because the documented Safety contracts only forbid data races and omit the requirement that callers must uphold Rust's strict aliasing rules for uniqueness of mutable references, could following this contract still lead to undefined behavior? > + // SAFETY: pointer is aligned and valid per type invariant. Aliasing > rule is satisfied per > + // safety requirement. > + unsafe { &mut *self.cpu_addr.as_ptr() } > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
