linux-kernel - Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgiCgMse0-L9Fb_r=3umucTqNosfO4R+1YVzOqavo07zMg@mail.gmail.com>
Date: Tue, 25 Nov 2025 15:22:39 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Zhi Wang <zhiw@...dia.com>, rust-for-linux@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, dakr@...nel.org, 
	bhelgaas@...gle.com, kwilczynski@...nel.org, ojeda@...nel.org, 
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net, 
	bjorn3_gh@...tonmail.com, lossin@...nel.org, a.hindborg@...nel.org, 
	tmgross@...ch.edu, markus.probst@...teo.de, helgaas@...nel.org, 
	cjia@...dia.com, smitra@...dia.com, ankita@...dia.com, aniketa@...dia.com, 
	kwankhede@...dia.com, targupta@...dia.com, joelagnelf@...dia.com, 
	jhubbard@...dia.com, zhiwang@...nel.org
Subject: Re: [PATCH v7 3/6] rust: io: factor common I/O helpers into Io trait

On Tue, Nov 25, 2025 at 3:15 PM Alexandre Courbot <acourbot@...dia.com> wrote:
>
> On Tue Nov 25, 2025 at 11:09 PM JST, Alexandre Courbot wrote:
> > On Wed Nov 19, 2025 at 8:21 PM JST, Zhi Wang wrote:
> > <snip>
> >> -impl<const SIZE: usize> Io<SIZE> {
> >> -    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
> >> -    ///
> >> -    /// # Safety
> >> -    ///
> >> -    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> >> -    /// `maxsize`.
> >> -    pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self {
> >> -        // SAFETY: `Io` is a transparent wrapper around `IoRaw`.
> >> -        unsafe { &*core::ptr::from_ref(raw).cast() }
> >> +/// Checks whether an access of type `U` at the given `offset`
> >> +/// is valid within this region.
> >> +#[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
> >> +    } else {
> >> +        false
> >>      }
> >> +}
> >> +
> >> +/// Represents a region of I/O space of a fixed size.
> >> +///
> >> +/// Provides common helpers for offset validation and address
> >> +/// calculation on top of a base address and maximum size.
> >> +///
> >> +pub trait Io {
> >> +    /// Minimum usable size of this region.
> >> +    const MIN_SIZE: usize;
> >
> > This associated constant should probably be part of `IoInfallible` -
> > otherwise what value should it take if some type only implement
> > `IoFallible`?
> >
> >>
> >>      /// Returns the base address of this mapping.
> >> -    #[inline]
> >> -    pub fn addr(&self) -> usize {
> >> -        self.0.addr()
> >> -    }
> >> +    fn addr(&self) -> usize;
> >>
> >>      /// Returns the maximum size of this mapping.
> >> -    #[inline]
> >> -    pub fn maxsize(&self) -> usize {
> >> -        self.0.maxsize()
> >> -    }
> >> -
> >> -    #[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
> >> -        } else {
> >> -            false
> >> -        }
> >> -    }
> >> +    fn maxsize(&self) -> usize;
> >>
> >> +    /// Returns the absolute I/O address for a given `offset`,
> >> +    /// performing runtime bound checks.
> >>      #[inline]
> >>      fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> >> -        if !Self::offset_valid::<U>(offset, self.maxsize()) {
> >> +        if !offset_valid::<U>(offset, self.maxsize()) {
> >>              return Err(EINVAL);
> >>          }
> >
> > Similarly I cannot find any context where `maxsize` and `io_addr` are
> > used outside of `IoFallible`, hinting that these should be part of this
> > trait.
> >
> >>
> >> @@ -239,50 +240,190 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> >>          self.addr().checked_add(offset).ok_or(EINVAL)
> >>      }
> >>
> >> +    /// Returns the absolute I/O address for a given `offset`,
> >> +    /// performing compile-time bound checks.
> >>      #[inline]
> >>      fn io_addr_assert<U>(&self, offset: usize) -> usize {
> >> -        build_assert!(Self::offset_valid::<U>(offset, SIZE));
> >> +        build_assert!(offset_valid::<U>(offset, Self::MIN_SIZE));
> >>
> >>          self.addr() + offset
> >>      }
> >
> > ... and `io_addr_assert` is only used from `IoFallible`.
> >
> > So if my gut feeling is correct, we can disband `Io` entirely and only
>
> ... except that we can't due to `addr`, unless we find a better way to
> provide this base. But even if we need to keep `Io`, the compile-time
> and runtime members should be moved to their respective traits.

If you have IoInfallible depend on IoFallible, then you can place
`addr` on IoFallible.

(And I still think you should rename IoFallible to Io and IoInfallible
to IoKnownSize.)

Alice