Re: [PATCH v3 18/19] rust: io: add copying methods

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Safe `copy_from_io_slice` and `copy_to_io_slice` do not handle or prevent overlapping memory slices, leading to Undefined Behavior or silent data corruption.
- [Low] Default implementations of `copy_from_io` and `copy_to_io` use `build_assert!()` on a function parameter but lack the `#[inline(always)]` annotation.
- [Low] The imports in `samples/rust/rust_dma.rs` do not follow the kernel alphabetical sorting style.
--

commit 0d38b02719ddeacdde6e32f8d5c83949c35fd537
Author: Gary Guo <gary@garyguo.net>

rust: io: add copying methods

This commit introduces copying methods for I/O memory in Rust, replacing the
older DMA read/write macros for larger structs. It adds `copy_read`,
`copy_write`, and slice copying methods for various backends, optimizing to
`memcpy` where possible or using a stack buffer fallback.

> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 72f3acc0f50d1..2b238b6256728 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -229,6 +230,63 @@ pub trait IoCapable<T>: IoBackend {
>      fn io_write<'a>(view: Self::View<'a, T>, value: T);
>  }
>  
> +/// Trait indicating that an I/O backend supports memory copy operations.
> +///
> +/// # Safety
> +///
> +/// If [`Self::is_mapped`] is overridden, it must be correct per documentation.
> +pub unsafe trait IoCopyable: IoBackend {
> +    /// Whether the pointers for this I/O backend are in the CPU address space, and are coherently
> +    /// mapped.
> +    ///
> +    /// When this returns true, `Self::as_ptr(view)` must return a valid and aligned pointer. The
> +    /// pointer may be accessed with byte-wise atomic memory copy or volatile read/write.
> +    ///
> +    /// This is not an associated constants to support backends where the view may be conditionally
> +    /// mapped. This method should be marked as `#[inline(always)]` if it always returns true, so
> +    /// `build_assert!()` in `copy_{from,to}io` can see it.
> +    #[inline]
> +    fn is_mapped<T: ?Sized + KnownSize>(_view: Self::View<'_, T>) -> bool {
> +        false
> +    }
> +
> +    /// Copy contents of `view` to `buffer`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `buffer` is valid for volatile write for `view.size()` bytes.
> +    #[inline]
> +    unsafe fn copy_from_io(view: Self::View<'_, [u8]>, buffer: *mut u8) {

[Severity: Low]
This isn't a bug, but the Rust subsystem guidelines mandate that functions using
`build_assert!()` that depend on function parameters need to be annotated with
`#[inline(always)]`.

Could these default implementations result in spurious compilation failures if
the compiler chooses not to inline them and attempts to evaluate the assertion
at link-time?

> +        build_assert!(Self::is_mapped(view));
> +
> +        let ptr = Self::as_ptr(view);
> +
> +        // Use `bindings::memcpy` instead of copy_nonoverlapping for volatile.
> +        // SAFETY:
> +        // - `is_mapped` guarantees `ptr` is in CPU address space and valid for read.
> +        // - `buffer` is valid for write for `view.size()` bytes which is equal to `ptr.len()`.
> +        unsafe { bindings::memcpy(buffer.cast(), ptr.cast(), ptr.len()) };
> +    }
> +
> +    /// Copy `size` bytes from `buffer` to `address`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - `buffer` is valid for volatile read for `view.size()` bytes.
> +    #[inline]
> +    unsafe fn copy_to_io(view: Self::View<'_, [u8]>, buffer: *const u8) {

[Severity: Low]
This isn't a bug, but the Rust subsystem guidelines mandate that functions using
`build_assert!()` that depend on function parameters need to be annotated with
`#[inline(always)]`.

Could these default implementations result in spurious compilation failures if
the compiler chooses not to inline them and attempts to evaluate the assertion
at link-time?

> +        build_assert!(Self::is_mapped(view));
> +
> +        let ptr = Self::as_ptr(view);
> +
> +        // Use `bindings::memcpy` instead of copy_nonoverlapping for volatile.
> +        // SAFETY:
> +        // - `is_mapped` guarantees `ptr` is in CPU address space and valid for write.
> +        // - `buffer` is valid for read for `view.size()` bytes which is equal to `ptr.len()`.
> +        unsafe { bindings::memcpy(ptr.cast(), buffer.cast(), ptr.len()) };
> +    }
> +}

