Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Unsound `AsBytes` implementation for `Region<SIZE>` leading to undefined behavior due to alignment padding. - [Low] Incorrect use of `#[inline(always)]` on `ProjectHelper` methods, violating subsystem coding guidelines. --
commit ab6f2e99706c5568aaec1a417178b7f6d1435b90 Author: Gary Guo <[email protected]> rust: io: add projection macro and methods Add a io_project!() macro allows projection from Io to a subview of it, using the pointer projection mechanism to perform compile-time checks. For cases where type-casting is required, the try_cast() function may be used where the size and alignment checks are performed at runtime. > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index 9c2ea17ca87b5..173f8c0ba2d6c 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs [ ... ] > @@ -92,6 +96,11 @@ fn size(p: *const Self) -> usize { > } > } > > +// SAFETY: I/O regions can compose of arbitrary bytes. > +unsafe impl<const SIZE: usize> kernel::transmute::FromBytes for Region<SIZE> > {} > +// SAFETY: Values read from I/O are always treated as initialized. > +unsafe impl<const SIZE: usize> kernel::transmute::AsBytes for Region<SIZE> {} [Severity: High] Does this implementation of AsBytes for Region<SIZE> introduce undefined behavior due to alignment padding? Region<SIZE> is defined with an alignment constraint and contains an unsized [u8] field. Because core::mem::size_of_val for dynamically sized types rounds the size up to a multiple of the alignment, a Region created with a size that is not a multiple of 8 (on 64-bit systems) will include trailing padding bytes. Since AsBytes::as_bytes() relies on size_of_val(self) to slice the memory, calling it on &Region<SIZE> could produce a &[u8] that extends past the actual allocated length into out-of-bounds or uninitialized memory. > + > /// Raw representation of an MMIO region. > /// > /// `MmioRaw<T>` is equivalent to `T __iomem *` in C. [ ... ] > @@ -983,3 +1039,78 @@ pub fn relaxed(self) -> RelaxedMmio<'a, T> { > // MMIO regions on 64-bit systems also support 64-bit accesses. > #[cfg(CONFIG_64BIT)] > impl_mmio_io_capable!(RelaxedMmioBackend, u64, readq_relaxed, > writeq_relaxed); > + > +// This helper turns associated functions to methods so it can be invoked in > macro. > +// Used by `io_project!()` only. > +#[doc(hidden)] > +#[derive(Clone, Copy)] > +pub struct ProjectHelper<T>(pub T); > + > +impl<'a, T> ProjectHelper<T> > +where > + T: Io<'a, Backend: IoBackend<View<'a, T::Target> = T>>, > +{ > + // These helper methods must not have symbols present in binary to avoid > confusion. > + #[inline(always)] > + pub fn as_ptr(self) -> *mut T::Target { [Severity: Low] This isn't a bug, but should this use #[inline] instead of #[inline(always)]? According to the rust subsystem guidelines, functions that are small or forwarding to a binding call in abstractions should be annotated with #[inline]. The #[inline(always)] annotation should be reserved for functions using build_assert!() that depend on function parameters. > + T::Backend::as_ptr(self.0) > + } > + > + /// # Safety > + /// > + /// Same as `IoBackend::project_view` > + #[inline(always)] > + pub unsafe fn project_view<U: ?Sized + KnownSize>( [Severity: Low] This isn't a bug, but should this use #[inline] instead of #[inline(always)]? According to the rust subsystem guidelines, functions that are small or forwarding to a binding call in abstractions should be annotated with #[inline]. The #[inline(always)] annotation should be reserved for functions using build_assert!() that depend on function parameters. > + self, > + ptr: *mut U, > + ) -> <T::Backend as IoBackend>::View<'a, U> { > + // SAFETY: Per safety requirement. > + unsafe { T::Backend::project_view::<T::Target, _>(self.0, ptr) } > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
