Re: [PATCH v5 15/20] rust: io: implement a view type for `Coherent`

[sashiko-bot@kernel.org]

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 <gary@garyguo.net>

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/20260626-io_projection-v5-0-d0961471ae50@garyguo.net?part=15