[ ... ]

> +    fn copy_from_io_slice<'b, T>(self, data: T)
> +    where
> +        Self::Backend: IoCopyable,
> +        Self: Io<'a, Target = [u8]>,
> +        T: Io<'b, Target = [u8], Backend: IoCopyable>,
> +    {
> +        fn copy_from_io_slice_via_buffer<
> +            'a,
> +            'b,
> +            T: Io<'a, Target = [u8], Backend: IoCopyable>,
> +            U: Io<'b, Target = [u8], Backend: IoCopyable>,
> +        >(
> +            dest: T,
> +            src: U,
> +        ) {
> +            let mut buf = MaybeUninit::<[u8; 256]>::uninit();
> +
> +            let mut offset = 0;
> +            let mut len = dest.len();
> +
> +            while len != 0 {
> +                let copy_len = core::cmp::min(len, 256);
> +
> +                // SAFETY: `buf.as_mut_ptr()` is valid for write for `copy_len` bytes as `copy_len
> +                // <= 256`.
> +                unsafe {
> +                    U::Backend::copy_from_io(
> +                        io_project!(src, [panic: offset..][panic: ..copy_len]),
> +                        buf.as_mut_ptr().cast(),
> +                    )
> +                };
> +
> +                // SAFETY: `buf.as_ptr()` is valid for read for `copy_len` bytes as `copy_len <=
> +                // 256`.
> +                unsafe {
> +                    T::Backend::copy_to_io(
> +                        io_project!(dest, [panic: offset..][panic: ..copy_len]),
> +                        buf.as_ptr().cast(),
> +                    )
> +                };
> +
> +                offset += copy_len;
> +                len -= copy_len;
> +            }
> +        }

[Severity: High]
Since `copy_from_io_slice` is a safe function, what prevents a user from
passing overlapping I/O views?

`Io` views implement `Copy` and wrap raw pointers, so a user could safely create
overlapping views of the same memory region and pass them here.

If the backends are CPU-mapped, this eventually delegates to C's
`bindings::memcpy()`. Calling `memcpy` with overlapping pointers is Undefined
Behavior and can trigger crashes under fortified implementations.

For unmapped backends, it falls back to `copy_from_io_slice_via_buffer()` which
processes the copy sequentially forward in 256-byte chunks. If the destination
overlaps and sits ahead of the source, wouldn't an earlier chunk's write
overwrite data that a later chunk's read is supposed to copy, causing silent
data corruption?

Should this method explicitly detect pointer overlap and panic (like
`[T]::copy_from_slice`), or dynamically choose the copy direction?

> +
> +        assert_eq!(self.len(), data.len());
> +
> +        let dst_view = self.as_view();
> +        let src_view = data.as_view();
> +
> +        if T::Backend::is_mapped(src_view) {
> +            // SAFETY: `T::Backend::as_ptr(src_view)` is valid for read for `data.len()`
> +            // bytes.
> +            unsafe {
> +                Self::Backend::copy_to_io(self.as_view(), T::Backend::as_ptr(src_view).cast())
> +            }
> +        } else if Self::Backend::is_mapped(dst_view) {
> +            // SAFETY: `Self::Backend::as_ptr(dst_view)` is valid for write for `data.len()`
> +            // bytes.
> +            unsafe {
> +                T::Backend::copy_from_io(data.as_view(), Self::Backend::as_ptr(dst_view).cast())
> +            }
> +        } else {
> +            copy_from_io_slice_via_buffer(dst_view, src_view)
> +        }
> +    }

[ ... ]

> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 6727c441658aa..0f2597798ff5b 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -12,7 +12,11 @@
>          Device,
>          DmaMask, //
>      },
> -    io::io_read,
> +    io::{
> +        io_project,
> +        io_read,
> +        Io, //
> +    },

[Severity: Low]
This isn't a bug, but the imports do not seem to follow the alphabetical sorting
style expected by the kernel vertical import guidelines. Since uppercase letters
precede lowercase ones in ASCII, shouldn't `Io` be sorted before `io_project`
and `io_read`?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-io_projection-v3-0-c5cde13a5ec4@garyguo.net?part=18