Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] `CoherentView<'a, T>` automatically implements `Send` when `T: Sync`, but provides an `as_mut()` capability to generate `&mut T`, allowing thread-safety bounds bypass. - [High] `CoherentView::as_mut` consumes `self` by value on a `Copy` type and returns `&'a mut T`, but its safety documentation fails to require strict exclusivity. --
commit 12e6f6d7abcd0a26719f3c4f268f24a07774b118 Author: Gary Guo <[email protected]> rust: io: implement a view type for `Coherent` 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 200def84fb69e..ab6504910e4f5 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, > +} [Severity: High] Since SysMem<'a, T> automatically implements Send when T: Sync, CoherentView also implements Send when T: Sync. If T is !Send, can a caller move CoherentView to another thread and call as_mut() to get a mutable reference? This appears to allow bypassing Rust's concurrency guarantees and could lead to data races or memory corruption if the type relies on thread-local invariants. Note that this same pattern seems to pre-exist in Coherent<T> as well. [ ... ] > + /// 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] Because CoherentView derives Copy and as_mut() takes self by value instead of taking a mutable borrow, can a caller call as_mut() multiple times in succession to create overlapping mutable references? While the safety documentation requires callers to prevent races, it does not explicitly require strict exclusivity. Could a caller sequentially use two aliases without data races, believing they adhered to the safety contract, but still trigger undefined behavior due to aliasing violations? This documentation omission might also exist in the preexisting Coherent::as_mut. > + // 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
