| 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
| ||
|
Message-ID: <97fc6c36933522eaf75f34f8fcd445715432fde4.camel@redhat.com>
Date: Fri, 06 Feb 2026 15:29:11 -0500
From: lyude@...hat.com
To: Alexandre Courbot <acourbot@...dia.com>, Danilo Krummrich
<dakr@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Daniel Almeida
<daniel.almeida@...labora.com>, Miguel Ojeda <ojeda@...nel.org>, Boqun
Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno
Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
Trevor Gross <tmgross@...ch.edu>, Bjorn Helgaas <bhelgaas@...gle.com>,
Krzysztof Wilczyński <kwilczynski@...nel.org>
Cc: driver-core@...ts.linux.dev, rust-for-linux@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org, Zhi Wang
<zhiw@...dia.com>, Eliot Courtney <ecourtney@...dia.com>
Subject: Re: [PATCH v2 1/6] rust: io: turn IoCapable into a functional trait
On Fri, 2026-02-06 at 15:00 +0900, Alexandre Courbot wrote:
> `IoCapable<T>` is currently used as a marker trait to signal that the
> methods of the `Io` trait corresponding to `T` have been overridden
> by
> the implementor (the default implementations triggering a build-time
> error).
>
> This goes against the DRY principle and separates the signaling of
> the
> capability from its implementation, making it possible to forget a
> step
> while implementing a new `Io`.
I realized another thing that might be missing from this series while
porting the iosys_map code over: it seems like IoCapable is fine being
a safe trait, but Io and IoKnownSize both seem like they should
actually be unsafe traits. The main reason being that IoCapable doesn't
actually provide any methods that provide a guarantee of being able to
read/write from the IO space, but Io does - and IoKnownSize is making
the guarantee the IO space is at least of size IoKnownSize::MIN_SIZE.
>
> Another undesirable side-effect is that it makes the implementation
> of
> I/O backends boilerplate-y and convoluted: currently this is done
> using
> two levels of imbricated macros that generate unsafe code.
>
> Fix these issues by turning `IoCapable` into a functional trait that
> includes the raw implementation of the I/O access for `T` using
> unsafe methods that work with an arbitrary address.
>
> This allows us to turn the default methods of `Io` into regular
> methods
> that check the passed offset, turn it into an address, and call into
> the
> corresponding `IoCapable` functions, removing the need to overload
> them
> at all.
>
> `IoCapable` must still be implemented for all supported primitive
> types,
> which is still done more concisely using a macro, but this macro
> becomes
> much simpler and does not require calling into another one.
>
> Reviewed-by: Daniel Almeida <daniel.almeida@...labora.com>
> Acked-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
> ---
> rust/kernel/io.rs | 169 ++++++++++++++++++++++++++++++++++++++--
> ----------
> rust/kernel/pci/io.rs | 37 ++++++++++-
> 2 files changed, 163 insertions(+), 43 deletions(-)
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index c1cca7b438c3..dc894a45bbcc 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -273,14 +273,29 @@ const fn offset_valid<U>(offset: usize, size:
> usize) -> bool {
> }
> }
>
> -/// Marker trait indicating that an I/O backend supports operations
> of a certain type.
> +/// Trait indicating that an I/O backend supports operations of a
> certain type and providing an
> +/// implementation for these operations.
> ///
> /// Different I/O backends can implement this trait to expose only
> the operations they support.
> ///
> /// For example, a PCI configuration space may implement
> `IoCapable<u8>`, `IoCapable<u16>`,
> /// and `IoCapable<u32>`, but not `IoCapable<u64>`, while an MMIO
> region on a 64-bit
> /// system might implement all four.
> -pub trait IoCapable<T> {}
> +pub trait IoCapable<T> {
> + /// Performs an I/O read of type `T` at `address` and returns
> the result.
> + ///
> + /// # Safety
> + ///
> + /// The range `[address..address + size_of::<T>()]` must be
> within the bounds of `Self`.
> + 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`.
> + unsafe fn io_write(&self, value: T, address: usize);
> +}
>
> /// Types implementing this trait (e.g. MMIO BARs or PCI config
> regions)
> /// can perform I/O operations on regions of memory.
> @@ -322,146 +337,198 @@ fn io_addr<U>(&self, offset: usize) ->
> Result<usize> {
>
> /// Fallible 8-bit read with runtime bounds check.
> #[inline(always)]
> - fn try_read8(&self, _offset: usize) -> Result<u8>
> + fn try_read8(&self, offset: usize) -> Result<u8>
> where
> Self: IoCapable<u8>,
> {
> - build_error!("Backend does not support fallible 8-bit read")
> + let address = self.io_addr::<u8>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + Ok(unsafe { self.io_read(address) })
> }
>
> /// Fallible 16-bit read with runtime bounds check.
> #[inline(always)]
> - fn try_read16(&self, _offset: usize) -> Result<u16>
> + fn try_read16(&self, offset: usize) -> Result<u16>
> where
> Self: IoCapable<u16>,
> {
> - build_error!("Backend does not support fallible 16-bit
> read")
> + let address = self.io_addr::<u16>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + Ok(unsafe { self.io_read(address) })
> }
>
> /// Fallible 32-bit read with runtime bounds check.
> #[inline(always)]
> - fn try_read32(&self, _offset: usize) -> Result<u32>
> + fn try_read32(&self, offset: usize) -> Result<u32>
> where
> Self: IoCapable<u32>,
> {
> - build_error!("Backend does not support fallible 32-bit
> read")
> + let address = self.io_addr::<u32>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + Ok(unsafe { self.io_read(address) })
> }
>
> /// Fallible 64-bit read with runtime bounds check.
> #[inline(always)]
> - fn try_read64(&self, _offset: usize) -> Result<u64>
> + fn try_read64(&self, offset: usize) -> Result<u64>
> where
> Self: IoCapable<u64>,
> {
> - build_error!("Backend does not support fallible 64-bit
> read")
> + let address = self.io_addr::<u64>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + Ok(unsafe { self.io_read(address) })
> }
>
> /// Fallible 8-bit write with runtime bounds check.
> #[inline(always)]
> - fn try_write8(&self, _value: u8, _offset: usize) -> Result
> + fn try_write8(&self, value: u8, offset: usize) -> Result
> where
> Self: IoCapable<u8>,
> {
> - build_error!("Backend does not support fallible 8-bit
> write")
> + let address = self.io_addr::<u8>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + unsafe { self.io_write(value, address) };
> + Ok(())
> }
>
> /// Fallible 16-bit write with runtime bounds check.
> #[inline(always)]
> - fn try_write16(&self, _value: u16, _offset: usize) -> Result
> + fn try_write16(&self, value: u16, offset: usize) -> Result
> where
> Self: IoCapable<u16>,
> {
> - build_error!("Backend does not support fallible 16-bit
> write")
> + let address = self.io_addr::<u16>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + unsafe { self.io_write(value, address) };
> + Ok(())
> }
>
> /// Fallible 32-bit write with runtime bounds check.
> #[inline(always)]
> - fn try_write32(&self, _value: u32, _offset: usize) -> Result
> + fn try_write32(&self, value: u32, offset: usize) -> Result
> where
> Self: IoCapable<u32>,
> {
> - build_error!("Backend does not support fallible 32-bit
> write")
> + let address = self.io_addr::<u32>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + unsafe { self.io_write(value, address) };
> + Ok(())
> }
>
> /// Fallible 64-bit write with runtime bounds check.
> #[inline(always)]
> - fn try_write64(&self, _value: u64, _offset: usize) -> Result
> + fn try_write64(&self, value: u64, offset: usize) -> Result
> where
> Self: IoCapable<u64>,
> {
> - build_error!("Backend does not support fallible 64-bit
> write")
> + let address = self.io_addr::<u64>(offset)?;
> +
> + // SAFETY: `address` has been validated by `io_addr`.
> + unsafe { self.io_write(value, address) };
> + Ok(())
> }
>
> /// Infallible 8-bit read with compile-time bounds check.
> #[inline(always)]
> - fn read8(&self, _offset: usize) -> u8
> + fn read8(&self, offset: usize) -> u8
> where
> Self: IoKnownSize + IoCapable<u8>,
> {
> - build_error!("Backend does not support infallible 8-bit
> read")
> + let address = self.io_addr_assert::<u8>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_read(address) }
> }
>
> /// Infallible 16-bit read with compile-time bounds check.
> #[inline(always)]
> - fn read16(&self, _offset: usize) -> u16
> + fn read16(&self, offset: usize) -> u16
> where
> Self: IoKnownSize + IoCapable<u16>,
> {
> - build_error!("Backend does not support infallible 16-bit
> read")
> + let address = self.io_addr_assert::<u16>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_read(address) }
> }
>
> /// Infallible 32-bit read with compile-time bounds check.
> #[inline(always)]
> - fn read32(&self, _offset: usize) -> u32
> + fn read32(&self, offset: usize) -> u32
> where
> Self: IoKnownSize + IoCapable<u32>,
> {
> - build_error!("Backend does not support infallible 32-bit
> read")
> + let address = self.io_addr_assert::<u32>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_read(address) }
> }
>
> /// Infallible 64-bit read with compile-time bounds check.
> #[inline(always)]
> - fn read64(&self, _offset: usize) -> u64
> + fn read64(&self, offset: usize) -> u64
> where
> Self: IoKnownSize + IoCapable<u64>,
> {
> - build_error!("Backend does not support infallible 64-bit
> read")
> + let address = self.io_addr_assert::<u64>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_read(address) }
> }
>
> /// Infallible 8-bit write with compile-time bounds check.
> #[inline(always)]
> - fn write8(&self, _value: u8, _offset: usize)
> + fn write8(&self, value: u8, offset: usize)
> where
> Self: IoKnownSize + IoCapable<u8>,
> {
> - build_error!("Backend does not support infallible 8-bit
> write")
> + let address = self.io_addr_assert::<u8>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_write(value, address) }
> }
>
> /// Infallible 16-bit write with compile-time bounds check.
> #[inline(always)]
> - fn write16(&self, _value: u16, _offset: usize)
> + fn write16(&self, value: u16, offset: usize)
> where
> Self: IoKnownSize + IoCapable<u16>,
> {
> - build_error!("Backend does not support infallible 16-bit
> write")
> + let address = self.io_addr_assert::<u16>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_write(value, address) }
> }
>
> /// Infallible 32-bit write with compile-time bounds check.
> #[inline(always)]
> - fn write32(&self, _value: u32, _offset: usize)
> + fn write32(&self, value: u32, offset: usize)
> where
> Self: IoKnownSize + IoCapable<u32>,
> {
> - build_error!("Backend does not support infallible 32-bit
> write")
> + let address = self.io_addr_assert::<u32>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_write(value, address) }
> }
>
> /// Infallible 64-bit write with compile-time bounds check.
> #[inline(always)]
> - fn write64(&self, _value: u64, _offset: usize)
> + fn write64(&self, value: u64, offset: usize)
> where
> Self: IoKnownSize + IoCapable<u64>,
> {
> - build_error!("Backend does not support infallible 64-bit
> write")
> + let address = self.io_addr_assert::<u64>(offset);
> +
> + // SAFETY: `address` has been validated by `io_addr_assert`.
> + unsafe { self.io_write(value, address) }
> }
> }
>
> @@ -487,14 +554,36 @@ fn io_addr_assert<U>(&self, offset: usize) ->
> usize {
> }
> }
>
> -// MMIO regions support 8, 16, and 32-bit accesses.
> -impl<const SIZE: usize> IoCapable<u8> for Mmio<SIZE> {}
> -impl<const SIZE: usize> IoCapable<u16> for Mmio<SIZE> {}
> -impl<const SIZE: usize> IoCapable<u32> for Mmio<SIZE> {}
> +/// Implements [`IoCapable`] on `$mmio` for `$ty` using `$read_fn`
> and `$write_fn`.
> +macro_rules! impl_mmio_io_capable {
> + ($mmio:ident, $(#[$attr:meta])* $ty:ty, $read_fn:ident,
> $write_fn:ident) => {
> + $(#[$attr])*
> + impl<const SIZE: usize> IoCapable<$ty> for $mmio<SIZE> {
> + unsafe fn io_read(&self, address: usize) -> $ty {
> + // SAFETY: By the trait invariant `address` is a
> valid address for MMIO operations.
> + unsafe { bindings::$read_fn(address as *const
> c_void) }
> + }
>
> + unsafe fn io_write(&self, value: $ty, address: usize) {
> + // SAFETY: By the trait invariant `address` is a
> valid address for MMIO operations.
> + unsafe { bindings::$write_fn(value, address as *mut
> c_void) }
> + }
> + }
> + };
> +}
> +
> +// MMIO regions support 8, 16, and 32-bit accesses.
> +impl_mmio_io_capable!(Mmio, u8, readb, writeb);
> +impl_mmio_io_capable!(Mmio, u16, readw, writew);
> +impl_mmio_io_capable!(Mmio, u32, readl, writel);
> // MMIO regions on 64-bit systems also support 64-bit accesses.
> -#[cfg(CONFIG_64BIT)]
> -impl<const SIZE: usize> IoCapable<u64> for Mmio<SIZE> {}
> +impl_mmio_io_capable!(
> + Mmio,
> + #[cfg(CONFIG_64BIT)]
> + u64,
> + readq,
> + writeq
> +);
>
> impl<const SIZE: usize> Io for Mmio<SIZE> {
> /// Returns the base address of this mapping.
> diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs
> index 6ca4cf75594c..8c8aab2e3f22 100644
> --- a/rust/kernel/pci/io.rs
> +++ b/rust/kernel/pci/io.rs
> @@ -142,10 +142,41 @@ macro_rules! call_config_write {
> };
> }
>
> +/// Implements [`IoCapable`] on [`ConfigSpace`] for `$ty` using
> `$read_fn` and `$write_fn`.
> +macro_rules! impl_config_space_io_capable {
> + ($ty:ty, $read_fn:ident, $write_fn:ident) => {
> + impl<'a, S: ConfigSpaceKind> IoCapable<$ty> for
> ConfigSpace<'a, S> {
> + unsafe fn io_read(&self, address: usize) -> $ty {
> + let mut val: $ty = 0;
> +
> + // Return value from C function is ignored in
> infallible accessors.
> + let _ret =
> + // SAFETY: By the type invariant `self.pdev` is
> a valid address.
> + // CAST: The offset is cast to `i32` because the
> C functions expect a 32-bit
> + // signed offset parameter. PCI configuration
> space size is at most 4096 bytes,
> + // so the value always fits within `i32` without
> truncation or sign change.
> + unsafe { bindings::$read_fn(self.pdev.as_raw(),
> address as i32, &mut val) };
> +
> + val
> + }
> +
> + unsafe fn io_write(&self, value: $ty, address: usize) {
> + // Return value from C function is ignored in
> infallible accessors.
> + let _ret =
> + // SAFETY: By the type invariant `self.pdev` is
> a valid address.
> + // CAST: The offset is cast to `i32` because the
> C functions expect a 32-bit
> + // signed offset parameter. PCI configuration
> space size is at most 4096 bytes,
> + // so the value always fits within `i32` without
> truncation or sign change.
> + unsafe { bindings::$write_fn(self.pdev.as_raw(),
> address as i32, value) };
> + }
> + }
> + };
> +}
> +
> // PCI configuration space supports 8, 16, and 32-bit accesses.
> -impl<'a, S: ConfigSpaceKind> IoCapable<u8> for ConfigSpace<'a, S> {}
> -impl<'a, S: ConfigSpaceKind> IoCapable<u16> for ConfigSpace<'a, S>
> {}
> -impl<'a, S: ConfigSpaceKind> IoCapable<u32> for ConfigSpace<'a, S>
> {}
> +impl_config_space_io_capable!(u8, pci_read_config_byte,
> pci_write_config_byte);
> +impl_config_space_io_capable!(u16, pci_read_config_word,
> pci_write_config_word);
> +impl_config_space_io_capable!(u32, pci_read_config_dword,
> pci_write_config_dword);
>
> impl<'a, S: ConfigSpaceKind> Io for ConfigSpace<'a, S> {
> /// Returns the base address of the I/O region. It is always 0
> for configuration space.
Powered by blists - more mailing lists