Re: [PATCH v5 6/6] rust: use strict provenance APIs

From: Benno Lossin
Date: Tue Mar 18 2025 - 05:25:04 EST

Next message: sw617.shin: "RE: [PATCH v2] watchdog: s3c2410_wdt: Fix PMU register bits for ExynosAutoV920 SoC"
Previous message: Frederic Weisbecker: "[PATCH] rcu/nocb: Dump gp state even if rdp gp itself is not offloaded"
In reply to: Tamir Duberstein: "Re: [PATCH v5 6/6] rust: use strict provenance APIs"
Next in thread: Boqun Feng: "Re: [PATCH v5 6/6] rust: use strict provenance APIs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue Mar 18, 2025 at 1:11 AM CET, Boqun Feng wrote:
> On Mon, Mar 17, 2025 at 02:36:08PM -0700, Boqun Feng wrote:
> [...]
>> >
>> > What about `pointer::expose_provenance`? It's a method that was added in 1.79.0.
>> >
>>
>> We have a few options:
>>
>> 1) we can decide to use funtion-version of expose_provenance() (i.e. the
>> stub), if we feel the symmetry with with_exposed_provenance() is
>> a strong rationale. This also means we won't likely use
>> pointer::expose_provenance() in the future. That is, although kernel
>> doesn't have stable internal API, but in the foreseeable future, we
>> decide to use funtion-version of expose_provenance().
>>
>> 2) we can introduce a PtrExt trait for <1.79
>>
>> pub trait PtrExt<T> {
>> fn expose_provenance(self) -> usize;
>> }
>>
>> and
>>
>> impl<T> PtrExt<T> for *const T {
>> ...
>> }
>>
>> and `PtrExt` in kernel::prelude.
>>
>> (we need to #[allow(unstable_name_collisions)] to make that work)
>>
>> We can also make with_exposed_provenance() use the same *Ext trick,
>> and remove it when we bump the minimal rustc version.
>
> This is probably a wrong suggestion, because with_exposed_provenance()
> is a function instead of a method in Rust std.
>
> Below is what I combined all together (based on your v5 patchset), and I
> did test on 1.78, 1.79, 1.84 and 1.85 and it seems working ;-)

I like this a lot, I also thought that we should just disable the
`incompatible_msrv` lint. That we get rid of the stubs is a nice bonus
:)

