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 <[email protected]> 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/[email protected]?part=18
