| 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: <20250918181357.GA1825487@joelbox2>
Date: Thu, 18 Sep 2025 14:13:57 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Danilo Krummrich <dakr@...nel.org>, 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>,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH] rust: io: use const generics for read/write offsets
On Thu, Sep 18, 2025 at 03:02:11PM +0000, Alice Ryhl wrote:
> Using build_assert! to assert that offsets are in bounds is really
> fragile and likely to result in spurious and hard-to-debug build
> failures. Therefore, build_assert! should be avoided for this case.
> Thus, update the code to perform the check in const evaluation instead.
I really don't think this patch is a good idea (and nobody I spoke to thinks
so). Not only does it mess up the user's caller syntax completely, it is also
super confusing to pass both a generic and a function argument separately.
Sorry if I knew this would be the syntax, I would have objected even when we
spoke :)
I think the best fix (from any I've seen so far), is to move the bindings
calls of offending code into a closure and call the closure directly, as I
posted in the other thread. I also passed the closure idea by Gary and he
confirmed the compiler should behave correctly (I will check the code gen
with/without later). Gary also provided a brilliant suggestion that we can
call the closure directly instead of assigning it to a variable first. That
fix is also smaller, and does not screw up the users. APIs should fix issues
within them instead of relying on user to work around them.
So from my side, NAK. But I do appreciate you taking a look!
thanks,
- Joel
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> drivers/gpu/drm/tyr/regs.rs | 4 ++--
> rust/kernel/devres.rs | 4 ++--
> rust/kernel/io.rs | 18 ++++++++++--------
> rust/kernel/io/mem.rs | 6 +++---
> samples/rust/rust_driver_pci.rs | 10 +++++-----
> 5 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
> index f46933aaa2214ee0ac58b1ea2a6aa99506a35b70..e3c306e48e86d1d6047cab7944e0fe000901d48b 100644
> --- a/drivers/gpu/drm/tyr/regs.rs
> +++ b/drivers/gpu/drm/tyr/regs.rs
> @@ -25,13 +25,13 @@
> impl<const OFFSET: usize> Register<OFFSET> {
> #[inline]
> pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
> - let value = (*iomem).access(dev)?.read32(OFFSET);
> + let value = (*iomem).access(dev)?.read32::<OFFSET>();
> Ok(value)
> }
>
> #[inline]
> pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
> - (*iomem).access(dev)?.write32(value, OFFSET);
> + (*iomem).access(dev)?.write32::<OFFSET>(value);
> Ok(())
> }
> }
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index da18091143a67fcfbb247e7cb4f59f5a4932cac5..3e66e10c05fa078e42162c7a367161fbf735a07f 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -96,7 +96,7 @@ struct Inner<T: Send> {
> /// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
> ///
> /// let res = devres.try_access().ok_or(ENXIO)?;
> -/// res.write8(0x42, 0x0);
> +/// res.write8::<0x0>(0x42);
> /// # Ok(())
> /// # }
> /// ```
> @@ -232,7 +232,7 @@ pub fn device(&self) -> &Device {
> ///
> /// // might_sleep()
> ///
> - /// bar.write32(0x42, 0x0);
> + /// bar.write32::<0x0>(0x42);
> ///
> /// Ok(())
> /// }
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 03b467722b8651ebecd660ac0e2d849cf88dc915..563ff8488100d9e07a7f4bffeb085db7bd7e9d6a 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -103,7 +103,7 @@ pub fn maxsize(&self) -> usize {
> ///# fn no_run() -> Result<(), Error> {
> /// // SAFETY: Invalid usage for example purposes.
> /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
> -/// iomem.write32(0x42, 0x0);
> +/// iomem.write32::<0x0>(0x42);
> /// assert!(iomem.try_write32(0x42, 0x0).is_ok());
> /// assert!(iomem.try_write32(0x42, 0x4).is_err());
> /// # Ok(())
> @@ -120,8 +120,8 @@ macro_rules! define_read {
> /// time, the build will fail.
> $(#[$attr])*
> #[inline]
> - pub fn $name(&self, offset: usize) -> $type_name {
> - let addr = self.io_addr_assert::<$type_name>(offset);
> + pub fn $name<const OFF: usize>(&self) -> $type_name {
> + let addr = self.io_addr_assert::<$type_name, OFF>();
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> unsafe { bindings::$c_fn(addr as *const c_void) }
> @@ -149,8 +149,8 @@ macro_rules! define_write {
> /// time, the build will fail.
> $(#[$attr])*
> #[inline]
> - pub fn $name(&self, value: $type_name, offset: usize) {
> - let addr = self.io_addr_assert::<$type_name>(offset);
> + pub fn $name<const OFF: usize>(&self, value: $type_name) {
> + let addr = self.io_addr_assert::<$type_name, OFF>();
>
> // SAFETY: By the type invariant `addr` is a valid address for MMIO operations.
> unsafe { bindings::$c_fn(value, addr as *mut c_void) }
> @@ -217,10 +217,12 @@ fn io_addr<U>(&self, offset: usize) -> Result<usize> {
> }
>
> #[inline]
> - fn io_addr_assert<U>(&self, offset: usize) -> usize {
> - build_assert!(Self::offset_valid::<U>(offset, SIZE));
> + fn io_addr_assert<U, const OFF: usize>(&self) -> usize {
> + const {
> + build_assert!(Self::offset_valid::<U>(OFF, SIZE));
> + }
>
> - self.addr() + offset
> + self.addr() + OFF
> }
>
> define_read!(read8, try_read8, readb -> u8);
> diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs
> index 6f99510bfc3a63dd72c1d47dc661dcd48fa7f54e..b73557f5f57c955ac251a46c9bdd6df0687411e2 100644
> --- a/rust/kernel/io/mem.rs
> +++ b/rust/kernel/io/mem.rs
> @@ -54,7 +54,7 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
> /// pdev: &platform::Device<Core>,
> /// info: Option<&Self::IdInfo>,
> /// ) -> Result<Pin<KBox<Self>>> {
> - /// let offset = 0; // Some offset.
> + /// const OFFSET: usize = 0; // Some offset.
> ///
> /// // If the size is known at compile time, use [`Self::iomap_sized`].
> /// //
> @@ -66,9 +66,9 @@ pub(crate) unsafe fn new(device: &'a Device<Bound>, resource: &'a Resource) -> S
> /// let io = iomem.access(pdev.as_ref())?;
> ///
> /// // Read and write a 32-bit value at `offset`.
> - /// let data = io.read32_relaxed(offset);
> + /// let data = io.read32_relaxed::<OFFSET>();
> ///
> - /// io.write32_relaxed(data, offset);
> + /// io.write32_relaxed::<OFFSET>(data);
> ///
> /// # Ok(KBox::new(SampleDriver, GFP_KERNEL)?.into())
> /// }
> diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
> index 606946ff4d7fd98e206ee6420a620d1c44eb0377..6f0388853e2b36e0800df5125a5dd8b20a6d5912 100644
> --- a/samples/rust/rust_driver_pci.rs
> +++ b/samples/rust/rust_driver_pci.rs
> @@ -46,17 +46,17 @@ struct SampleDriver {
> impl SampleDriver {
> fn testdev(index: &TestIndex, bar: &Bar0) -> Result<u32> {
> // Select the test.
> - bar.write8(index.0, Regs::TEST);
> + bar.write8::<{ Regs::TEST }>(index.0);
>
> - let offset = u32::from_le(bar.read32(Regs::OFFSET)) as usize;
> - let data = bar.read8(Regs::DATA);
> + let offset = u32::from_le(bar.read32::<{ Regs::OFFSET }>()) as usize;
> + let data = bar.read8::<{ Regs::DATA }>();
>
> // Write `data` to `offset` to increase `count` by one.
> //
> // Note that we need `try_write8`, since `offset` can't be checked at compile-time.
> bar.try_write8(data, offset)?;
>
> - Ok(bar.read32(Regs::COUNT))
> + Ok(bar.read32::<{ Regs::COUNT }>())
> }
> }
>
> @@ -98,7 +98,7 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
> fn unbind(pdev: &pci::Device<Core>, this: Pin<&Self>) {
> if let Ok(bar) = this.bar.access(pdev.as_ref()) {
> // Reset pci-testdev by writing a new test index.
> - bar.write8(this.index.0, Regs::TEST);
> + bar.write8::<{ Regs::TEST }>(this.index.0);
> }
> }
> }
>
> ---
> base-commit: cf4fd52e323604ccfa8390917593e1fb965653ee
> change-id: 20250918-write-offset-const-0b231c4282ea
>
> Best regards,
> --
> Alice Ryhl <aliceryhl@...gle.com>
>
Powered by blists - more mailing lists