| Message ID | 20250317-ptr-as-ptr-v5-6-5b5f21fa230a@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: reduce pointer casts, enable related lints | expand |
On Mon, Mar 17, 2025 at 10:24 AM Tamir Duberstein <tamird@gmail.com> wrote: > > Throughout the tree, use the strict provenance APIs stabilized in Rust > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > functions at the `kernel` crate root along with polyfills for rustc < > 1.84.0. > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > 1.84.0 as our MSRV is 1.78.0. > > In the `kernel` crate, enable the strict provenance lints on rustc >= > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > compiler flags that are dependent on the rustc version in use. As Benno pointed out on v4, this should probably include: Note that the enablement of the strict provenance lints does not extend to the `kernel` crate's doctests.
On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: [...] > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index fc6835cc36a3..c1b274c04a0f 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -17,6 +17,11 @@ > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > +#![cfg_attr( > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > + feature(strict_provenance_lints), > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > +)] > #![feature(inline_const)] > #![feature(lint_reasons)] > // Stable in Rust 1.83 > @@ -25,6 +30,109 @@ > #![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() > + } > + For addr(), I would just enable feature(strict_provenance) if CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is available for 1.78. Plus we may need with_addr() or map_addr() in the future. It saves the cost of maintaining our own *addr() and removing it when we bump to a strict_provenance stablized version as minimal verision in the future. Thoughts? Regards, Boqun [...] > // 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 40d1bd13682c..b70076d16008 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 { > - self.0.data as usize > + crate::addr(self.0.data) > } > } > [...]
On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote: > Throughout the tree, use the strict provenance APIs stabilized in Rust > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > functions at the `kernel` crate root along with polyfills for rustc < > 1.84.0. > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > 1.84.0 as our MSRV is 1.78.0. > > In the `kernel` crate, enable the strict provenance lints on rustc >= > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > compiler flags that are dependent on the rustc version in use. > > Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > Signed-off-by: Tamir Duberstein <tamird@gmail.com> One comment below, with that fixed: Reviewed-by: Benno Lossin <benno.lossin@proton.me> > --- > init/Kconfig | 3 ++ > rust/kernel/alloc.rs | 2 +- > rust/kernel/devres.rs | 4 +- > rust/kernel/io.rs | 14 +++---- > rust/kernel/lib.rs | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ > rust/kernel/of.rs | 2 +- > rust/kernel/pci.rs | 4 +- > rust/kernel/str.rs | 16 +++----- > rust/kernel/uaccess.rs | 12 ++++-- > 9 files changed, 138 insertions(+), 27 deletions(-) > +#[cfg(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE))] > +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 { > + // 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::<()>()) > + } I think we should just use `ptr as usize` here instead. It's going away at some point and it will only affect optimizations (I don't even know if they exist at the moment) of old versions. --- Cheers, Benno > + } > + > + /// 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 > + } > + > + /// 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 { > + addr as *const T > + } > + > + /// 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 { > + addr as *mut T > + } > + > + /// 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 { > + addr as *mut T > + } > +} > + > +pub use strict_provenance::*; > + > // Ensure conditional compilation based on the kernel configuration works; > // otherwise we may silently break things like initcall handling. > #[cfg(not(CONFIG_RUST))]
On Mon, Mar 17, 2025 at 1:39 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > [...] > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > index fc6835cc36a3..c1b274c04a0f 100644 > > --- a/rust/kernel/lib.rs > > +++ b/rust/kernel/lib.rs > > @@ -17,6 +17,11 @@ > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > > +#![cfg_attr( > > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > > + feature(strict_provenance_lints), > > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > > +)] > > #![feature(inline_const)] > > #![feature(lint_reasons)] > > // Stable in Rust 1.83 > > @@ -25,6 +30,109 @@ > > #![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() > > + } > > + > > For addr(), I would just enable feature(strict_provenance) if > CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is > available for 1.78. Plus we may need with_addr() or map_addr() in the > future. We still need these stubs to avoid `clippy::incompatible_msrv`, and we'll need those until MSRV is above 1.84. > > It saves the cost of maintaining our own *addr() and removing it when > we bump to a strict_provenance stablized version as minimal verision in > the future. Thoughts? > I can do this by making this particular stub unconditional. I'll do that.
On Mon, Mar 17, 2025 at 02:04:34PM -0400, Tamir Duberstein wrote: > On Mon, Mar 17, 2025 at 1:39 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > [...] > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > > index fc6835cc36a3..c1b274c04a0f 100644 > > > --- a/rust/kernel/lib.rs > > > +++ b/rust/kernel/lib.rs > > > @@ -17,6 +17,11 @@ > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > > > +#![cfg_attr( > > > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > > > + feature(strict_provenance_lints), > > > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > > > +)] > > > #![feature(inline_const)] > > > #![feature(lint_reasons)] > > > // Stable in Rust 1.83 > > > @@ -25,6 +30,109 @@ > > > #![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() > > > + } > > > + > > > > For addr(), I would just enable feature(strict_provenance) if > > CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is > > available for 1.78. Plus we may need with_addr() or map_addr() in the > > future. > > We still need these stubs to avoid `clippy::incompatible_msrv`, and > we'll need those until MSRV is above 1.84. > Hmm.. why? Clippy cannot work with unstable features? Regards, Boqun > > > > It saves the cost of maintaining our own *addr() and removing it when > > we bump to a strict_provenance stablized version as minimal verision in > > the future. Thoughts? > > > > I can do this by making this particular stub unconditional. I'll do that.
On Mon, Mar 17, 2025 at 2:06 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 02:04:34PM -0400, Tamir Duberstein wrote: > > On Mon, Mar 17, 2025 at 1:39 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > > [...] > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > > > index fc6835cc36a3..c1b274c04a0f 100644 > > > > --- a/rust/kernel/lib.rs > > > > +++ b/rust/kernel/lib.rs > > > > @@ -17,6 +17,11 @@ > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > > > > +#![cfg_attr( > > > > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > > > > + feature(strict_provenance_lints), > > > > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > > > > +)] > > > > #![feature(inline_const)] > > > > #![feature(lint_reasons)] > > > > // Stable in Rust 1.83 > > > > @@ -25,6 +30,109 @@ > > > > #![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() > > > > + } > > > > + > > > > > > For addr(), I would just enable feature(strict_provenance) if > > > CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is > > > available for 1.78. Plus we may need with_addr() or map_addr() in the > > > future. > > > > We still need these stubs to avoid `clippy::incompatible_msrv`, and > > we'll need those until MSRV is above 1.84. > > > > Hmm.. why? Clippy cannot work with unstable features? Yes, `clippy::incompatible_msrv` doesn't pay attention to enabled unstable features.
On Mon, Mar 17, 2025 at 02:10:42PM -0400, Tamir Duberstein wrote: > On Mon, Mar 17, 2025 at 2:06 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Mon, Mar 17, 2025 at 02:04:34PM -0400, Tamir Duberstein wrote: > > > On Mon, Mar 17, 2025 at 1:39 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > > > [...] > > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > > > > index fc6835cc36a3..c1b274c04a0f 100644 > > > > > --- a/rust/kernel/lib.rs > > > > > +++ b/rust/kernel/lib.rs > > > > > @@ -17,6 +17,11 @@ > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > > > > > +#![cfg_attr( > > > > > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > > > > > + feature(strict_provenance_lints), > > > > > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > > > > > +)] > > > > > #![feature(inline_const)] > > > > > #![feature(lint_reasons)] > > > > > // Stable in Rust 1.83 > > > > > @@ -25,6 +30,109 @@ > > > > > #![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() > > > > > + } > > > > > + > > > > > > > > For addr(), I would just enable feature(strict_provenance) if > > > > CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is > > > > available for 1.78. Plus we may need with_addr() or map_addr() in the > > > > future. > > > > > > We still need these stubs to avoid `clippy::incompatible_msrv`, and > > > we'll need those until MSRV is above 1.84. > > > > > > > Hmm.. why? Clippy cannot work with unstable features? > > Yes, `clippy::incompatible_msrv` doesn't pay attention to enabled > unstable features. Then we should fix clippy or how we set msrv rather adding the stub. @Miguel? Regards, Boqun
On Mon, Mar 17, 2025 at 1:50 PM Benno Lossin <benno.lossin@proton.me> wrote: > > On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote: > > Throughout the tree, use the strict provenance APIs stabilized in Rust > > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > > functions at the `kernel` crate root along with polyfills for rustc < > > 1.84.0. > > > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > > 1.84.0 as our MSRV is 1.78.0. > > > > In the `kernel` crate, enable the strict provenance lints on rustc >= > > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > > compiler flags that are dependent on the rustc version in use. > > > > Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > One comment below, with that fixed: > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > > --- > > init/Kconfig | 3 ++ > > rust/kernel/alloc.rs | 2 +- > > rust/kernel/devres.rs | 4 +- > > rust/kernel/io.rs | 14 +++---- > > rust/kernel/lib.rs | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ > > rust/kernel/of.rs | 2 +- > > rust/kernel/pci.rs | 4 +- > > rust/kernel/str.rs | 16 +++----- > > rust/kernel/uaccess.rs | 12 ++++-- > > 9 files changed, 138 insertions(+), 27 deletions(-) > > > > +#[cfg(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE))] > > +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 { > > + // 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::<()>()) > > + } > > I think we should just use `ptr as usize` here instead. It's going away > at some point and it will only affect optimizations (I don't even know > if they exist at the moment) of old versions. Why get cute? I'd rather defer to the standard library.
On Mon, Mar 17, 2025 at 2:31 PM Tamir Duberstein <tamird@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 1:50 PM Benno Lossin <benno.lossin@proton.me> wrote: > > > > On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote: > > > Throughout the tree, use the strict provenance APIs stabilized in Rust > > > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > > > functions at the `kernel` crate root along with polyfills for rustc < > > > 1.84.0. > > > > > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > > > 1.84.0 as our MSRV is 1.78.0. > > > > > > In the `kernel` crate, enable the strict provenance lints on rustc >= > > > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > > > compiler flags that are dependent on the rustc version in use. > > > > > > Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > > Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > One comment below, with that fixed: > > > > Reviewed-by: Benno Lossin <benno.lossin@proton.me> > > > > > --- > > > init/Kconfig | 3 ++ > > > rust/kernel/alloc.rs | 2 +- > > > rust/kernel/devres.rs | 4 +- > > > rust/kernel/io.rs | 14 +++---- > > > rust/kernel/lib.rs | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > rust/kernel/of.rs | 2 +- > > > rust/kernel/pci.rs | 4 +- > > > rust/kernel/str.rs | 16 +++----- > > > rust/kernel/uaccess.rs | 12 ++++-- > > > 9 files changed, 138 insertions(+), 27 deletions(-) > > > > > > > +#[cfg(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE))] > > > +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 { > > > + // 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::<()>()) > > > + } > > > > I think we should just use `ptr as usize` here instead. It's going away > > at some point and it will only affect optimizations (I don't even know > > if they exist at the moment) of old versions. > > Why get cute? I'd rather defer to the standard library. Ah, this is gone anyway with Boqun's suggestion - this function exists in 1.78.
On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 02:10:42PM -0400, Tamir Duberstein wrote: > > On Mon, Mar 17, 2025 at 2:06 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Mon, Mar 17, 2025 at 02:04:34PM -0400, Tamir Duberstein wrote: > > > > On Mon, Mar 17, 2025 at 1:39 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > > > > [...] > > > > > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > > > > > > index fc6835cc36a3..c1b274c04a0f 100644 > > > > > > --- a/rust/kernel/lib.rs > > > > > > +++ b/rust/kernel/lib.rs > > > > > > @@ -17,6 +17,11 @@ > > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] > > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] > > > > > > #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] > > > > > > +#![cfg_attr( > > > > > > + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, > > > > > > + feature(strict_provenance_lints), > > > > > > + deny(fuzzy_provenance_casts, lossy_provenance_casts) > > > > > > +)] > > > > > > #![feature(inline_const)] > > > > > > #![feature(lint_reasons)] > > > > > > // Stable in Rust 1.83 > > > > > > @@ -25,6 +30,109 @@ > > > > > > #![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() > > > > > > + } > > > > > > + > > > > > > > > > > For addr(), I would just enable feature(strict_provenance) if > > > > > CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE=n, because that feature is > > > > > available for 1.78. Plus we may need with_addr() or map_addr() in the > > > > > future. > > > > > > > > We still need these stubs to avoid `clippy::incompatible_msrv`, and > > > > we'll need those until MSRV is above 1.84. > > > > > > > > > > Hmm.. why? Clippy cannot work with unstable features? > > > > Yes, `clippy::incompatible_msrv` doesn't pay attention to enabled > > unstable features. > > Then we should fix clippy or how we set msrv rather adding the stub. > @Miguel? I filed https://github.com/rust-lang/rust-clippy/issues/14425.
On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > @Miguel? > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. I don't think we can wait for that to be fixed, though. Usually clippy is distributed with rustc via rustup, so even if this is eventually fixed, all versions between 1.84.0 and the fix will need this workaround until MSRV is >= 1.84.0.
On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > @Miguel? > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > I don't think we can wait for that to be fixed, though. Usually clippy > is distributed with rustc via rustup, so even if this is eventually > fixed, all versions between 1.84.0 and the fix will need this > workaround until MSRV is >= 1.84.0. We need to take one step back to evalute this "workaround". First, expose_provenance() and with_exposed_provenance{,_mut}() API are clearly defined as equavilent to `as` operation [1]. Therefore, the changes in this patch doing the conversion with expose_provenance() and with_exposed_provenance{,_mut}() don't change anything related to provenance in practice. I do agree we want to use the explicit provenance API, but I don't think we want to introduce some API that we know we will change them latter when we bump the rustc minimal version. So the question is: are these stubs what we want even though in the future our minimal rustc version stablizes provenance API? If not, then the cost of this patch cannot justify its benefits IMO. Now let's also look into why we choose a msrv for clippy, I would guess it's because we need to support all the versions of rustc starting at 1.78 and we want clippy to report a problem based on 1.78 even though we're using a higher version of rustc. But for this particular case, we use a feature that has already been stablized in a higher version of rustc, which means the problem reported by clippy doesn't help us, nor does it provide better code. Frankly speaking, I think we have other ways to ensure the support of all rustc versions without a msrv for clippy. If I was to choose, I would simply drop the msrv. But maybe I'm missing something. The point is tools should help us to write good and maintainable code, we shouldn't introduce complicated structure of code just because some tools fail to do its job. [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html Regards, Boqun
On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > > @Miguel? > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > > > I don't think we can wait for that to be fixed, though. Usually clippy > > is distributed with rustc via rustup, so even if this is eventually > > fixed, all versions between 1.84.0 and the fix will need this > > workaround until MSRV is >= 1.84.0. > > We need to take one step back to evalute this "workaround". > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are > clearly defined as equavilent to `as` operation [1]. Therefore, the > changes in this patch doing the conversion with expose_provenance() and > with_exposed_provenance{,_mut}() don't change anything related to > provenance in practice. > > I do agree we want to use the explicit provenance API, but I don't think > we want to introduce some API that we know we will change them latter > when we bump the rustc minimal version. So the question is: are these > stubs what we want even though in the future our minimal rustc version > stablizes provenance API? If not, then the cost of this patch cannot > justify its benefits IMO. > > Now let's also look into why we choose a msrv for clippy, I would guess > it's because we need to support all the versions of rustc starting at > 1.78 and we want clippy to report a problem based on 1.78 even though > we're using a higher version of rustc. But for this particular case, we > use a feature that has already been stablized in a higher version of > rustc, which means the problem reported by clippy doesn't help us, nor > does it provide better code. Frankly speaking, I think we have other > ways to ensure the support of all rustc versions without a msrv for > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm > missing something. > > The point is tools should help us to write good and maintainable code, > we shouldn't introduce complicated structure of code just because some > tools fail to do its job. > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html Even if we globally disable this clippy lint, we still need stubs because exposed_provenance was added in 1.79.0. Did your suggestion address this? Perhaps I missed it.
On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote: > On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > > > @Miguel? > > > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > > > > > I don't think we can wait for that to be fixed, though. Usually clippy > > > is distributed with rustc via rustup, so even if this is eventually > > > fixed, all versions between 1.84.0 and the fix will need this > > > workaround until MSRV is >= 1.84.0. > > > > We need to take one step back to evalute this "workaround". > > > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are > > clearly defined as equavilent to `as` operation [1]. Therefore, the > > changes in this patch doing the conversion with expose_provenance() and > > with_exposed_provenance{,_mut}() don't change anything related to > > provenance in practice. > > > > I do agree we want to use the explicit provenance API, but I don't think > > we want to introduce some API that we know we will change them latter > > when we bump the rustc minimal version. So the question is: are these > > stubs what we want even though in the future our minimal rustc version > > stablizes provenance API? If not, then the cost of this patch cannot > > justify its benefits IMO. > > > > Now let's also look into why we choose a msrv for clippy, I would guess > > it's because we need to support all the versions of rustc starting at > > 1.78 and we want clippy to report a problem based on 1.78 even though > > we're using a higher version of rustc. But for this particular case, we > > use a feature that has already been stablized in a higher version of > > rustc, which means the problem reported by clippy doesn't help us, nor > > does it provide better code. Frankly speaking, I think we have other > > ways to ensure the support of all rustc versions without a msrv for > > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm > > missing something. > > > > The point is tools should help us to write good and maintainable code, > > we shouldn't introduce complicated structure of code just because some > > tools fail to do its job. > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html > > Even if we globally disable this clippy lint, we still need stubs > because exposed_provenance was added in 1.79.0. Did your suggestion > address this? Perhaps I missed it. No, I didn't. That's a separate topic though, because I can see the argument that: because with_exposed_provenance() is a function rather than a method, it won't be very benefical to use ptr::with_exposed_provenance() instead of kernel::with_exposed_provenance(), therefor these stubs of exposed_provenance make sense to exist. But I don't think the same argument works for ptr::{with_,map_,}addr(). Regards, Boqun
On Mon, Mar 17, 2025 at 4:46 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote: > > On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > > > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > > > > @Miguel? > > > > > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > > > > > > > I don't think we can wait for that to be fixed, though. Usually clippy > > > > is distributed with rustc via rustup, so even if this is eventually > > > > fixed, all versions between 1.84.0 and the fix will need this > > > > workaround until MSRV is >= 1.84.0. > > > > > > We need to take one step back to evalute this "workaround". > > > > > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are > > > clearly defined as equavilent to `as` operation [1]. Therefore, the > > > changes in this patch doing the conversion with expose_provenance() and > > > with_exposed_provenance{,_mut}() don't change anything related to > > > provenance in practice. > > > > > > I do agree we want to use the explicit provenance API, but I don't think > > > we want to introduce some API that we know we will change them latter > > > when we bump the rustc minimal version. So the question is: are these > > > stubs what we want even though in the future our minimal rustc version > > > stablizes provenance API? If not, then the cost of this patch cannot > > > justify its benefits IMO. > > > > > > Now let's also look into why we choose a msrv for clippy, I would guess > > > it's because we need to support all the versions of rustc starting at > > > 1.78 and we want clippy to report a problem based on 1.78 even though > > > we're using a higher version of rustc. But for this particular case, we > > > use a feature that has already been stablized in a higher version of > > > rustc, which means the problem reported by clippy doesn't help us, nor > > > does it provide better code. Frankly speaking, I think we have other > > > ways to ensure the support of all rustc versions without a msrv for > > > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm > > > missing something. > > > > > > The point is tools should help us to write good and maintainable code, > > > we shouldn't introduce complicated structure of code just because some > > > tools fail to do its job. > > > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html > > > > Even if we globally disable this clippy lint, we still need stubs > > because exposed_provenance was added in 1.79.0. Did your suggestion > > address this? Perhaps I missed it. > > No, I didn't. > > That's a separate topic though, because I can see the argument that: > because with_exposed_provenance() is a function rather than a method, it > won't be very benefical to use ptr::with_exposed_provenance() instead of > kernel::with_exposed_provenance(), therefor these stubs of > exposed_provenance make sense to exist. But I don't think the same > argument works for ptr::{with_,map_,}addr(). What about `pointer::expose_provenance`? It's a method that was added in 1.79.0. We can certainly disable the clippy lint rather than add stubs for `pointer::{with_,map_,}addr`, but it doesn't bring us to a solution where only free functions require stubs.
On Mon, Mar 17, 2025 at 04:53:18PM -0400, Tamir Duberstein wrote: > On Mon, Mar 17, 2025 at 4:46 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote: > > > On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > > > > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > > > > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > > > > > @Miguel? > > > > > > > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > > > > > > > > > I don't think we can wait for that to be fixed, though. Usually clippy > > > > > is distributed with rustc via rustup, so even if this is eventually > > > > > fixed, all versions between 1.84.0 and the fix will need this > > > > > workaround until MSRV is >= 1.84.0. > > > > > > > > We need to take one step back to evalute this "workaround". > > > > > > > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are > > > > clearly defined as equavilent to `as` operation [1]. Therefore, the > > > > changes in this patch doing the conversion with expose_provenance() and > > > > with_exposed_provenance{,_mut}() don't change anything related to > > > > provenance in practice. > > > > > > > > I do agree we want to use the explicit provenance API, but I don't think > > > > we want to introduce some API that we know we will change them latter > > > > when we bump the rustc minimal version. So the question is: are these > > > > stubs what we want even though in the future our minimal rustc version > > > > stablizes provenance API? If not, then the cost of this patch cannot > > > > justify its benefits IMO. > > > > > > > > Now let's also look into why we choose a msrv for clippy, I would guess > > > > it's because we need to support all the versions of rustc starting at > > > > 1.78 and we want clippy to report a problem based on 1.78 even though > > > > we're using a higher version of rustc. But for this particular case, we > > > > use a feature that has already been stablized in a higher version of > > > > rustc, which means the problem reported by clippy doesn't help us, nor > > > > does it provide better code. Frankly speaking, I think we have other > > > > ways to ensure the support of all rustc versions without a msrv for > > > > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm > > > > missing something. > > > > > > > > The point is tools should help us to write good and maintainable code, > > > > we shouldn't introduce complicated structure of code just because some > > > > tools fail to do its job. > > > > > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html > > > > > > Even if we globally disable this clippy lint, we still need stubs > > > because exposed_provenance was added in 1.79.0. Did your suggestion > > > address this? Perhaps I missed it. > > > > No, I didn't. > > > > That's a separate topic though, because I can see the argument that: > > because with_exposed_provenance() is a function rather than a method, it > > won't be very benefical to use ptr::with_exposed_provenance() instead of > > kernel::with_exposed_provenance(), therefor these stubs of > > exposed_provenance make sense to exist. But I don't think the same > > argument works for ptr::{with_,map_,}addr(). > > 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. Regards, Boqun > We can certainly disable the clippy lint rather than add stubs for > `pointer::{with_,map_,}addr`, but it doesn't bring us to a solution > where only free functions require stubs.
On Mon, Mar 17, 2025 at 5:36 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > On Mon, Mar 17, 2025 at 04:53:18PM -0400, Tamir Duberstein wrote: > > On Mon, Mar 17, 2025 at 4:46 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > On Mon, Mar 17, 2025 at 04:35:42PM -0400, Tamir Duberstein wrote: > > > > On Mon, Mar 17, 2025 at 4:28 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > On Mon, Mar 17, 2025 at 03:05:45PM -0400, Tamir Duberstein wrote: > > > > > > On Mon, Mar 17, 2025 at 2:50 PM Tamir Duberstein <tamird@gmail.com> wrote: > > > > > > > > > > > > > > On Mon, Mar 17, 2025 at 2:17 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > > > > > > > > > > > > > > > Then we should fix clippy or how we set msrv rather adding the stub. > > > > > > > > @Miguel? > > > > > > > > > > > > > > I filed https://github.com/rust-lang/rust-clippy/issues/14425. > > > > > > > > > > > > I don't think we can wait for that to be fixed, though. Usually clippy > > > > > > is distributed with rustc via rustup, so even if this is eventually > > > > > > fixed, all versions between 1.84.0 and the fix will need this > > > > > > workaround until MSRV is >= 1.84.0. > > > > > > > > > > We need to take one step back to evalute this "workaround". > > > > > > > > > > First, expose_provenance() and with_exposed_provenance{,_mut}() API are > > > > > clearly defined as equavilent to `as` operation [1]. Therefore, the > > > > > changes in this patch doing the conversion with expose_provenance() and > > > > > with_exposed_provenance{,_mut}() don't change anything related to > > > > > provenance in practice. > > > > > > > > > > I do agree we want to use the explicit provenance API, but I don't think > > > > > we want to introduce some API that we know we will change them latter > > > > > when we bump the rustc minimal version. So the question is: are these > > > > > stubs what we want even though in the future our minimal rustc version > > > > > stablizes provenance API? If not, then the cost of this patch cannot > > > > > justify its benefits IMO. > > > > > > > > > > Now let's also look into why we choose a msrv for clippy, I would guess > > > > > it's because we need to support all the versions of rustc starting at > > > > > 1.78 and we want clippy to report a problem based on 1.78 even though > > > > > we're using a higher version of rustc. But for this particular case, we > > > > > use a feature that has already been stablized in a higher version of > > > > > rustc, which means the problem reported by clippy doesn't help us, nor > > > > > does it provide better code. Frankly speaking, I think we have other > > > > > ways to ensure the support of all rustc versions without a msrv for > > > > > clippy. If I was to choose, I would simply drop the msrv. But maybe I'm > > > > > missing something. > > > > > > > > > > The point is tools should help us to write good and maintainable code, > > > > > we shouldn't introduce complicated structure of code just because some > > > > > tools fail to do its job. > > > > > > > > > > [1]: https://doc.rust-lang.org/std/ptr/fn.with_exposed_provenance_mut.html > > > > > > > > Even if we globally disable this clippy lint, we still need stubs > > > > because exposed_provenance was added in 1.79.0. Did your suggestion > > > > address this? Perhaps I missed it. > > > > > > No, I didn't. > > > > > > That's a separate topic though, because I can see the argument that: > > > because with_exposed_provenance() is a function rather than a method, it > > > won't be very benefical to use ptr::with_exposed_provenance() instead of > > > kernel::with_exposed_provenance(), therefor these stubs of > > > exposed_provenance make sense to exist. But I don't think the same > > > argument works for ptr::{with_,map_,}addr(). > > > > 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(). I don't think we want these stubs forever. > 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) I like this idea, but I can't get it to work. When both inherent and trait methods are available, the compiler seems to prefer the inherent method. > We can also make with_exposed_provenance() use the same *Ext trick, > and remove it when we bump the minimal rustc version. This part I don't understand. What would we impl the Ext on, given that `with_exposed_provenance` is a free function? Option 3) take this series without the last commit, and revisit when MSRV >= 1.79.0 or >= 1.84.0?
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 ;-) 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 { - /// 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> { + /// 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}; + // 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}
On Mon, Mar 17, 2025 at 07:56:09PM -0400, Tamir Duberstein wrote: [..] > > Option 3) take this series without the last commit, and revisit when > MSRV >= 1.79.0 or >= 1.84.0? Works for me as well. Regards, Boqun
On Mon, Mar 17, 2025 at 8:11 PM Boqun Feng <boqun.feng@gmail.com> wrote: > > 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)] Ah, this is the reason this works for you (and the one in the kernel root). When I said it didn't work, I was referring to not being able to convincingly avoid these lints without disabling the check altogether. Let's see what Miguel thinks. I agree that the options are: extension trait + stubs/reexports + suppressing `incompatible_msrv` or just dropping the last patch until MSRV bump.
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}
On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > Throughout the tree, use the strict provenance APIs stabilized in Rust > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > functions at the `kernel` crate root along with polyfills for rustc < > 1.84.0. > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > 1.84.0 as our MSRV is 1.78.0. > > In the `kernel` crate, enable the strict provenance lints on rustc >= > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > compiler flags that are dependent on the rustc version in use. > > Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > Suggested-by: Benno Lossin <benno.lossin@proton.me> > Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > Signed-off-by: Tamir Duberstein <tamird@gmail.com> I'm not convinced that the pros of this change outweigh the cons. I think this is going to be too confusing for the C developers who look at this code. > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > index 719b0a48ff55..96393bcf6bd7 100644 > --- a/rust/kernel/uaccess.rs > +++ b/rust/kernel/uaccess.rs > @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > } > // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write > // that many bytes to it. > - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; > + let res = unsafe { > + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) > + }; > if res != 0 { > return Err(EFAULT); > } > @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { > let res = unsafe { > bindings::_copy_from_user( > out.as_mut_ptr().cast::<c_void>(), > - self.ptr as *const c_void, > + crate::with_exposed_provenance(self.ptr), > len, > ) > }; That's especially true for cases like this. These are userspace pointers that are never dereferenced. It's not useful to care about provenance here. Alice
On Tue, Mar 18, 2025 at 8:29 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > Throughout the tree, use the strict provenance APIs stabilized in Rust > > 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > > functions at the `kernel` crate root along with polyfills for rustc < > > 1.84.0. > > > > Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > > 1.84.0 as our MSRV is 1.78.0. > > > > In the `kernel` crate, enable the strict provenance lints on rustc >= > > 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > > compiler flags that are dependent on the rustc version in use. > > > > Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > > Suggested-by: Benno Lossin <benno.lossin@proton.me> > > Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > > Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > I'm not convinced that the pros of this change outweigh the cons. I > think this is going to be too confusing for the C developers who look at > this code. > > > diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > > index 719b0a48ff55..96393bcf6bd7 100644 > > --- a/rust/kernel/uaccess.rs > > +++ b/rust/kernel/uaccess.rs > > @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > > } > > // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write > > // that many bytes to it. > > - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; > > + let res = unsafe { > > + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) > > + }; > > if res != 0 { > > return Err(EFAULT); > > } > > @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { > > let res = unsafe { > > bindings::_copy_from_user( > > out.as_mut_ptr().cast::<c_void>(), > > - self.ptr as *const c_void, > > + crate::with_exposed_provenance(self.ptr), > > len, > > ) > > }; > > That's especially true for cases like this. These are userspace pointers > that are never dereferenced. It's not useful to care about provenance > here. > > Alice Let's just drop this last patch. It can be revisited later or not at all. Perhaps in the future I need to be more willing to say no to scope creep.
On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: >> Throughout the tree, use the strict provenance APIs stabilized in Rust >> 1.84.0[1]. Retain backwards-compatibility by introducing forwarding >> functions at the `kernel` crate root along with polyfills for rustc < >> 1.84.0. >> >> Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < >> 1.84.0 as our MSRV is 1.78.0. >> >> In the `kernel` crate, enable the strict provenance lints on rustc >= >> 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing >> compiler flags that are dependent on the rustc version in use. >> >> Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] >> Suggested-by: Benno Lossin <benno.lossin@proton.me> >> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ >> Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > I'm not convinced that the pros of this change outweigh the cons. I > think this is going to be too confusing for the C developers who look at > this code. 1) I think we should eliminate all possible `as` conversions. They are non-descriptive (since they can do may *very* different things) and ptr2int conversions are part of that. 2) At some point we will have to move to the provenance API, since that's what Rust chose to do. I don't think that doing it at a later point is doing anyone a favor. 3) I don't understand the argument that this is confusing to C devs. They are just normal functions that are well-documented (and if that's not the case, we can just improve them upstream). And functions are much easier to learn about than `as` casts (those are IMO much more difficult to figure out than then strict provenance functions). Thus I think we should keep this patch (with Boqun's improvement). >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs >> index 719b0a48ff55..96393bcf6bd7 100644 >> --- a/rust/kernel/uaccess.rs >> +++ b/rust/kernel/uaccess.rs >> @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { >> } >> // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write >> // that many bytes to it. >> - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; >> + let res = unsafe { >> + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) >> + }; >> if res != 0 { >> return Err(EFAULT); >> } >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { >> let res = unsafe { >> bindings::_copy_from_user( >> out.as_mut_ptr().cast::<c_void>(), >> - self.ptr as *const c_void, >> + crate::with_exposed_provenance(self.ptr), >> len, >> ) >> }; > > That's especially true for cases like this. These are userspace pointers > that are never dereferenced. It's not useful to care about provenance > here. I agree for this case, but I think we shouldn't be using raw pointers for this to begin with. I'd think that a newtype wrapping `usize` is a much better fit. It can then also back the `IoRaw` type. AFAIU user space pointers don't have provenance, right? (if they do, then we should use this API :) --- Cheers, Benno
On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: > On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > >> Throughout the tree, use the strict provenance APIs stabilized in Rust > >> 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > >> functions at the `kernel` crate root along with polyfills for rustc < > >> 1.84.0. > >> > >> Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > >> 1.84.0 as our MSRV is 1.78.0. > >> > >> In the `kernel` crate, enable the strict provenance lints on rustc >= > >> 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > >> compiler flags that are dependent on the rustc version in use. > >> > >> Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > >> Suggested-by: Benno Lossin <benno.lossin@proton.me> > >> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > >> Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > I'm not convinced that the pros of this change outweigh the cons. I > > think this is going to be too confusing for the C developers who look at > > this code. > > 1) I think we should eliminate all possible `as` conversions. They are > non-descriptive (since they can do may *very* different things) and > ptr2int conversions are part of that. > 2) At some point we will have to move to the provenance API, since > that's what Rust chose to do. I don't think that doing it at a later > point is doing anyone a favor. We don't *have* to do anything. Sure, most `as` conversions can be removed now that we have fixed the integer type mappings, but I'm still not convinced by this case. Like, sure, use it for that one case in `kernel::str` where it uses integers for pointers for some reason. But most other cases, provenance isn't useful. > 3) I don't understand the argument that this is confusing to C devs. > They are just normal functions that are well-documented (and if > that's not the case, we can just improve them upstream). And > functions are much easier to learn about than `as` casts (those are > IMO much more difficult to figure out than then strict provenance > functions). I really don't think that's true, no matter how good the docs are. If you see `addr as *mut c_void` as a C dev, you are going to immediately understand what that means. If you see with_exposed_provenance(addr), you're not going to understand what that means from the name - you have to interrupt your reading and look up the function with the weird name. And those docs probably spend a long time talking about stuff that doesn't matter for your pointer, since it's probably a userspace pointer or similar. > Thus I think we should keep this patch (with Boqun's improvement). > > >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > >> index 719b0a48ff55..96393bcf6bd7 100644 > >> --- a/rust/kernel/uaccess.rs > >> +++ b/rust/kernel/uaccess.rs > >> @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > >> } > >> // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write > >> // that many bytes to it. > >> - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; > >> + let res = unsafe { > >> + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) > >> + }; > >> if res != 0 { > >> return Err(EFAULT); > >> } > >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { > >> let res = unsafe { > >> bindings::_copy_from_user( > >> out.as_mut_ptr().cast::<c_void>(), > >> - self.ptr as *const c_void, > >> + crate::with_exposed_provenance(self.ptr), > >> len, > >> ) > >> }; > > > > That's especially true for cases like this. These are userspace pointers > > that are never dereferenced. It's not useful to care about provenance > > here. > > I agree for this case, but I think we shouldn't be using raw pointers > for this to begin with. I'd think that a newtype wrapping `usize` is a > much better fit. It can then also back the `IoRaw` type. AFAIU user > space pointers don't have provenance, right? (if they do, then we should > use this API :) We're doing that to the fullest extent possible already. We only convert them to pointers when calling C FFI functions that take user pointers as a raw pointer. Alice
On Wed, Mar 19, 2025 at 8:21 AM Alice Ryhl <aliceryhl@google.com> wrote: > > On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: > > On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: > > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > > >> Throughout the tree, use the strict provenance APIs stabilized in Rust > > >> 1.84.0[1]. Retain backwards-compatibility by introducing forwarding > > >> functions at the `kernel` crate root along with polyfills for rustc < > > >> 1.84.0. > > >> > > >> Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < > > >> 1.84.0 as our MSRV is 1.78.0. > > >> > > >> In the `kernel` crate, enable the strict provenance lints on rustc >= > > >> 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing > > >> compiler flags that are dependent on the rustc version in use. > > >> > > >> Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] > > >> Suggested-by: Benno Lossin <benno.lossin@proton.me> > > >> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ > > >> Signed-off-by: Tamir Duberstein <tamird@gmail.com> > > > > > > I'm not convinced that the pros of this change outweigh the cons. I > > > think this is going to be too confusing for the C developers who look at > > > this code. > > > > 1) I think we should eliminate all possible `as` conversions. They are > > non-descriptive (since they can do may *very* different things) and > > ptr2int conversions are part of that. > > 2) At some point we will have to move to the provenance API, since > > that's what Rust chose to do. I don't think that doing it at a later > > point is doing anyone a favor. > > We don't *have* to do anything. Sure, most `as` conversions can be > removed now that we have fixed the integer type mappings, but I'm still > not convinced by this case. > > Like, sure, use it for that one case in `kernel::str` where it uses > integers for pointers for some reason. But most other cases, provenance > isn't useful. > > > 3) I don't understand the argument that this is confusing to C devs. > > They are just normal functions that are well-documented (and if > > that's not the case, we can just improve them upstream). And > > functions are much easier to learn about than `as` casts (those are > > IMO much more difficult to figure out than then strict provenance > > functions). > > I really don't think that's true, no matter how good the docs are. If > you see `addr as *mut c_void` as a C dev, you are going to immediately > understand what that means. If you see with_exposed_provenance(addr), > you're not going to understand what that means from the name - you have > to interrupt your reading and look up the function with the weird name. > > And those docs probably spend a long time talking about stuff that > doesn't matter for your pointer, since it's probably a userspace pointer > or similar. > > > Thus I think we should keep this patch (with Boqun's improvement). > > > > >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs > > >> index 719b0a48ff55..96393bcf6bd7 100644 > > >> --- a/rust/kernel/uaccess.rs > > >> +++ b/rust/kernel/uaccess.rs > > >> @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { > > >> } > > >> // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write > > >> // that many bytes to it. > > >> - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; > > >> + let res = unsafe { > > >> + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) > > >> + }; > > >> if res != 0 { > > >> return Err(EFAULT); > > >> } > > >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { > > >> let res = unsafe { > > >> bindings::_copy_from_user( > > >> out.as_mut_ptr().cast::<c_void>(), > > >> - self.ptr as *const c_void, > > >> + crate::with_exposed_provenance(self.ptr), > > >> len, > > >> ) > > >> }; > > > > > > That's especially true for cases like this. These are userspace pointers > > > that are never dereferenced. It's not useful to care about provenance > > > here. > > > > I agree for this case, but I think we shouldn't be using raw pointers > > for this to begin with. I'd think that a newtype wrapping `usize` is a > > much better fit. It can then also back the `IoRaw` type. AFAIU user > > space pointers don't have provenance, right? (if they do, then we should > > use this API :) > > We're doing that to the fullest extent possible already. We only convert > them to pointers when calling C FFI functions that take user pointers as > a raw pointer. > > Alice Personally, I agree with Benno that `as` conversions are a misfeature in the language. I think this patch and the ensuing discussion is making perfect the enemy of good, so I'd prefer to drop it and revisit when the ergonomics have improved.
On Wed Mar 19, 2025 at 1:21 PM CET, Alice Ryhl wrote: > On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: >> On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: >> > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: >> >> Throughout the tree, use the strict provenance APIs stabilized in Rust >> >> 1.84.0[1]. Retain backwards-compatibility by introducing forwarding >> >> functions at the `kernel` crate root along with polyfills for rustc < >> >> 1.84.0. >> >> >> >> Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < >> >> 1.84.0 as our MSRV is 1.78.0. >> >> >> >> In the `kernel` crate, enable the strict provenance lints on rustc >= >> >> 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing >> >> compiler flags that are dependent on the rustc version in use. >> >> >> >> Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] >> >> Suggested-by: Benno Lossin <benno.lossin@proton.me> >> >> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ >> >> Signed-off-by: Tamir Duberstein <tamird@gmail.com> >> > >> > I'm not convinced that the pros of this change outweigh the cons. I >> > think this is going to be too confusing for the C developers who look at >> > this code. >> >> 1) I think we should eliminate all possible `as` conversions. They are >> non-descriptive (since they can do may *very* different things) and >> ptr2int conversions are part of that. >> 2) At some point we will have to move to the provenance API, since >> that's what Rust chose to do. I don't think that doing it at a later >> point is doing anyone a favor. > > We don't *have* to do anything. Sure, most `as` conversions can be > removed now that we have fixed the integer type mappings, but I'm still > not convinced by this case. > > Like, sure, use it for that one case in `kernel::str` where it uses > integers for pointers for some reason. But most other cases, provenance > isn't useful. I disagree, it's only going to get more painful in the long run to change this. >> 3) I don't understand the argument that this is confusing to C devs. >> They are just normal functions that are well-documented (and if >> that's not the case, we can just improve them upstream). And >> functions are much easier to learn about than `as` casts (those are >> IMO much more difficult to figure out than then strict provenance >> functions). > > I really don't think that's true, no matter how good the docs are. If > you see `addr as *mut c_void` as a C dev, you are going to immediately > understand what that means. If you see with_exposed_provenance(addr), > you're not going to understand what that means from the name - you have > to interrupt your reading and look up the function with the weird name. I see this as a double edged sword, yes `addr as *mut c_void` might seem more easily digestible on the first encounter, but that might also lead them to never look up what it exactly does. And I don't think that we should optimize these functions for C readers. They aren't used commonly (or supposed to IMO) and there are several other functions that are similarly confusing if not more already in our codebase. > And those docs probably spend a long time talking about stuff that > doesn't matter for your pointer, since it's probably a userspace pointer > or similar. For userspace pointers, see below. >> Thus I think we should keep this patch (with Boqun's improvement). >> >> >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs >> >> index 719b0a48ff55..96393bcf6bd7 100644 >> >> --- a/rust/kernel/uaccess.rs >> >> +++ b/rust/kernel/uaccess.rs >> >> @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { >> >> } >> >> // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write >> >> // that many bytes to it. >> >> - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; >> >> + let res = unsafe { >> >> + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) >> >> + }; >> >> if res != 0 { >> >> return Err(EFAULT); >> >> } >> >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { >> >> let res = unsafe { >> >> bindings::_copy_from_user( >> >> out.as_mut_ptr().cast::<c_void>(), >> >> - self.ptr as *const c_void, >> >> + crate::with_exposed_provenance(self.ptr), >> >> len, >> >> ) >> >> }; >> > >> > That's especially true for cases like this. These are userspace pointers >> > that are never dereferenced. It's not useful to care about provenance >> > here. >> >> I agree for this case, but I think we shouldn't be using raw pointers >> for this to begin with. I'd think that a newtype wrapping `usize` is a >> much better fit. It can then also back the `IoRaw` type. AFAIU user >> space pointers don't have provenance, right? (if they do, then we should >> use this API :) > > We're doing that to the fullest extent possible already. We only convert > them to pointers when calling C FFI functions that take user pointers as > a raw pointer. We should make bindgen use that type in those interfaces already. --- Cheers, Benno
On Wed Mar 19, 2025 at 3:14 PM CET, Tamir Duberstein wrote: > On Wed, Mar 19, 2025 at 8:21 AM Alice Ryhl <aliceryhl@google.com> wrote: >> On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: >> > On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: >> > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: >> > >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { >> > >> let res = unsafe { >> > >> bindings::_copy_from_user( >> > >> out.as_mut_ptr().cast::<c_void>(), >> > >> - self.ptr as *const c_void, >> > >> + crate::with_exposed_provenance(self.ptr), >> > >> len, >> > >> ) >> > >> }; >> > > >> > > That's especially true for cases like this. These are userspace pointers >> > > that are never dereferenced. It's not useful to care about provenance >> > > here. >> > >> > I agree for this case, but I think we shouldn't be using raw pointers >> > for this to begin with. I'd think that a newtype wrapping `usize` is a >> > much better fit. It can then also back the `IoRaw` type. AFAIU user >> > space pointers don't have provenance, right? (if they do, then we should >> > use this API :) >> >> We're doing that to the fullest extent possible already. We only convert >> them to pointers when calling C FFI functions that take user pointers as >> a raw pointer. >> >> Alice > > Personally, I agree with Benno that `as` conversions are a misfeature > in the language. > > I think this patch and the ensuing discussion is making perfect the > enemy of good, so I'd prefer to drop it and revisit when the > ergonomics have improved. I don't think that we need to rush on the rest of the patch series. Boqun's suggestion is very good and I'm not sure which ergonomics need to be improved here. --- Cheers, Benno
On Tue, Mar 18, 2025 at 09:23:42AM +0000, Benno Lossin wrote: [..] > > +#![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. > We still need to provide stubs for with_exposed_provenance() and its friends for rustc == 1.78, so there are a few more functions in this 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. > Right, I was missing this. > > + /// 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? > We need re-export these for ructc >=1.79, because for rustc == 1.78 we only have kernel::expose_provenance() and its friends, therefore user-side can only use them. Regards, Boqun > --- > 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} > >
On Wed, Mar 19, 2025 at 10:42 AM Benno Lossin <benno.lossin@proton.me> wrote: > > On Wed Mar 19, 2025 at 3:14 PM CET, Tamir Duberstein wrote: > > On Wed, Mar 19, 2025 at 8:21 AM Alice Ryhl <aliceryhl@google.com> wrote: > >> On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: > >> > On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: > >> > > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: > >> > >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { > >> > >> let res = unsafe { > >> > >> bindings::_copy_from_user( > >> > >> out.as_mut_ptr().cast::<c_void>(), > >> > >> - self.ptr as *const c_void, > >> > >> + crate::with_exposed_provenance(self.ptr), > >> > >> len, > >> > >> ) > >> > >> }; > >> > > > >> > > That's especially true for cases like this. These are userspace pointers > >> > > that are never dereferenced. It's not useful to care about provenance > >> > > here. > >> > > >> > I agree for this case, but I think we shouldn't be using raw pointers > >> > for this to begin with. I'd think that a newtype wrapping `usize` is a > >> > much better fit. It can then also back the `IoRaw` type. AFAIU user > >> > space pointers don't have provenance, right? (if they do, then we should > >> > use this API :) > >> > >> We're doing that to the fullest extent possible already. We only convert > >> them to pointers when calling C FFI functions that take user pointers as > >> a raw pointer. > >> > >> Alice > > > > Personally, I agree with Benno that `as` conversions are a misfeature > > in the language. > > > > I think this patch and the ensuing discussion is making perfect the > > enemy of good, so I'd prefer to drop it and revisit when the > > ergonomics have improved. > > I don't think that we need to rush on the rest of the patch series. > Boqun's suggestion is very good and I'm not sure which ergonomics need > to be improved here. The improved ergonomics arrive in Rust 1.79. See Boqun's reply that explains we need to keep all the stubs until then. Regarding landing the rest of the series - you said it yourself: "it's only going to get more painful in the long run to change this". The nature of lints is that the longer you don't enable them, the likelier you are to have a higher hill to climb later.
On Wed Mar 19, 2025 at 1:21 PM CET, Alice Ryhl wrote: > On Wed, Mar 19, 2025 at 12:23:44AM +0000, Benno Lossin wrote: >> On Tue Mar 18, 2025 at 1:29 PM CET, Alice Ryhl wrote: >> > On Mon, Mar 17, 2025 at 10:23:56AM -0400, Tamir Duberstein wrote: >> >> Throughout the tree, use the strict provenance APIs stabilized in Rust >> >> 1.84.0[1]. Retain backwards-compatibility by introducing forwarding >> >> functions at the `kernel` crate root along with polyfills for rustc < >> >> 1.84.0. >> >> >> >> Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < >> >> 1.84.0 as our MSRV is 1.78.0. >> >> >> >> In the `kernel` crate, enable the strict provenance lints on rustc >= >> >> 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing >> >> compiler flags that are dependent on the rustc version in use. >> >> >> >> Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] >> >> Suggested-by: Benno Lossin <benno.lossin@proton.me> >> >> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ >> >> Signed-off-by: Tamir Duberstein <tamird@gmail.com> >> > >> > I'm not convinced that the pros of this change outweigh the cons. I >> > think this is going to be too confusing for the C developers who look at >> > this code. >> >> 1) I think we should eliminate all possible `as` conversions. They are >> non-descriptive (since they can do may *very* different things) and >> ptr2int conversions are part of that. >> 2) At some point we will have to move to the provenance API, since >> that's what Rust chose to do. I don't think that doing it at a later >> point is doing anyone a favor. > > We don't *have* to do anything. Sure, most `as` conversions can be > removed now that we have fixed the integer type mappings, but I'm still > not convinced by this case. > > Like, sure, use it for that one case in `kernel::str` where it uses > integers for pointers for some reason. But most other cases, provenance > isn't useful. > >> 3) I don't understand the argument that this is confusing to C devs. >> They are just normal functions that are well-documented (and if >> that's not the case, we can just improve them upstream). And >> functions are much easier to learn about than `as` casts (those are >> IMO much more difficult to figure out than then strict provenance >> functions). > > I really don't think that's true, no matter how good the docs are. If > you see `addr as *mut c_void` as a C dev, you are going to immediately > understand what that means. If you see with_exposed_provenance(addr), > you're not going to understand what that means from the name - you have > to interrupt your reading and look up the function with the weird name. > > And those docs probably spend a long time talking about stuff that > doesn't matter for your pointer, since it's probably a userspace pointer > or similar. > >> Thus I think we should keep this patch (with Boqun's improvement). >> >> >> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs >> >> index 719b0a48ff55..96393bcf6bd7 100644 >> >> --- a/rust/kernel/uaccess.rs >> >> +++ b/rust/kernel/uaccess.rs >> >> @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { >> >> } >> >> // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write >> >> // that many bytes to it. >> >> - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; >> >> + let res = unsafe { >> >> + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) >> >> + }; >> >> if res != 0 { >> >> return Err(EFAULT); >> >> } >> >> @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { >> >> let res = unsafe { >> >> bindings::_copy_from_user( >> >> out.as_mut_ptr().cast::<c_void>(), >> >> - self.ptr as *const c_void, >> >> + crate::with_exposed_provenance(self.ptr), >> >> len, >> >> ) >> >> }; >> > >> > That's especially true for cases like this. These are userspace pointers >> > that are never dereferenced. It's not useful to care about provenance >> > here. >> >> I agree for this case, but I think we shouldn't be using raw pointers >> for this to begin with. I'd think that a newtype wrapping `usize` is a >> much better fit. It can then also back the `IoRaw` type. AFAIU user >> space pointers don't have provenance, right? (if they do, then we should >> use this API :) > > We're doing that to the fullest extent possible already. We only convert > them to pointers when calling C FFI functions that take user pointers as > a raw pointer. In our meeting, we discussed all of this in more detail. I now understand Alice's concern better: it's about userspace pointers, they don't have provenance and thus shouldn't use the strict provenance API. There are two possible solutions to this: * introduce a transparent `UserPtr` type that bindgen uses instead of a raw pointer when it encounters a userspace pointer (annotated on the C side). * use a `to_user_pointer` function instead of `without_provenance` to better convey the meaning. I prefer the first one, but it probably needs special bindgen support, so we should go with the second one until we can change it. Miguel also said that he wants to have a piece of documentation in the patch. I can write that, if he specifies a bit more what exactly it should contain :) --- Cheers, Benno
On Wed Mar 19, 2025 at 4:25 PM CET, Boqun Feng wrote: > On Tue, Mar 18, 2025 at 09:23:42AM +0000, Benno Lossin wrote: > [..] >> > +#![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. > > We still need to provide stubs for with_exposed_provenance() and its > friends for rustc == 1.78, so there are a few more functions in this > module. Ah I see, that's okay then. --- Cheers, Benno
On Wed Mar 19, 2025 at 7:23 PM CET, Tamir Duberstein wrote: > The improved ergonomics arrive in Rust 1.79. See Boqun's reply that > explains we need to keep all the stubs until then. > > Regarding landing the rest of the series - you said it yourself: "it's > only going to get more painful in the long run to change this". The > nature of lints is that the longer you don't enable them, the likelier > you are to have a higher hill to climb later. Yeah that's also true (for some reason I was understanding "dropping" the patch as "abandoning" the patch :) As discussed in the meeting, feel free to split the series. --- Cheers, Benno
diff --git a/init/Kconfig b/init/Kconfig index d0d021b3fa3b..82e28d6f7c3f 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -132,6 +132,9 @@ config CC_HAS_COUNTED_BY config RUSTC_HAS_COERCE_POINTEE def_bool RUSTC_VERSION >= 108400 +config RUSTC_HAS_STABLE_STRICT_PROVENANCE + def_bool RUSTC_VERSION >= 108400 + config PAHOLE_VERSION int default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs index fc9c9c41cd79..a1d282e48249 100644 --- a/rust/kernel/alloc.rs +++ b/rust/kernel/alloc.rs @@ -217,7 +217,7 @@ unsafe fn free(ptr: NonNull<u8>, layout: Layout) { /// Returns a properly aligned dangling pointer from the given `layout`. pub(crate) fn dangling_from_layout(layout: Layout) -> NonNull<u8> { - let ptr = layout.align() as *mut u8; + let ptr = crate::without_provenance_mut(layout.align()); // SAFETY: `layout.align()` (and hence `ptr`) is guaranteed to be non-zero. unsafe { NonNull::new_unchecked(ptr) } diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 34571f992f0d..e8232bb771b2 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -64,14 +64,14 @@ struct DevresInner<T> { /// return Err(ENOMEM); /// } /// -/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?)) +/// Ok(IoMem(IoRaw::new(kernel::expose_provenance(addr), SIZE)?)) /// } /// } /// /// impl<const SIZE: usize> Drop for IoMem<SIZE> { /// fn drop(&mut self) { /// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`. -/// unsafe { bindings::iounmap(self.0.addr() as *mut c_void); }; +/// unsafe { bindings::iounmap(kernel::with_exposed_provenance_mut(self.0.addr())); }; /// } /// } /// diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs index 9d2aadf40edf..0a018ad7478a 100644 --- a/rust/kernel/io.rs +++ b/rust/kernel/io.rs @@ -5,7 +5,7 @@ //! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h) use crate::error::{code::EINVAL, Result}; -use crate::{bindings, build_assert, ffi::c_void}; +use crate::{bindings, build_assert}; /// Raw representation of an MMIO region. /// @@ -75,14 +75,14 @@ pub fn maxsize(&self) -> usize { /// return Err(ENOMEM); /// } /// -/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?)) +/// Ok(IoMem(IoRaw::new(kernel::expose_provenance(addr), SIZE)?)) /// } /// } /// /// impl<const SIZE: usize> Drop for IoMem<SIZE> { /// fn drop(&mut self) { /// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`. -/// unsafe { bindings::iounmap(self.0.addr() as *mut c_void); }; +/// unsafe { bindings::iounmap(kernel::with_exposed_provenance_mut(self.0.addr())); }; /// } /// } /// @@ -119,7 +119,7 @@ pub fn $name(&self, offset: usize) -> $type_name { let addr = self.io_addr_assert::<$type_name>(offset); // SAFETY: By the type invariant `addr` is a valid address for MMIO operations. - unsafe { bindings::$name(addr as *const c_void) } + unsafe { bindings::$name(crate::with_exposed_provenance(addr)) } } /// Read IO data from a given offset. @@ -131,7 +131,7 @@ pub fn $try_name(&self, offset: usize) -> Result<$type_name> { let addr = self.io_addr::<$type_name>(offset)?; // SAFETY: By the type invariant `addr` is a valid address for MMIO operations. - Ok(unsafe { bindings::$name(addr as *const c_void) }) + Ok(unsafe { bindings::$name(crate::with_exposed_provenance(addr)) }) } }; } @@ -148,7 +148,7 @@ pub fn $name(&self, value: $type_name, offset: usize) { let addr = self.io_addr_assert::<$type_name>(offset); // SAFETY: By the type invariant `addr` is a valid address for MMIO operations. - unsafe { bindings::$name(value, addr as *mut c_void) } + unsafe { bindings::$name(value, crate::with_exposed_provenance_mut(addr)) } } /// Write IO data from a given offset. @@ -160,7 +160,7 @@ pub fn $try_name(&self, value: $type_name, offset: usize) -> Result { let addr = self.io_addr::<$type_name>(offset)?; // SAFETY: By the type invariant `addr` is a valid address for MMIO operations. - unsafe { bindings::$name(value, addr as *mut c_void) } + unsafe { bindings::$name(value, crate::with_exposed_provenance_mut(addr)) } Ok(()) } }; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index fc6835cc36a3..c1b274c04a0f 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -17,6 +17,11 @@ #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))] #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))] #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))] +#![cfg_attr( + CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE, + feature(strict_provenance_lints), + deny(fuzzy_provenance_casts, lossy_provenance_casts) +)] #![feature(inline_const)] #![feature(lint_reasons)] // Stable in Rust 1.83 @@ -25,6 +30,109 @@ #![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) + } +} + +#[cfg(not(CONFIG_RUSTC_HAS_STABLE_STRICT_PROVENANCE))] +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 { + // 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::<()>()) + } + } + + /// 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 + } + + /// 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 { + addr as *const T + } + + /// 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 { + addr as *mut T + } + + /// 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 { + addr as *mut T + } +} + +pub use strict_provenance::*; + // 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 40d1bd13682c..b70076d16008 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 { - self.0.data as usize + crate::addr(self.0.data) } } diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index a26f154ae1b9..87c9f67b3f0f 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: usize = unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) } as usize; + let ioptr = crate::expose_provenance(unsafe { bindings::pci_iomap(pdev.as_raw(), num, 0) }); if ioptr == 0 { // SAFETY: // `pdev` valid by the invariants of `Device`. @@ -320,7 +320,7 @@ unsafe fn do_release(pdev: &Device, ioptr: usize, num: i32) { // `ioptr` is valid by the safety requirements. // `num` is valid by the safety requirements. unsafe { - bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut kernel::ffi::c_void); + bindings::pci_iounmap(pdev.as_raw(), crate::with_exposed_provenance_mut(ioptr)); bindings::pci_release_region(pdev.as_raw(), num); } } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 0b80a119d5f0..6bc6357293e4 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -692,9 +692,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: pos as usize, - pos: pos as usize, - end: end as usize, + beg: crate::expose_provenance(pos), + pos: crate::expose_provenance(pos), + end: crate::expose_provenance(end), } } @@ -705,7 +705,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 = buf as usize; + let pos = crate::expose_provenance(buf); // INVARIANT: We ensure that `end` is never less then `buf`, and the safety requirements // guarantees that the memory region is valid for writes. Self { @@ -719,7 +719,7 @@ pub(crate) unsafe fn from_buffer(buf: *mut u8, len: usize) -> Self { /// /// N.B. It may point to invalid memory. pub(crate) fn pos(&self) -> *mut u8 { - self.pos as *mut u8 + crate::with_exposed_provenance_mut(self.pos) } /// Returns the number of bytes written to the formatter. @@ -741,11 +741,7 @@ fn write_str(&mut self, s: &str) -> fmt::Result { // SAFETY: If `len_to_copy` is non-zero, then we know `pos` has not gone past `end` // yet, so it is valid for write per the type invariants. unsafe { - core::ptr::copy_nonoverlapping( - s.as_bytes().as_ptr(), - self.pos as *mut u8, - len_to_copy, - ) + core::ptr::copy_nonoverlapping(s.as_bytes().as_ptr(), self.pos(), len_to_copy) }; } diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 719b0a48ff55..96393bcf6bd7 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -226,7 +226,9 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { } // SAFETY: `out_ptr` points into a mutable slice of length `len`, so we may write // that many bytes to it. - let res = unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len) }; + let res = unsafe { + bindings::copy_from_user(out_ptr, crate::with_exposed_provenance(self.ptr), len) + }; if res != 0 { return Err(EFAULT); } @@ -264,7 +266,7 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> { let res = unsafe { bindings::_copy_from_user( out.as_mut_ptr().cast::<c_void>(), - self.ptr as *const c_void, + crate::with_exposed_provenance(self.ptr), len, ) }; @@ -330,7 +332,9 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result { } // SAFETY: `data_ptr` points into an immutable slice of length `len`, so we may read // that many bytes from it. - let res = unsafe { bindings::copy_to_user(self.ptr as *mut c_void, data_ptr, len) }; + let res = unsafe { + bindings::copy_to_user(crate::with_exposed_provenance_mut(self.ptr), data_ptr, len) + }; if res != 0 { return Err(EFAULT); } @@ -357,7 +361,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { // is a compile-time constant. let res = unsafe { bindings::_copy_to_user( - self.ptr as *mut c_void, + crate::with_exposed_provenance_mut(self.ptr), (value as *const T).cast::<c_void>(), len, )
Throughout the tree, use the strict provenance APIs stabilized in Rust 1.84.0[1]. Retain backwards-compatibility by introducing forwarding functions at the `kernel` crate root along with polyfills for rustc < 1.84.0. Use `#[allow(clippy::incompatible_msrv)]` to avoid warnings on rustc < 1.84.0 as our MSRV is 1.78.0. In the `kernel` crate, enable the strict provenance lints on rustc >= 1.84.0; do this in `lib.rs` rather than `Makefile` to avoid introducing compiler flags that are dependent on the rustc version in use. Link: https://blog.rust-lang.org/2025/01/09/Rust-1.84.0.html#strict-provenance-apis [1] Suggested-by: Benno Lossin <benno.lossin@proton.me> Link: https://lore.kernel.org/all/D8EIXDMRXMJP.36TFCGWZBRS3Y@proton.me/ Signed-off-by: Tamir Duberstein <tamird@gmail.com> --- init/Kconfig | 3 ++ rust/kernel/alloc.rs | 2 +- rust/kernel/devres.rs | 4 +- rust/kernel/io.rs | 14 +++---- rust/kernel/lib.rs | 108 +++++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/of.rs | 2 +- rust/kernel/pci.rs | 4 +- rust/kernel/str.rs | 16 +++----- rust/kernel/uaccess.rs | 12 ++++-- 9 files changed, 138 insertions(+), 27 deletions(-)