Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The safety documentation for `CoherentView::as_mut` and `as_ref` fails to demand Rust's reference aliasing exclusivity, allowing Undefined Behavior. --
commit 20e5ba166044275ffb7dbced6b142a0a7c1c896e Author: Gary Guo <[email protected]> Implement a CoherentView type which is a view of Coherent. To be able to give out DMA handles, the view type contains both CPU and DMA pointers, and the projection method projects both at once. Delegate most of the Io implementation to SysMemBackend. Provide a method to erase the DMA handle and give out a SysMem view, if the user does not need the dma_handle. > diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs > index 200def84fb69..ab6504910e4f 100644 > --- a/rust/kernel/dma.rs > +++ b/rust/kernel/dma.rs [ ... ] > +impl<T: ?Sized> Copy for CoherentView<'_, T> {} > +impl<T: ?Sized> Clone for CoherentView<'_, T> { > + #[inline] > + fn clone(&self) -> Self { > + *self > + } > +} > + > +impl<'a, T: ?Sized> CoherentView<'a, T> { [ ... ] > + /// Returns a reference to the data in the region. > + /// > + /// # Safety > + /// > + /// * Callers must ensure that the device does not read/write to/from > memory while the returned > + /// reference is live. > + /// * Callers must ensure that this call does not race with a write to > the same region while > + /// the returned reference is live. > + #[inline] > + pub unsafe fn as_ref(self) -> &'a T { [Severity: High] Since CoherentView implements Copy, callers can duplicate the view and call this method multiple times. Does this safety contract adequately protect against creating overlapping shared and mutable references? Rust's aliasing rules consider it undefined behavior to create aliasing mutable and shared references, even in single-threaded code without a data race. It seems the preconditions should explicitly require callers to ensure no mutable references to the same memory region exist while the returned reference is live. > + // SAFETY: pointer is aligned and valid per type invariant. Aliasing > rule is satisfied per > + // safety requirement. > + unsafe { &*self.cpu_addr.as_ptr() } > + } > + > + /// 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 > + /// reference is live. > + /// * Callers must ensure that this call does not race with a read or > write to the same region > + /// while the returned reference is live. > + #[inline] > + pub unsafe fn as_mut(self) -> &'a mut T { [Severity: High] Similar to as_ref() above, does this safety contract prevent the creation of aliasing mutable references? Because the struct implements Copy and this method takes self by value returning &'a mut T, a caller could copy the view and call as_mut() sequentially. The safety contract only forbids racing with a read or write, which does not prevent the immediate undefined behavior of having multiple overlapping &'a mut T references. Should the documentation be updated to demand Rust's reference aliasing exclusivity? > + // 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=15