The only annoying thing that's left IMO is that we need to conditionally
import the `PtrExt` trait, but that would need the built-in prelude
feature :(

Couple notes below.

> Regards,
> Boqun
> ------------------------------------->8
> diff --git a/init/Kconfig b/init/Kconfig
> index 82e28d6f7c3f..e316b98b3612 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -135,6 +135,9 @@ config RUSTC_HAS_COERCE_POINTEE
> config RUSTC_HAS_STABLE_STRICT_PROVENANCE
> def_bool RUSTC_VERSION >= 108400
>
> +config RUSTC_HAS_EXPOSED_PROVENANCE
> + def_bool RUSTC_VERSION >= 107900
> +
> config PAHOLE_VERSION
> int
> default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index e8232bb771b2..a87a437bd9ab 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -64,7 +64,7 @@ struct DevresInner<T> {
> /// return Err(ENOMEM);
> /// }
> ///
> -/// Ok(IoMem(IoRaw::new(kernel::expose_provenance(addr), SIZE)?))
> +/// Ok(IoMem(IoRaw::new(addr.expose_provenance(), SIZE)?))
> /// }
> /// }
> ///
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 0a018ad7478a..60c71f26d29d 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -75,7 +75,7 @@ pub fn maxsize(&self) -> usize {
> /// return Err(ENOMEM);
> /// }
> ///
> -/// Ok(IoMem(IoRaw::new(kernel::expose_provenance(addr), SIZE)?))
> +/// Ok(IoMem(IoRaw::new(addr.expose_provenance(), SIZE)?))
> /// }
> /// }
> ///
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index c1b274c04a0f..79b19e601372 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -22,6 +22,9 @@
> feature(strict_provenance_lints),
> deny(fuzzy_provenance_casts, lossy_provenance_casts)
> )]
> +#![cfg_attr(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE), feature(strict_provenance))]
> +#![cfg_attr(all(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE), CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE), feature(exposed_provenance))]
> +
> #![feature(inline_const)]
> #![feature(lint_reasons)]
> // Stable in Rust 1.83
> @@ -30,78 +33,24 @@
> #![feature(const_ptr_write)]
> #![feature(const_refs_to_cell)]
>
> -#[cfg(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE)]
> -#[allow(clippy::incompatible_msrv)]
> -mod strict_provenance {
> - /// Gets the "address" portion of the pointer.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.addr.
> - #[inline]
> - pub fn addr<T>(ptr: *const T) -> usize {
> - ptr.addr()
> - }
> -
> - /// Exposes the "provenance" part of the pointer for future use in
> - /// [`with_exposed_provenance`] and returns the "address" portion.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.expose_provenance.
> - #[inline]
> - pub fn expose_provenance<T>(ptr: *const T) -> usize {
> - ptr.expose_provenance()
> - }
> -
> - /// Converts an address back to a pointer, picking up some previously 'exposed'
> - /// provenance.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/ptr/fn.with_exposed_provenance.html.
> - #[inline]
> - pub fn with_exposed_provenance<T>(addr: usize) -> *const T {
> - core::ptr::with_exposed_provenance(addr)
> - }
> -
> - /// Converts an address back to a mutable pointer, picking up some previously 'exposed'
> - /// provenance.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/ptr/fn.with_exposed_provenance_mut.html
> - #[inline]
> - pub fn with_exposed_provenance_mut<T>(addr: usize) -> *mut T {
> - core::ptr::with_exposed_provenance_mut(addr)
> - }
> -
> - /// Creates a pointer with the given address and no [provenance][crate::ptr#provenance].
> - ///
> - /// See https://doc.rust-lang.org/stable/core/ptr/fn.without_provenance_mut.html.
> - #[inline]
> - pub fn without_provenance_mut<T>(addr: usize) -> *mut T {
> - core::ptr::without_provenance_mut(addr)
> - }
> -}
> +#![allow(clippy::incompatible_msrv)]
>
> -#[cfg(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE))]
> +#[cfg(not(CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE))]
> mod strict_provenance {

Since there is only a single trait and impl in here, I think we don't
need a module.

> - /// Gets the "address" portion of the pointer.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.addr.
> - #[inline]
> - pub fn addr<T>(ptr: *const T) -> usize {
> - // This is core's implementation from
> - // https://github.com/rust-lang/rust/commit/4291332175d12e79e6061cdc3f5dccac2e28b969 through
> - // https://github.com/rust-lang/rust/blob/1.84.0/library/core/src/ptr/const_ptr.rs#L172
> - // which is the first version that satisfies `CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE`.
> - #[allow(clippy::undocumented_unsafe_blocks)]
> - unsafe {
> - #[allow(clippy::transmutes_expressible_as_ptr_casts)]
> - core::mem::transmute(ptr.cast::<()>())
> - }
> + #[doc(hidden)]
> + pub trait PtrExt<T> {

The `T` here and in the impl below probably should have a `?Sized`
bound, since that's also what the stdlib does.

> + /// Exposes the "provenance" part of the pointer for future use in
> + /// [`with_exposed_provenance`] and returns the "address" portion.
> + ///
> + /// See https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.expose_provenance.
> + fn expose_provenance(self) -> usize;
> }
>
> - /// Exposes the "provenance" part of the pointer for future use in
> - /// [`with_exposed_provenance`] and returns the "address" portion.
> - ///
> - /// See https://doc.rust-lang.org/stable/core/primitive.pointer.html#method.expose_provenance.
> - #[inline]
> - pub fn expose_provenance<T>(ptr: *const T) -> usize {
> - ptr.cast::<()>() as usize
> + impl<T> PtrExt<T> for *const T {
> + #[inline]
> + fn expose_provenance(self) -> usize {
> + self.cast::<()>() as usize
> + }
> }
>
> /// Converts an address back to a pointer, picking up some previously 'exposed'
> @@ -131,8 +80,12 @@ pub fn without_provenance_mut<T>(addr: usize) -> *mut T {
> }
> }
>
> +#[cfg(not(CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE))]
> pub use strict_provenance::*;
>
> +#[cfg(CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE)]
> +pub use core::ptr::{with_exposed_provenance, with_exposed_provenance_mut, without_provenance_mut};

We shouldn't need this any longer, right?

---
Cheers,
Benno

> +
> // Ensure conditional compilation based on the kernel configuration works;
> // otherwise we may silently break things like initcall handling.
> #[cfg(not(CONFIG_RUST))]
> diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs
> index b70076d16008..3670676071ff 100644
> --- a/rust/kernel/of.rs
> +++ b/rust/kernel/of.rs
> @@ -22,7 +22,7 @@ unsafe impl RawDeviceId for DeviceId {
> const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::of_device_id, data);
>
> fn index(&self) -> usize {
> - crate::addr(self.0.data)
> + self.0.data.addr()
> }
> }
>
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 87c9f67b3f0f..73958abdc522 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -287,7 +287,7 @@ fn new(pdev: Device, num: u32, name: &CStr) -> Result<Self> {
> // `pdev` is valid by the invariants of `Device`.
> // `num` is checked for validity by a previous call to `Device::resource_len`.
> // `name` is always valid.
> - let ioptr = crate::expose_provenance(unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) });
> + let ioptr = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) }.expose_provenance();
> if ioptr == 0 {
> // SAFETY:
> // `pdev` valid by the invariants of `Device`.
> diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
> index baa774a351ce..3ea6aa9e40e5 100644
> --- a/rust/kernel/prelude.rs
> +++ b/rust/kernel/prelude.rs
> @@ -41,3 +41,6 @@
> pub use super::init::InPlaceInit;
>
> pub use super::current;
> +
> +#[cfg(not(CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE))]
> +pub use super::PtrExt;
> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 6bc6357293e4..d8e740267f14 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -8,6 +8,9 @@
>
> use crate::error::{code::*, Error};
>
> +#[cfg(not(CONFIG_RUSTC_HAS_EXPOSED_PROVENANCE))]
> +use crate::PtrExt;
> +
> /// Byte string without UTF-8 validity guarantee.
> #[repr(transparent)]
> pub struct BStr([u8]);
> @@ -692,9 +695,9 @@ fn new() -> Self {
> pub(crate) unsafe fn from_ptrs(pos: *mut u8, end: *mut u8) -> Self {
> // INVARIANT: The safety requirements guarantee the type invariants.
> Self {
> - beg: crate::expose_provenance(pos),
> - pos: crate::expose_provenance(pos),
> - end: crate::expose_provenance(end),
> + beg: pos.expose_provenance(),
> + pos: pos.expose_provenance(),
> + end: end.expose_provenance(),
> }
> }
>
> @@ -705,7 +708,7 @@ pub(crate) unsafe fn from_ptrs(pos: *mut u8, end: *mut u8) -> Self {
> /// The memory region starting at `buf` and extending for `len` bytes must be valid for writes
> /// for the lifetime of the returned [`RawFormatter`].
> pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self {
> - let pos = crate::expose_provenance(buf);
> + let pos = buf.expose_provenance();
> // INVARIANT: We ensure that `end` is never less then `buf`, and the safety requirements
> // guarantees that the memory region is valid for writes.
> Self {
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 08b6380933f5..b070da0ea972 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -226,7 +226,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
> # Compile Rust sources (.rs)
> # ---------------------------------------------------------------------------
>
> -rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons
> +rust_allowed_features := asm_const,asm_goto,arbitrary_self_types,lint_reasons,exposed_provenance
>
> # `--out-dir` is required to avoid temporaries being created by `rustc` in the
> # current working directory, which may be not accessible in the out-of-tree
> diff --git a/scripts/rustdoc_test_gen.rs b/scripts/rustdoc_test_gen.rs
> index 036635fb1621..331ed32adc35 100644
> --- a/scripts/rustdoc_test_gen.rs
> +++ b/scripts/rustdoc_test_gen.rs
> @@ -224,6 +224,8 @@ macro_rules! assert_eq {{
> BufWriter::new(File::create("rust/doctests_kernel_generated.rs").unwrap()),
> r#"//! `kernel` crate documentation tests.
>
> +#![allow(clippy::incompatible_msrv)]
> +
> const __LOG_PREFIX: &[u8] = b"rust_doctests_kernel\0";
>
> {rust_tests}

Next message: sw617.shin: "RE: [PATCH v2] watchdog: s3c2410_wdt: Fix PMU register bits for ExynosAutoV920 SoC"
Previous message: Frederic Weisbecker: "[PATCH] rcu/nocb: Dump gp state even if rdp gp itself is not offloaded"
In reply to: Tamir Duberstein: "Re: [PATCH v5 6/6] rust: use strict provenance APIs"
Next in thread: Boqun Feng: "Re: [PATCH v5 6/6] rust: use strict provenance APIs"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]