On Mon Jun 15, 2026 at 7:13 PM JST, Gary Guo wrote: > On Mon Jun 15, 2026 at 5:28 AM BST, Alexandre Courbot wrote: >> On Fri Jun 12, 2026 at 1:28 AM JST, Gary Guo wrote: >>> The current safety comment on `io_read`/`io_write` does not cover the topic >>> about alignment. Add it so it can be relied on by implementor of >>> `IoCapable`. >>> >>> Expand the check `Io` by taking `self.addr()` into consideration when >> >> "the check performed by `Io`" maybe? >> >>> checking if `offset` is aligned. For the compile-time `io_addr_assert` >>> check, check using the known minimum alignment of `IO::Target` and the >> >> typo: s/IO/Io. >> >>> accessed type. >>> >>> While at it, fix the alignment check to use `align_of` instead of >>> `size_of`. The values match for all primitives (including u64, given that >>> we do not provide u64 accessor on 32-bit platforms), but are not >>> necessarily true for custom types. >>> >>> Signed-off-by: Gary Guo <[email protected]> >>> --- >>> rust/kernel/io.rs | 25 ++++++++++++++++--------- >>> 1 file changed, 16 insertions(+), 9 deletions(-) >>> >>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs >>> index bef571dad6eb..fa9ae39ad9d2 100644 >>> --- a/rust/kernel/io.rs >>> +++ b/rust/kernel/io.rs >>> @@ -196,13 +196,14 @@ pub fn maxsize(&self) -> usize { >>> #[repr(transparent)] >>> pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>); >>> >>> -/// Checks whether an access of type `U` at the given `offset` >>> +/// Checks whether an access of type `U` at the given `base` and the given >>> `offset` >>> /// is valid within this region. >>> +/// >>> +/// The `base` is used for alignment checking only. This can be set to 0 >>> to skip the check. >>> #[inline] >>> -const fn offset_valid<U>(offset: usize, size: usize) -> bool { >>> - let type_size = core::mem::size_of::<U>(); >>> - if let Some(end) = offset.checked_add(type_size) { >>> - end <= size && offset % type_size == 0 >>> +const fn offset_valid<U>(base: usize, offset: usize, size: usize) -> bool { >>> + if let Some(end) = offset.checked_add(size_of::<U>()) { >>> + end <= size && (base.wrapping_add(offset) % align_of::<U>() == 0) >>> } else { >>> false >>> } >>> @@ -221,14 +222,16 @@ pub trait IoCapable<T> { >>> /// >>> /// # Safety >>> /// >>> - /// The range `[address..address + size_of::<T>()]` must be within the >>> bounds of `Self`. >>> + /// - The range `[address..address + size_of::<T>()]` must be within >>> the bounds of `Self`. >>> + /// - `address` must be aligned. >>> unsafe fn io_read(&self, address: usize) -> T; >>> >>> /// Performs an I/O write of `value` at `address`. >>> /// >>> /// # Safety >>> /// >>> - /// The range `[address..address + size_of::<T>()]` must be within the >>> bounds of `Self`. >>> + /// - The range `[address..address + size_of::<T>()]` must be within >>> the bounds of `Self`. >>> + /// - `address` must be aligned. >>> unsafe fn io_write(&self, value: T, address: usize); >>> } >>> >>> @@ -310,7 +313,11 @@ pub trait Io { >>> // Always inline to optimize out error path of `build_assert`. >>> #[inline(always)] >>> fn io_addr_assert<U>(&self, offset: usize) -> usize { >>> - build_assert!(offset_valid::<U>(offset, Self::Target::MIN_SIZE)); >>> + // We cannot check alignment with `offset_valid` using >>> `self.addr()`. So set 0 for it and >>> + // ensure alignment by checking that the alignment of `U` is >>> smaller or equal to the >>> + // alignment of `Self::Target`. >>> + const_assert!(Alignment::of::<U>().as_usize() <= >>> Self::Target::MIN_ALIGN.as_usize()); >>> + build_assert!(offset_valid::<U>(0, offset, >>> Self::Target::MIN_SIZE)); >> >> IIUC this can allow unaligned accesses if `self.addr()` itself is not >> properly aligned. Do we need a new `Io` invariant for that or is it >> already enforced somewhere? > > Adding a trait invariant would require marking the trait as `unsafe`, which I > don't want to do because the `addr()` method is removed later anyway. > > One argument is that it's `Io` implementation causing issue for its own if its > `addr()` is not aligned. This is later redefined using projection and views, > which further shifts responsiblity of upholding invariants to the `Io` type > implementator itself.
If we end up removing `addr` anyway it is less critical to solve this. Especially since this is not a new issue.
