Re: [PATCH v3 14/19] 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::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 <gary@garyguo.net>

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/20260608-io_projection-v3-0-c5cde13a5ec4@garyguo.net?part=14