This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
Link: https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/ [1]
Signed-off-by: Tamir Duberstein tamird@gmail.com --- Changes in v5: - Use `pointer::addr` in OF. (Boqun Feng) - Add documentation on stubs. (Benno Lossin) - Mark stubs `#[inline]`. - Pick up Alice's RB on a shared commit from https://lore.kernel.org/all/Z9f-3Aj3_FWBZRrm@google.com/. - Link to v4: https://lore.kernel.org/r/20250315-ptr-as-ptr-v4-0-b2d72c14dc26@gmail.com
Changes in v4: - Add missing SoB. (Benno Lossin) - Use `without_provenance_mut` in alloc. (Boqun Feng) - Limit strict provenance lints to the `kernel` crate to avoid complex logic in the build system. This can be revisited on MSRV >= 1.84.0. - Rebase on rust-next. - Link to v3: https://lore.kernel.org/r/20250314-ptr-as-ptr-v3-0-e7ba61048f4a@gmail.com
Changes in v3: - Fixed clippy warning in rust/kernel/firmware.rs. (kernel test robot) Link: https://lore.kernel.org/all/202503120332.YTCpFEvv-lkp@intel.com/ - s/as u64/as bindings::phys_addr_t/g. (Benno Lossin) - Use strict provenance APIs and enable lints. (Benno Lossin) - Link to v2: https://lore.kernel.org/r/20250309-ptr-as-ptr-v2-0-25d60ad922b7@gmail.com
Changes in v2: - Fixed typo in first commit message. - Added additional patches, converted to series. - Link to v1: https://lore.kernel.org/r/20250307-ptr-as-ptr-v1-1-582d06514c98@gmail.com
--- Tamir Duberstein (6): rust: retain pointer mut-ness in `container_of!` rust: enable `clippy::ptr_as_ptr` lint rust: enable `clippy::ptr_cast_constness` lint rust: enable `clippy::as_ptr_cast_mut` lint rust: enable `clippy::as_underscore` lint rust: use strict provenance APIs
Makefile | 4 ++ init/Kconfig | 3 + rust/bindings/lib.rs | 1 + rust/kernel/alloc.rs | 2 +- rust/kernel/alloc/allocator_test.rs | 2 +- rust/kernel/alloc/kvec.rs | 4 +- rust/kernel/block/mq/operations.rs | 2 +- rust/kernel/block/mq/request.rs | 7 +- rust/kernel/device.rs | 5 +- rust/kernel/device_id.rs | 2 +- rust/kernel/devres.rs | 19 +++--- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 +- rust/kernel/fs/file.rs | 2 +- rust/kernel/io.rs | 16 ++--- rust/kernel/kunit.rs | 15 ++--- rust/kernel/lib.rs | 113 ++++++++++++++++++++++++++++++++- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/of.rs | 6 +- rust/kernel/pci.rs | 15 +++-- rust/kernel/platform.rs | 6 +- rust/kernel/print.rs | 11 ++-- rust/kernel/rbtree.rs | 23 +++---- rust/kernel/seq_file.rs | 3 +- rust/kernel/str.rs | 18 ++---- rust/kernel/sync/poll.rs | 2 +- rust/kernel/uaccess.rs | 12 ++-- rust/kernel/workqueue.rs | 12 ++-- rust/uapi/lib.rs | 1 + 30 files changed, 218 insertions(+), 97 deletions(-) --- base-commit: 498f7ee4773f22924f00630136da8575f38954e8 change-id: 20250307-ptr-as-ptr-21b1867fc4d4
Best regards,
Avoid casting the input pointer to `*const _`, allowing the output pointer to be `*mut` if the input is `*mut`. This allows a number of `*const` to `*mut` conversions to be removed at the cost of slightly worse ergonomics when the macro is used with a reference rather than a pointer; the only example of this was in the macro's own doctest.
Reviewed-by: Benno Lossin benno.lossin@proton.me Reviewed-by: Alice Ryhl aliceryhl@google.com Signed-off-by: Tamir Duberstein tamird@gmail.com --- rust/kernel/lib.rs | 5 ++--- rust/kernel/pci.rs | 2 +- rust/kernel/platform.rs | 2 +- rust/kernel/rbtree.rs | 23 ++++++++++------------- 4 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index c92497c7c655..fc6835cc36a3 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -187,7 +187,7 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { /// } /// /// let test = Test { a: 10, b: 20 }; -/// let b_ptr = &test.b; +/// let b_ptr: *const _ = &test.b; /// // SAFETY: The pointer points at the `b` field of a `Test`, so the resulting pointer will be /// // in-bounds of the same allocation as `b_ptr`. /// let test_alias = unsafe { container_of!(b_ptr, Test, b) }; @@ -196,9 +196,8 @@ fn panic(info: &core::panic::PanicInfo<'_>) -> ! { #[macro_export] macro_rules! container_of { ($ptr:expr, $type:ty, $($f:tt)*) => {{ - let ptr = $ptr as *const _ as *const u8; let offset: usize = ::core::mem::offset_of!($type, $($f)*); - ptr.sub(offset) as *const $type + $ptr.byte_sub(offset).cast::<$type>() }} }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index f7b2743828ae..271a7690a9a0 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -364,7 +364,7 @@ pub unsafe fn from_dev(dev: ARefdevice::Device) -> Self { fn as_raw(&self) -> *mut bindings::pci_dev { // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device` // embedded in `struct pci_dev`. - unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) as _ } + unsafe { container_of!(self.0.as_raw(), bindings::pci_dev, dev) } }
/// Returns the PCI vendor ID. diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index 1297f5292ba9..84a4ecc642a1 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -189,7 +189,7 @@ unsafe fn from_dev(dev: ARefdevice::Device) -> Self { fn as_raw(&self) -> *mut bindings::platform_device { // SAFETY: By the type invariant `self.0.as_raw` is a pointer to the `struct device` // embedded in `struct platform_device`. - unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) }.cast_mut() + unsafe { container_of!(self.0.as_raw(), bindings::platform_device, dev) } } }
diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs index 1ea25c7092fb..27de954a0889 100644 --- a/rust/kernel/rbtree.rs +++ b/rust/kernel/rbtree.rs @@ -424,7 +424,7 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>> while !node.is_null() { // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self` // point to the links field of `Node<K, V>` objects. - let this = unsafe { container_of!(node, Node<K, V>, links) }.cast_mut(); + let this = unsafe { container_of!(node, Node<K, V>, links) }; // SAFETY: `this` is a non-null node so it is valid by the type invariants. let this_key = unsafe { &(*this).key }; // SAFETY: `node` is a non-null node so it is valid by the type invariants. @@ -496,7 +496,7 @@ fn drop(&mut self) { // but it is not observable. The loop invariant is still maintained.
// SAFETY: `this` is valid per the loop invariant. - unsafe { drop(KBox::from_raw(this.cast_mut())) }; + unsafe { drop(KBox::from_raw(this)) }; } } } @@ -761,7 +761,7 @@ pub fn remove_current(self) -> (Option<Self>, RBTreeNode<K, V>) { let next = self.get_neighbor_raw(Direction::Next); // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self` // point to the links field of `Node<K, V>` objects. - let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) }.cast_mut(); + let this = unsafe { container_of!(self.current.as_ptr(), Node<K, V>, links) }; // SAFETY: `this` is valid by the type invariants as described above. let node = unsafe { KBox::from_raw(this) }; let node = RBTreeNode { node }; @@ -806,7 +806,7 @@ fn remove_neighbor(&mut self, direction: Direction) -> Option<RBTreeNode<K, V>> unsafe { bindings::rb_erase(neighbor, addr_of_mut!(self.tree.root)) }; // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self` // point to the links field of `Node<K, V>` objects. - let this = unsafe { container_of!(neighbor, Node<K, V>, links) }.cast_mut(); + let this = unsafe { container_of!(neighbor, Node<K, V>, links) }; // SAFETY: `this` is valid by the type invariants as described above. let node = unsafe { KBox::from_raw(this) }; return Some(RBTreeNode { node }); @@ -912,7 +912,7 @@ unsafe fn to_key_value_mut<'b>(node: NonNullbindings::rb_node) -> (&'b K, &'b unsafe fn to_key_value_raw<'b>(node: NonNullbindings::rb_node) -> (&'b K, *mut V) { // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self` // point to the links field of `Node<K, V>` objects. - let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) }.cast_mut(); + let this = unsafe { container_of!(node.as_ptr(), Node<K, V>, links) }; // SAFETY: The passed `node` is the current node or a non-null neighbor, // thus `this` is valid by the type invariants. let k = unsafe { &(*this).key }; @@ -1021,7 +1021,7 @@ fn next(&mut self) -> OptionSelf::Item {
// SAFETY: By the type invariant of `IterRaw`, `self.next` is a valid node in an `RBTree`, // and by the type invariant of `RBTree`, all nodes point to the links field of `Node<K, V>` objects. - let cur = unsafe { container_of!(self.next, Node<K, V>, links) }.cast_mut(); + let cur = unsafe { container_of!(self.next, Node<K, V>, links) };
// SAFETY: `self.next` is a valid tree node by the type invariants. self.next = unsafe { bindings::rb_next(self.next) }; @@ -1216,7 +1216,7 @@ pub fn get_mut(&mut self) -> &mut V { // SAFETY: // - `self.node_links` is a valid pointer to a node in the tree. // - We have exclusive access to the underlying tree, and can thus give out a mutable reference. - unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value } + unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value } }
/// Converts the entry into a mutable reference to its value. @@ -1226,7 +1226,7 @@ pub fn into_mut(self) -> &'a mut V { // SAFETY: // - `self.node_links` is a valid pointer to a node in the tree. // - This consumes the `&'a mut RBTree<K, V>`, therefore it can give out a mutable reference that lives for `'a`. - unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links).cast_mut())).value } + unsafe { &mut (*(container_of!(self.node_links, Node<K, V>, links))).value } }
/// Remove this entry from the [`RBTree`]. @@ -1239,9 +1239,7 @@ pub fn remove_node(self) -> RBTreeNode<K, V> { RBTreeNode { // SAFETY: The node was a node in the tree, but we removed it, so we can convert it // back into a box. - node: unsafe { - KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut()) - }, + node: unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) }, } }
@@ -1272,8 +1270,7 @@ fn replace(self, node: RBTreeNode<K, V>) -> RBTreeNode<K, V> { // SAFETY: // - `self.node_ptr` produces a valid pointer to a node in the tree. // - Now that we removed this entry from the tree, we can convert the node to a box. - let old_node = - unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links).cast_mut()) }; + let old_node = unsafe { KBox::from_raw(container_of!(self.node_links, Node<K, V>, links)) };
RBTreeNode { node: old_node } }
In Rust 1.51.0, Clippy introduced the `ptr_as_ptr` lint [1]:
Though `as` casts between raw pointers are not terrible, `pointer::cast` is safer because it cannot accidentally change the pointer's mutability, nor cast the pointer to other types like `usize`.
There are a few classes of changes required: - Modules generated by bindgen are marked `#[allow(clippy::ptr_as_ptr)]`. - Inferred casts (` as _`) are replaced with `.cast()`. - Ascribed casts (` as *... T`) are replaced with `.cast::<T>()`. - Multistep casts from references (` as *const _ as *const T`) are replaced with `let x: *const _ = &x;` and `.cast()` or `.cast::<T>()` according to the previous rules. The intermediate `let` binding is required because `(x as *const _).cast::<T>()` results in inference failure. - Native literal C strings are replaced with `c_str!().as_char_ptr()`. - `*mut *mut T as _` is replaced with `let *mut *const T = (*mut *mut T)`.cast();` since pointer to pointer can be confusing.
Apply these changes and enable the lint -- no functional change intended.
Link: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr [1] Reviewed-by: Benno Lossin benno.lossin@proton.me Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + rust/bindings/lib.rs | 1 + rust/kernel/alloc/allocator_test.rs | 2 +- rust/kernel/alloc/kvec.rs | 4 ++-- rust/kernel/device.rs | 5 +++-- rust/kernel/devres.rs | 2 +- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 ++- rust/kernel/fs/file.rs | 2 +- rust/kernel/kunit.rs | 15 +++++++-------- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/pci.rs | 2 +- rust/kernel/platform.rs | 4 +++- rust/kernel/print.rs | 11 +++++------ rust/kernel/seq_file.rs | 3 ++- rust/kernel/str.rs | 2 +- rust/kernel/sync/poll.rs | 2 +- rust/kernel/workqueue.rs | 10 +++++----- rust/uapi/lib.rs | 1 + 19 files changed, 40 insertions(+), 34 deletions(-)
diff --git a/Makefile b/Makefile index 70bdbf2218fc..ec8efc8e23ba 100644 --- a/Makefile +++ b/Makefile @@ -483,6 +483,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::needless_continue \ -Aclippy::needless_lifetimes \ -Wclippy::no_mangle_with_rust_abi \ + -Wclippy::ptr_as_ptr \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ -Wclippy::unnecessary_safety_doc \ diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index 014af0d1fc70..0486a32ed314 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -25,6 +25,7 @@ )]
#[allow(dead_code)] +#[allow(clippy::ptr_as_ptr)] #[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { // Manual definition for blocklisted types. diff --git a/rust/kernel/alloc/allocator_test.rs b/rust/kernel/alloc/allocator_test.rs index c37d4c0c64e9..8017aa9d5213 100644 --- a/rust/kernel/alloc/allocator_test.rs +++ b/rust/kernel/alloc/allocator_test.rs @@ -82,7 +82,7 @@ unsafe fn realloc(
// SAFETY: Returns either NULL or a pointer to a memory allocation that satisfies or // exceeds the given size and alignment requirements. - let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) } as *mut u8; + let dst = unsafe { libc_aligned_alloc(layout.align(), layout.size()) }.cast::<u8>(); let dst = NonNull::new(dst).ok_or(AllocError)?;
if flags.contains(__GFP_ZERO) { diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs index ae9d072741ce..c12844764671 100644 --- a/rust/kernel/alloc/kvec.rs +++ b/rust/kernel/alloc/kvec.rs @@ -262,7 +262,7 @@ pub fn spare_capacity_mut(&mut self) -> &mut [MaybeUninit<T>] { // - `self.len` is smaller than `self.capacity` and hence, the resulting pointer is // guaranteed to be part of the same allocated object. // - `self.len` can not overflow `isize`. - let ptr = unsafe { self.as_mut_ptr().add(self.len) } as *mut MaybeUninit<T>; + let ptr = unsafe { self.as_mut_ptr().add(self.len) }.cast::<MaybeUninit<T>>();
// SAFETY: The memory between `self.len` and `self.capacity` is guaranteed to be allocated // and valid, but uninitialized. @@ -554,7 +554,7 @@ fn drop(&mut self) { // - `ptr` points to memory with at least a size of `size_of::<T>() * len`, // - all elements within `b` are initialized values of `T`, // - `len` does not exceed `isize::MAX`. - unsafe { Vec::from_raw_parts(ptr as _, len, len) } + unsafe { Vec::from_raw_parts(ptr.cast(), len, len) } } }
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index db2d9658ba47..9e500498835d 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -168,16 +168,17 @@ pub fn pr_dbg(&self, args: fmt::Arguments<'_>) { /// `KERN_*`constants, for example, `KERN_CRIT`, `KERN_ALERT`, etc. #[cfg_attr(not(CONFIG_PRINTK), allow(unused_variables))] unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) { + let msg: *const _ = &msg; // SAFETY: `klevel` is null-terminated and one of the kernel constants. `self.as_raw` // is valid because `self` is valid. The "%pA" format string expects a pointer to // `fmt::Arguments`, which is what we're passing as the last argument. #[cfg(CONFIG_PRINTK)] unsafe { bindings::_dev_printk( - klevel as *const _ as *const crate::ffi::c_char, + klevel.as_ptr().cast::crate::ffi::c_char(), self.as_raw(), c_str!("%pA").as_char_ptr(), - &msg as *const _ as *const crate::ffi::c_void, + msg.cast::crate::ffi::c_void(), ) }; } diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 942376f6f3af..3a9d998ec371 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -157,7 +157,7 @@ fn remove_action(this: &Arc<Self>) {
#[allow(clippy::missing_safety_doc)] unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) { - let ptr = ptr as *mut DevresInner<T>; + let ptr = ptr.cast::<DevresInner<T>>(); // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the // reference. // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 376f6a6ae5e3..8eca5be30f2c 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -152,7 +152,7 @@ pub(crate) fn to_blk_status(self) -> bindings::blk_status_t { /// Returns the error encoded as a pointer. pub fn to_ptr<T>(self) -> *mut T { // SAFETY: `self.0` is a valid error due to its invariant. - unsafe { bindings::ERR_PTR(self.0.get() as _) as *mut _ } + unsafe { bindings::ERR_PTR(self.0.get() as _).cast() } }
/// Returns a string representing the error, if one exists. diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index c5162fdc95ff..74df815d2f4e 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -58,10 +58,11 @@ impl Firmware { fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result<Self> { let mut fw: *mut bindings::firmware = core::ptr::null_mut(); let pfw: *mut *mut bindings::firmware = &mut fw; + let pfw: *mut *const bindings::firmware = pfw.cast();
// SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer. // `name` and `dev` are valid as by their type invariants. - let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) }; + let ret = unsafe { func.0(pfw, name.as_char_ptr(), dev.as_raw()) }; if ret != 0 { return Err(Error::from_errno(ret)); } diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs index ed57e0137cdb..9e2639aee61a 100644 --- a/rust/kernel/fs/file.rs +++ b/rust/kernel/fs/file.rs @@ -364,7 +364,7 @@ fn deref(&self) -> &LocalFile { // // By the type invariants, there are no `fdget_pos` calls that did not take the // `f_pos_lock` mutex. - unsafe { LocalFile::from_raw_file(self as *const File as *const bindings::file) } + unsafe { LocalFile::from_raw_file((self as *const Self).cast()) } } }
diff --git a/rust/kernel/kunit.rs b/rust/kernel/kunit.rs index 824da0e9738a..7ed2063c1af0 100644 --- a/rust/kernel/kunit.rs +++ b/rust/kernel/kunit.rs @@ -8,19 +8,20 @@
use core::{ffi::c_void, fmt};
+#[cfg(CONFIG_PRINTK)] +use crate::c_str; + /// Prints a KUnit error-level message. /// /// Public but hidden since it should only be used from KUnit generated code. #[doc(hidden)] pub fn err(args: fmt::Arguments<'_>) { + let args: *const _ = &args; // SAFETY: The format string is null-terminated and the `%pA` specifier matches the argument we // are passing. #[cfg(CONFIG_PRINTK)] unsafe { - bindings::_printk( - c"\x013%pA".as_ptr() as _, - &args as *const _ as *const c_void, - ); + bindings::_printk(c_str!("\x013%pA").as_char_ptr(), args.cast::<c_void>()); } }
@@ -29,14 +30,12 @@ pub fn err(args: fmt::Arguments<'_>) { /// Public but hidden since it should only be used from KUnit generated code. #[doc(hidden)] pub fn info(args: fmt::Arguments<'_>) { + let args: *const _ = &args; // SAFETY: The format string is null-terminated and the `%pA` specifier matches the argument we // are passing. #[cfg(CONFIG_PRINTK)] unsafe { - bindings::_printk( - c"\x016%pA".as_ptr() as _, - &args as *const _ as *const c_void, - ); + bindings::_printk(c_str!("\x016%pA").as_char_ptr(), args.cast::<c_void>()); } }
diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs index a0438537cee1..1f9498c1458f 100644 --- a/rust/kernel/list/impl_list_item_mod.rs +++ b/rust/kernel/list/impl_list_item_mod.rs @@ -34,7 +34,7 @@ pub unsafe trait HasListLinks<const ID: u64 = 0> { unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks<ID> { // SAFETY: The caller promises that the pointer is valid. The implementer promises that the // `OFFSET` constant is correct. - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut ListLinks<ID> } + unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast() } } }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 271a7690a9a0..003c9aaafb24 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -75,7 +75,7 @@ extern "C" fn probe_callback( // Let the `struct pci_dev` own a reference of the driver's private data. // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a // `struct pci_dev`. - unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) }; + unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign().cast()) }; } Err(err) => return Error::to_errno(err), } diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index 84a4ecc642a1..26b2502970ef 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -66,7 +66,9 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff // Let the `struct platform_device` own a reference of the driver's private data. // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a // `struct platform_device`. - unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) }; + unsafe { + bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign().cast()) + }; } Err(err) => return Error::to_errno(err), } diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index cf4714242e14..8ae57d2cd36c 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -25,7 +25,7 @@ // SAFETY: The C contract guarantees that `buf` is valid if it's less than `end`. let mut w = unsafe { RawFormatter::from_ptrs(buf.cast(), end.cast()) }; // SAFETY: TODO. - let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) }); + let _ = w.write_fmt(unsafe { *ptr.cast::<fmt::Arguments<'_>>() }); w.pos().cast() }
@@ -102,6 +102,7 @@ pub unsafe fn call_printk( module_name: &[u8], args: fmt::Arguments<'_>, ) { + let args: *const _ = &args; // `_printk` does not seem to fail in any path. #[cfg(CONFIG_PRINTK)] // SAFETY: TODO. @@ -109,7 +110,7 @@ pub unsafe fn call_printk( bindings::_printk( format_string.as_ptr(), module_name.as_ptr(), - &args as *const _ as *const c_void, + args.cast::<c_void>(), ); } } @@ -122,15 +123,13 @@ pub unsafe fn call_printk( #[doc(hidden)] #[cfg_attr(not(CONFIG_PRINTK), allow(unused_variables))] pub fn call_printk_cont(args: fmt::Arguments<'_>) { + let args: *const _ = &args; // `_printk` does not seem to fail in any path. // // SAFETY: The format string is fixed. #[cfg(CONFIG_PRINTK)] unsafe { - bindings::_printk( - format_strings::CONT.as_ptr(), - &args as *const _ as *const c_void, - ); + bindings::_printk(format_strings::CONT.as_ptr(), args.cast::<c_void>()); } }
diff --git a/rust/kernel/seq_file.rs b/rust/kernel/seq_file.rs index 4c03881a9eba..d7a530d3eb07 100644 --- a/rust/kernel/seq_file.rs +++ b/rust/kernel/seq_file.rs @@ -31,12 +31,13 @@ pub unsafe fn from_raw<'a>(ptr: *mut bindings::seq_file) -> &'a SeqFile {
/// Used by the [`seq_print`] macro. pub fn call_printf(&self, args: core::fmt::Arguments<'_>) { + let args: *const _ = &args; // SAFETY: Passing a void pointer to `Arguments` is valid for `%pA`. unsafe { bindings::seq_printf( self.inner.get(), c_str!("%pA").as_char_ptr(), - &args as *const _ as *const crate::ffi::c_void, + args.cast::crate::ffi::c_void(), ); } } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 28e2201604d6..6a1a982b946d 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -191,7 +191,7 @@ pub unsafe fn from_char_ptr<'a>(ptr: *const crate::ffi::c_char) -> &'a Self { // to a `NUL`-terminated C string. let len = unsafe { bindings::strlen(ptr) } + 1; // SAFETY: Lifetime guaranteed by the safety precondition. - let bytes = unsafe { core::slice::from_raw_parts(ptr as _, len) }; + let bytes = unsafe { core::slice::from_raw_parts(ptr.cast(), len) }; // SAFETY: As `len` is returned by `strlen`, `bytes` does not contain interior `NUL`. // As we have added 1 to `len`, the last byte is known to be `NUL`. unsafe { Self::from_bytes_with_nul_unchecked(bytes) } diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs index e105477a3cb1..d04d90486b09 100644 --- a/rust/kernel/sync/poll.rs +++ b/rust/kernel/sync/poll.rs @@ -73,7 +73,7 @@ pub fn register_wait(&mut self, file: &File, cv: &PollCondVar) { // be destroyed, the destructor must run. That destructor first removes all waiters, // and then waits for an rcu grace period. Therefore, `cv.wait_queue_head` is valid for // long enough. - unsafe { qproc(file.as_ptr() as _, cv.wait_queue_head.get(), self.0.get()) }; + unsafe { qproc(file.as_ptr().cast(), cv.wait_queue_head.get(), self.0.get()) }; } } } diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index 0cd100d2aefb..8ff54105be3f 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -170,7 +170,7 @@ impl Queue { pub unsafe fn from_raw<'a>(ptr: *const bindings::workqueue_struct) -> &'a Queue { // SAFETY: The `Queue` type is `#[repr(transparent)]`, so the pointer cast is valid. The // caller promises that the pointer is not dangling. - unsafe { &*(ptr as *const Queue) } + unsafe { &*ptr.cast::<Queue>() } }
/// Enqueues a work item. @@ -457,7 +457,7 @@ fn get_work_offset(&self) -> usize { #[inline] unsafe fn raw_get_work(ptr: *mut Self) -> *mut Work<T, ID> { // SAFETY: The caller promises that the pointer is valid. - unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut Work<T, ID> } + unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<Work<T, ID>>() } }
/// Returns a pointer to the struct containing the [`Work<T, ID>`] field. @@ -472,7 +472,7 @@ unsafe fn work_container_of(ptr: *mut Work<T, ID>) -> *mut Self { // SAFETY: The caller promises that the pointer points at a field of the right type in the // right kind of struct. - unsafe { (ptr as *mut u8).sub(Self::OFFSET) as *mut Self } + unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() } } }
@@ -538,7 +538,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Arc<T> { unsafe extern "C" fn run(ptr: *mut bindings::work_struct) { // The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. - let ptr = ptr as *mut Work<T, ID>; + let ptr = ptr.cast::<Work<T, ID>>(); // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`. let ptr = unsafe { T::work_container_of(ptr) }; // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership. @@ -591,7 +591,7 @@ unsafe impl<T, const ID: u64> WorkItemPointer<ID> for Pin<KBox<T>> { unsafe extern "C" fn run(ptr: *mut bindings::work_struct) { // The `__enqueue` method always uses a `work_struct` stored in a `Work<T, ID>`. - let ptr = ptr as *mut Work<T, ID>; + let ptr = ptr.cast::<Work<T, ID>>(); // SAFETY: This computes the pointer that `__enqueue` got from `Arc::into_raw`. let ptr = unsafe { T::work_container_of(ptr) }; // SAFETY: This pointer comes from `Arc::into_raw` and we've been given back ownership. diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs index 13495910271f..fe9bf7b5a306 100644 --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -15,6 +15,7 @@ #![allow( clippy::all, clippy::undocumented_unsafe_blocks, + clippy::ptr_as_ptr, dead_code, missing_docs, non_camel_case_types,
In Rust 1.72.0, Clippy introduced the `ptr_cast_constness` lint [1]:
Though `as` casts between raw pointers are not terrible, `pointer::cast_mut` and `pointer::cast_const` are safer because they cannot accidentally cast the pointer to another type.
There are only 2 affected sites: - `*mut T as *const U as *mut U` becomes `(*mut T).cast()` - `&self as *const Self as *mut Self` becomes a reference-to-pointer coercion + `(*const Self).cast()`.
Apply these changes and enable the lint -- no functional change intended.
Link: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_cast_constness [1] Reviewed-by: Benno Lossin benno.lossin@proton.me Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + rust/kernel/block/mq/request.rs | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Makefile b/Makefile index ec8efc8e23ba..c62bae2b107b 100644 --- a/Makefile +++ b/Makefile @@ -484,6 +484,7 @@ export rust_common_flags := --edition=2021 \ -Aclippy::needless_lifetimes \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::ptr_as_ptr \ + -Wclippy::ptr_cast_constness \ -Wclippy::undocumented_unsafe_blocks \ -Wclippy::unnecessary_safety_comment \ -Wclippy::unnecessary_safety_doc \ diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs index 7943f43b9575..10c6d69be7f3 100644 --- a/rust/kernel/block/mq/request.rs +++ b/rust/kernel/block/mq/request.rs @@ -69,7 +69,7 @@ pub(crate) unsafe fn aref_from_raw(ptr: *mut bindings::request) -> ARef<Self> { // INVARIANT: By the safety requirements of this function, invariants are upheld. // SAFETY: By the safety requirement of this function, we own a // reference count that we can pass to `ARef`. - unsafe { ARef::from_raw(NonNull::new_unchecked(ptr as *const Self as *mut Self)) } + unsafe { ARef::from_raw(NonNull::new_unchecked(ptr.cast())) } }
/// Notify the block layer that a request is going to be processed now. @@ -151,11 +151,12 @@ pub(crate) unsafe fn wrapper_ptr(this: *mut Self) -> NonNull<RequestDataWrapper> /// Return a reference to the [`RequestDataWrapper`] stored in the private /// area of the request structure. pub(crate) fn wrapper_ref(&self) -> &RequestDataWrapper { + let this: *const _ = self; // SAFETY: By type invariant, `self.0` is a valid allocation. Further, // the private data associated with this request is initialized and // valid. The existence of `&self` guarantees that the private data is // valid as a shared reference. - unsafe { Self::wrapper_ptr(self as *const Self as *mut Self).as_ref() } + unsafe { Self::wrapper_ptr(this.cast_mut()).as_ref() } } }
In Rust 1.66.0, Clippy introduced the `as_ptr_cast_mut` lint [1]:
Since `as_ptr` takes a `&self`, the pointer won’t have write permissions unless interior mutability is used, making it unlikely that having it as a mutable pointer is correct.
There is only one affected callsite, and the change amounts to replacing `as _` with `.cast_mut().cast()`. This doesn't change the semantics, but is more descriptive of what's going on.
Apply this change and enable the lint -- no functional change intended.
Link: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut [1] Reviewed-by: Benno Lossin benno.lossin@proton.me Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + rust/kernel/devres.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/Makefile b/Makefile index c62bae2b107b..bb15b86182a3 100644 --- a/Makefile +++ b/Makefile @@ -477,6 +477,7 @@ export rust_common_flags := --edition=2021 \ -Wrust_2018_idioms \ -Wunreachable_pub \ -Wclippy::all \ + -Wclippy::as_ptr_cast_mut \ -Wclippy::ignored_unit_patterns \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \ diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 3a9d998ec371..598001157293 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -143,7 +143,7 @@ fn remove_action(this: &Arc<Self>) { bindings::devm_remove_action_nowarn( this.dev.as_raw(), Some(this.callback), - this.as_ptr() as _, + this.as_ptr().cast_mut().cast(), ) };
In Rust 1.63.0, Clippy introduced the `as_underscore` lint [1]:
The conversion might include lossy conversion or a dangerous cast that might go undetected due to the type being inferred.
The lint is allowed by default as using `_` is less wordy than always specifying the type.
Always specifying the type is especially helpful in function call contexts where the inferred type may change at a distance. Specifying the type also allows Clippy to spot more cases of `useless_conversion`.
The primary downside is the need to specify the type in trivial getters. There are 4 such functions: 3 have become slightly less ergonomic, 1 was revealed to be a `useless_conversion`.
While this doesn't eliminate unchecked `as` conversions, it makes such conversions easier to scrutinize. It also has the slight benefit of removing a degree of freedom on which to bikeshed. Thus apply the changes and enable the lint -- no functional change intended.
Link: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore [1] Reviewed-by: Benno Lossin benno.lossin@proton.me Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + rust/kernel/block/mq/operations.rs | 2 +- rust/kernel/block/mq/request.rs | 2 +- rust/kernel/device_id.rs | 2 +- rust/kernel/devres.rs | 15 ++++++++------- rust/kernel/error.rs | 2 +- rust/kernel/io.rs | 18 +++++++++--------- rust/kernel/miscdevice.rs | 2 +- rust/kernel/of.rs | 6 +++--- rust/kernel/pci.rs | 9 ++++++--- rust/kernel/str.rs | 8 ++++---- rust/kernel/workqueue.rs | 2 +- 12 files changed, 37 insertions(+), 32 deletions(-)
diff --git a/Makefile b/Makefile index bb15b86182a3..2af40bfed9ce 100644 --- a/Makefile +++ b/Makefile @@ -478,6 +478,7 @@ export rust_common_flags := --edition=2021 \ -Wunreachable_pub \ -Wclippy::all \ -Wclippy::as_ptr_cast_mut \ + -Wclippy::as_underscore \ -Wclippy::ignored_unit_patterns \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \ diff --git a/rust/kernel/block/mq/operations.rs b/rust/kernel/block/mq/operations.rs index 864ff379dc91..d18ef55490da 100644 --- a/rust/kernel/block/mq/operations.rs +++ b/rust/kernel/block/mq/operations.rs @@ -101,7 +101,7 @@ impl<T: Operations> OperationsVTable<T> { if let Err(e) = ret { e.to_blk_status() } else { - bindings::BLK_STS_OK as _ + bindings::BLK_STS_OK as u8 } }
diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs index 10c6d69be7f3..bcf2b73d9189 100644 --- a/rust/kernel/block/mq/request.rs +++ b/rust/kernel/block/mq/request.rs @@ -125,7 +125,7 @@ pub fn end_ok(this: ARef<Self>) -> Result<(), ARef<Self>> { // success of the call to `try_set_end` guarantees that there are no // `ARef`s pointing to this request. Therefore it is safe to hand it // back to the block layer. - unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as _) }; + unsafe { bindings::blk_mq_end_request(request_ptr, bindings::BLK_STS_OK as u8) };
Ok(()) } diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index e5859217a579..4063f09d76d9 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -82,7 +82,7 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> { unsafe { raw_ids[i] .as_mut_ptr() - .byte_offset(T::DRIVER_DATA_OFFSET as _) + .byte_add(T::DRIVER_DATA_OFFSET) .cast::<usize>() .write(i); } diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 598001157293..34571f992f0d 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -45,7 +45,7 @@ struct DevresInner<T> { /// # Example /// /// ```no_run -/// # use kernel::{bindings, c_str, device::Device, devres::Devres, io::{Io, IoRaw}}; +/// # use kernel::{bindings, c_str, device::Device, devres::Devres, ffi::c_void, io::{Io, IoRaw}}; /// # use core::ops::Deref; /// /// // See also [`pci::Bar`] for a real example. @@ -59,19 +59,19 @@ struct DevresInner<T> { /// unsafe fn new(paddr: usize) -> Result<Self>{ /// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is /// // valid for `ioremap`. -/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) }; +/// let addr = unsafe { bindings::ioremap(paddr as bindings::phys_addr_t, SIZE) }; /// if addr.is_null() { /// return Err(ENOMEM); /// } /// -/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?)) +/// Ok(IoMem(IoRaw::new(addr as usize, 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 _); }; +/// unsafe { bindings::iounmap(self.0.addr() as *mut c_void); }; /// } /// } /// @@ -115,8 +115,9 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
// SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is // detached. - let ret = - unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) }; + let ret = unsafe { + bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data.cast_mut().cast()) + };
if ret != 0 { // SAFETY: We just created another reference to `inner` in order to pass it to @@ -130,7 +131,7 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> { }
fn as_ptr(&self) -> *const Self { - self as _ + self }
fn remove_action(this: &Arc<Self>) { diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 8eca5be30f2c..340650abf446 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -152,7 +152,7 @@ pub(crate) fn to_blk_status(self) -> bindings::blk_status_t { /// Returns the error encoded as a pointer. pub fn to_ptr<T>(self) -> *mut T { // SAFETY: `self.0` is a valid error due to its invariant. - unsafe { bindings::ERR_PTR(self.0.get() as _).cast() } + unsafe { bindings::ERR_PTR(self.0.get() as isize).cast() } }
/// Returns a string representing the error, if one exists. diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs index d4a73e52e3ee..9d2aadf40edf 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}; +use crate::{bindings, build_assert, ffi::c_void};
/// Raw representation of an MMIO region. /// @@ -56,7 +56,7 @@ pub fn maxsize(&self) -> usize { /// # Examples /// /// ```no_run -/// # use kernel::{bindings, io::{Io, IoRaw}}; +/// # use kernel::{bindings, ffi::c_void, io::{Io, IoRaw}}; /// # use core::ops::Deref; /// /// // See also [`pci::Bar`] for a real example. @@ -70,19 +70,19 @@ pub fn maxsize(&self) -> usize { /// unsafe fn new(paddr: usize) -> Result<Self>{ /// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is /// // valid for `ioremap`. -/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) }; +/// let addr = unsafe { bindings::ioremap(paddr as bindings::phys_addr_t, SIZE) }; /// if addr.is_null() { /// return Err(ENOMEM); /// } /// -/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?)) +/// Ok(IoMem(IoRaw::new(addr as usize, 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 _); }; +/// unsafe { bindings::iounmap(self.0.addr() as *mut c_void); }; /// } /// } /// @@ -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 _) } + unsafe { bindings::$name(addr as *const c_void) } }
/// 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 _) }) + Ok(unsafe { bindings::$name(addr as *const c_void) }) } }; } @@ -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 _, ) } + unsafe { bindings::$name(value, addr as *mut c_void) } }
/// 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 _) } + unsafe { bindings::$name(value, addr as *mut c_void) } Ok(()) } }; diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs index e14433b2ab9d..2c66e926bffb 100644 --- a/rust/kernel/miscdevice.rs +++ b/rust/kernel/miscdevice.rs @@ -33,7 +33,7 @@ impl MiscDeviceOptions { pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice { // SAFETY: All zeros is valid for this C type. let mut result: bindings::miscdevice = unsafe { MaybeUninit::zeroed().assume_init() }; - result.minor = bindings::MISC_DYNAMIC_MINOR as _; + result.minor = bindings::MISC_DYNAMIC_MINOR as i32; result.name = self.name.as_char_ptr(); result.fops = create_vtable::<T>(); result diff --git a/rust/kernel/of.rs b/rust/kernel/of.rs index 04f2d8ef29cb..40d1bd13682c 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 _ + self.0.data as usize } }
@@ -34,10 +34,10 @@ pub const fn new(compatible: &'static CStr) -> Self { // SAFETY: FFI type is valid to be zero-initialized. let mut of: bindings::of_device_id = unsafe { core::mem::zeroed() };
- // TODO: Use `clone_from_slice` once the corresponding types do match. + // TODO: Use `copy_from_slice` once stabilized for `const`. let mut i = 0; while i < src.len() { - of.compatible[i] = src[i] as _; + of.compatible[i] = src[i]; i += 1; }
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 003c9aaafb24..a26f154ae1b9 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -166,7 +166,7 @@ unsafe impl RawDeviceId for DeviceId { const DRIVER_DATA_OFFSET: usize = core::mem::offset_of!(bindings::pci_device_id, driver_data);
fn index(&self) -> usize { - self.0.driver_data as _ + self.0.driver_data } }
@@ -201,7 +201,10 @@ macro_rules! pci_device_table { /// MODULE_PCI_TABLE, /// <MyDriver as pci::Driver>::IdInfo, /// [ -/// (pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as _), ()) +/// ( +/// pci::DeviceId::from_id(bindings::PCI_VENDOR_ID_REDHAT, bindings::PCI_ANY_ID as u32), +/// (), +/// ) /// ] /// ); /// @@ -317,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 _); + bindings::pci_iounmap(pdev.as_raw(), ioptr as *mut kernel::ffi::c_void); bindings::pci_release_region(pdev.as_raw(), num); } } diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 6a1a982b946d..0b80a119d5f0 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 _, - pos: pos as _, - end: end as _, + beg: pos as usize, + pos: pos as usize, + end: end as usize, } }
@@ -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 _ + self.pos as *mut u8 }
/// Returns the number of bytes written to the formatter. diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs index 8ff54105be3f..d03f3440cb5a 100644 --- a/rust/kernel/workqueue.rs +++ b/rust/kernel/workqueue.rs @@ -198,7 +198,7 @@ pub fn enqueue<W, const ID: u64>(&self, w: W) -> W::EnqueueOutput unsafe { w.__enqueue(move |work_ptr| { bindings::queue_work_on( - bindings::wq_misc_consts_WORK_CPU_UNBOUND as _, + bindings::wq_misc_consts_WORK_CPU_UNBOUND as i32, queue_ptr, work_ptr, )
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-api... [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(-)
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_p.... + #[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.htm... + #[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/4291332175d12e79e6061cdc3f5dccac2e2... through + // https://github.com/rust-lang/rust/blob/1.84.0/library/core/src/ptr/const_ptr... + // 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_p.... + #[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.htm... + #[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, )
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 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 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.
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.
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.
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.
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.
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:
- 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.
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 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 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:
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().
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_p.... - #[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.htm... - #[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/4291332175d12e79e6061cdc3f5dccac2e2... through - // https://github.com/rust-lang/rust/blob/1.84.0/library/core/src/ptr/const_ptr... - // 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_p.... + 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_p.... - #[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 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:
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().
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_p....
- #[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.htm...
- #[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_p....
- #[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 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_p....
- #[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 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 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-api... [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_p....
- #[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.htm...
- #[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: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-api... [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-api... [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 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-api... [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-api... [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-api... [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-api... [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.
- 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.
- 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 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-api... [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.
- 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.
- 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 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 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 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 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
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-api... [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.
- 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.
- 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.
- 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 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-api... [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.
- 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.
- 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 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 Mon, Mar 17, 2025 at 10:23 AM Tamir Duberstein tamird@gmail.com wrote:
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
Link: https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/ [1]
Signed-off-by: Tamir Duberstein tamird@gmail.com
Changes in v5:
- Use `pointer::addr` in OF. (Boqun Feng)
- Add documentation on stubs. (Benno Lossin)
- Mark stubs `#[inline]`.
- Pick up Alice's RB on a shared commit from https://lore.kernel.org/all/Z9f-3Aj3_FWBZRrm@google.com/.
- Link to v4: https://lore.kernel.org/r/20250315-ptr-as-ptr-v4-0-b2d72c14dc26@gmail.com
Changes in v4:
- Add missing SoB. (Benno Lossin)
- Use `without_provenance_mut` in alloc. (Boqun Feng)
- Limit strict provenance lints to the `kernel` crate to avoid complex logic in the build system. This can be revisited on MSRV >= 1.84.0.
- Rebase on rust-next.
- Link to v3: https://lore.kernel.org/r/20250314-ptr-as-ptr-v3-0-e7ba61048f4a@gmail.com
Changes in v3:
- Fixed clippy warning in rust/kernel/firmware.rs. (kernel test robot) Link: https://lore.kernel.org/all/202503120332.YTCpFEvv-lkp@intel.com/
- s/as u64/as bindings::phys_addr_t/g. (Benno Lossin)
- Use strict provenance APIs and enable lints. (Benno Lossin)
- Link to v2: https://lore.kernel.org/r/20250309-ptr-as-ptr-v2-0-25d60ad922b7@gmail.com
Changes in v2:
- Fixed typo in first commit message.
- Added additional patches, converted to series.
- Link to v1: https://lore.kernel.org/r/20250307-ptr-as-ptr-v1-1-582d06514c98@gmail.com
Tamir Duberstein (6): rust: retain pointer mut-ness in `container_of!` rust: enable `clippy::ptr_as_ptr` lint rust: enable `clippy::ptr_cast_constness` lint rust: enable `clippy::as_ptr_cast_mut` lint rust: enable `clippy::as_underscore` lint rust: use strict provenance APIs
Makefile | 4 ++ init/Kconfig | 3 + rust/bindings/lib.rs | 1 + rust/kernel/alloc.rs | 2 +- rust/kernel/alloc/allocator_test.rs | 2 +- rust/kernel/alloc/kvec.rs | 4 +- rust/kernel/block/mq/operations.rs | 2 +- rust/kernel/block/mq/request.rs | 7 +- rust/kernel/device.rs | 5 +- rust/kernel/device_id.rs | 2 +- rust/kernel/devres.rs | 19 +++--- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 +- rust/kernel/fs/file.rs | 2 +- rust/kernel/io.rs | 16 ++--- rust/kernel/kunit.rs | 15 ++--- rust/kernel/lib.rs | 113 ++++++++++++++++++++++++++++++++- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/of.rs | 6 +- rust/kernel/pci.rs | 15 +++-- rust/kernel/platform.rs | 6 +- rust/kernel/print.rs | 11 ++-- rust/kernel/rbtree.rs | 23 +++---- rust/kernel/seq_file.rs | 3 +- rust/kernel/str.rs | 18 ++---- rust/kernel/sync/poll.rs | 2 +- rust/kernel/uaccess.rs | 12 ++-- rust/kernel/workqueue.rs | 12 ++-- rust/uapi/lib.rs | 1 + 30 files changed, 218 insertions(+), 97 deletions(-)
base-commit: 498f7ee4773f22924f00630136da8575f38954e8 change-id: 20250307-ptr-as-ptr-21b1867fc4d4
Per the discussion in today's Rust-for-Linux meeting and Benno's reply[0] please take this series without the last patch, whenever you see fit to do so. At this time no changes have been requested to the first 5 patches, so I am not planning to respin.
Cheers. Tamir
[0] https://lore.kernel.org/all/D8KIHNXCPE0P.K4MD7QJ1AC17@proton.me/
On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote:
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
I applied the patches to v6.14-rc7 and did a quick pass with
rg -nC 3 -t rust ' as ' | bat -l rust
to see if there are any cases left that we could fix and I found a couple:
* there are several cases of `number as int_type` (like `num as c_int` or `my_u32 as usize` etc.) not sure what we can do about these, some are probably unavoidable, but since the kernel doesn't support 16 bit systems (that is true, right?), we *could* have a `From<u32> for usize` impl... * some instances of `'|' as u32` (samples/rust/rust_misc_device.rs:112). There is a `From<char> for u32` impl, so this can just be replaced with `.into()` (or maybe by using a byte literal `b'|'`?). * `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this. * some pointer casts in rust/kernel/list/impl_list_item_mod.rs:{253,254} not sure if they can be converted though (maybe they are unsizing the pointer?) Another pointer cast in rust/kernel/driver.rs:81 (I'm pretty sure this one can be replaced by a `.cast()`)
Some clippy lints that we could also enable that share the spirit of this series:
* `char_lit_as_u8` (maybe that also covers the `'|' as u32` case from above?) * `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
Tamir Duberstein (6): rust: retain pointer mut-ness in `container_of!` rust: enable `clippy::ptr_as_ptr` lint rust: enable `clippy::ptr_cast_constness` lint rust: enable `clippy::as_ptr_cast_mut` lint rust: enable `clippy::as_underscore` lint rust: use strict provenance APIs
Makefile | 4 ++ init/Kconfig | 3 + rust/bindings/lib.rs | 1 + rust/kernel/alloc.rs | 2 +- rust/kernel/alloc/allocator_test.rs | 2 +- rust/kernel/alloc/kvec.rs | 4 +- rust/kernel/block/mq/operations.rs | 2 +- rust/kernel/block/mq/request.rs | 7 +- rust/kernel/device.rs | 5 +- rust/kernel/device_id.rs | 2 +- rust/kernel/devres.rs | 19 +++--- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 +- rust/kernel/fs/file.rs | 2 +- rust/kernel/io.rs | 16 ++--- rust/kernel/kunit.rs | 15 ++--- rust/kernel/lib.rs | 113 ++++++++++++++++++++++++++++++++- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/of.rs | 6 +- rust/kernel/pci.rs | 15 +++-- rust/kernel/platform.rs | 6 +- rust/kernel/print.rs | 11 ++-- rust/kernel/rbtree.rs | 23 +++---- rust/kernel/seq_file.rs | 3 +- rust/kernel/str.rs | 18 ++---- rust/kernel/sync/poll.rs | 2 +- rust/kernel/uaccess.rs | 12 ++-- rust/kernel/workqueue.rs | 12 ++-- rust/uapi/lib.rs | 1 + 30 files changed, 218 insertions(+), 97 deletions(-)
base-commit: 498f7ee4773f22924f00630136da8575f38954e8
Btw I didn't find this commit anywhere I usually check, where is it from?
--- Cheers, Benno
change-id: 20250307-ptr-as-ptr-21b1867fc4d4
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote:
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
I applied the patches to v6.14-rc7 and did a quick pass with
rg -nC 3 -t rust ' as ' | bat -l rustto see if there are any cases left that we could fix and I found a couple:
- there are several cases of `number as int_type` (like `num as c_int` or `my_u32 as usize` etc.) not sure what we can do about these, some are probably unavoidable, but since the kernel doesn't support 16 bit systems (that is true, right?), we *could* have a `From<u32> for usize` impl...
Yeah, these are the most difficult ones to get rid of.
- some instances of `'|' as u32` (samples/rust/rust_misc_device.rs:112). There is a `From<char> for u32` impl, so this can just be replaced with `.into()` (or maybe by using a byte literal `b'|'`?).
We can enable https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow#cast_... for this one.
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I think there's not a focused one. There's a nuclear option: https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow#as_co...
- some pointer casts in rust/kernel/list/impl_list_item_mod.rs:{253,254} not sure if they can be converted though (maybe they are unsizing the pointer?)
I have a local series that gets rid of these by doing similar things to https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/. I can send it later this week but it probably can't land until Alice is back from vacation; she was the author of this code.
Another pointer cast in rust/kernel/driver.rs:81 (I'm pretty sure this one can be replaced by a `.cast()`)
Some clippy lints that we could also enable that share the spirit of this series:
- `char_lit_as_u8` (maybe that also covers the `'|' as u32` case from above?)
It's already enabled, it's warn-by-default.
- `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
Yeah, suggested the same above. I had hoped this would deal with the char as u32 pattern but it did not.
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Tamir Duberstein (6): rust: retain pointer mut-ness in `container_of!` rust: enable `clippy::ptr_as_ptr` lint rust: enable `clippy::ptr_cast_constness` lint rust: enable `clippy::as_ptr_cast_mut` lint rust: enable `clippy::as_underscore` lint rust: use strict provenance APIs
Makefile | 4 ++ init/Kconfig | 3 + rust/bindings/lib.rs | 1 + rust/kernel/alloc.rs | 2 +- rust/kernel/alloc/allocator_test.rs | 2 +- rust/kernel/alloc/kvec.rs | 4 +- rust/kernel/block/mq/operations.rs | 2 +- rust/kernel/block/mq/request.rs | 7 +- rust/kernel/device.rs | 5 +- rust/kernel/device_id.rs | 2 +- rust/kernel/devres.rs | 19 +++--- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 +- rust/kernel/fs/file.rs | 2 +- rust/kernel/io.rs | 16 ++--- rust/kernel/kunit.rs | 15 ++--- rust/kernel/lib.rs | 113 ++++++++++++++++++++++++++++++++- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/of.rs | 6 +- rust/kernel/pci.rs | 15 +++-- rust/kernel/platform.rs | 6 +- rust/kernel/print.rs | 11 ++-- rust/kernel/rbtree.rs | 23 +++---- rust/kernel/seq_file.rs | 3 +- rust/kernel/str.rs | 18 ++---- rust/kernel/sync/poll.rs | 2 +- rust/kernel/uaccess.rs | 12 ++-- rust/kernel/workqueue.rs | 12 ++-- rust/uapi/lib.rs | 1 + 30 files changed, 218 insertions(+), 97 deletions(-)
base-commit: 498f7ee4773f22924f00630136da8575f38954e8
Btw I didn't find this commit anywhere I usually check, where is it from?
It was probably nowhere, a local frankenstein that included some fixes from rust-fixes.
On Mon, Mar 24, 2025 at 4:55 PM Tamir Duberstein tamird@gmail.com wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote:
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
I applied the patches to v6.14-rc7 and did a quick pass with
So I rebased this on rust-next and fixed a few more instances (in addition to enabling the extra lint), but I realized that rust-next is still based on v6.14-rc5. I think we're going to have the same problem here as in the &raw series; either Miguel is going to have to apply fixups when picking these patches, or we have to split the fixes out from the lints and land it over several cycles. Thoughts?
On Mon Mar 24, 2025 at 10:16 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:55 PM Tamir Duberstein tamird@gmail.com wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 17, 2025 at 3:23 PM CET, Tamir Duberstein wrote:
This started with a patch that enabled `clippy::ptr_as_ptr`. Benno Lossin suggested I also look into `clippy::ptr_cast_constness` and I discovered `clippy::as_ptr_cast_mut`. This series now enables all 3 lints. It also enables `clippy::as_underscore` which ensures other pointer casts weren't missed. The first commit reduces the need for pointer casts and is shared with another series[1].
The final patch also enables pointer provenance lints and fixes violations. See that commit message for details. The build system portion of that commit is pretty messy but I couldn't find a better way to convincingly ensure that these lints were applied globally. Suggestions would be very welcome.
I applied the patches to v6.14-rc7 and did a quick pass with
So I rebased this on rust-next and fixed a few more instances (in addition to enabling the extra lint), but I realized that rust-next is still based on v6.14-rc5. I think we're going to have the same problem here as in the &raw series; either Miguel is going to have to apply fixups when picking these patches, or we have to split the fixes out from the lints and land it over several cycles. Thoughts?
One option is to pick the patches early in the cycle and then ask authors of patches to rebase. We did that with the alloc changes in the past.
--- Cheers, Benno
On Mon, Mar 24, 2025 at 4:55 PM Tamir Duberstein tamird@gmail.com wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
- some pointer casts in rust/kernel/list/impl_list_item_mod.rs:{253,254} not sure if they can be converted though (maybe they are unsizing the pointer?)
I have a local series that gets rid of these by doing similar things to https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/. I can send it later this week but it probably can't land until Alice is back from vacation; she was the author of this code.
https://lore.kernel.org/all/20250324-list-no-offset-v1-0-afd2b7fc442a@gmail....
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I think there's not a focused one. There's a nuclear option: https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow#as_co...
Yeah I saw that one, I don't think it's a good idea, since there will be false positives.
- some pointer casts in rust/kernel/list/impl_list_item_mod.rs:{253,254} not sure if they can be converted though (maybe they are unsizing the pointer?)
I have a local series that gets rid of these by doing similar things to https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/. I can send it later this week but it probably can't land until Alice is back from vacation; she was the author of this code.
No worries, as I wrote below, I think it's fine to do that in a new series.
Another pointer cast in rust/kernel/driver.rs:81 (I'm pretty sure this one can be replaced by a `.cast()`)
Some clippy lints that we could also enable that share the spirit of this series:
- `char_lit_as_u8` (maybe that also covers the `'|' as u32` case from above?)
It's already enabled, it's warn-by-default.
Ah I see, didn't look :)
- `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
Yeah, suggested the same above. I had hoped this would deal with the char as u32 pattern but it did not.
Aw that's a shame. Maybe we should create a clippy issue for that, thoughts?
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Also I wouldn't mark it as a good-first-issue yet, since it's pretty complicated and needs to be delayed/based on this series.
--- Cheers, Benno
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I think there's not a focused one. There's a nuclear option: https://rust-lang.github.io/rust-clippy/master/index.html?levels=allow#as_co...
Yeah I saw that one, I don't think it's a good idea, since there will be false positives.
- some pointer casts in rust/kernel/list/impl_list_item_mod.rs:{253,254} not sure if they can be converted though (maybe they are unsizing the pointer?)
I have a local series that gets rid of these by doing similar things to https://lore.kernel.org/all/20250307-no-offset-v1-0-0c728f63b69c@gmail.com/. I can send it later this week but it probably can't land until Alice is back from vacation; she was the author of this code.
No worries, as I wrote below, I think it's fine to do that in a new series.
Another pointer cast in rust/kernel/driver.rs:81 (I'm pretty sure this one can be replaced by a `.cast()`)
Some clippy lints that we could also enable that share the spirit of this series:
- `char_lit_as_u8` (maybe that also covers the `'|' as u32` case from above?)
It's already enabled, it's warn-by-default.
Ah I see, didn't look :)
- `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
Yeah, suggested the same above. I had hoped this would deal with the char as u32 pattern but it did not.
Aw that's a shame. Maybe we should create a clippy issue for that, thoughts?
Yeah, it's not clear to me why it isn't covered by `cast_lossless`. Might just be a bug. Want to file it?
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Also I wouldn't mark it as a good-first-issue yet, since it's pretty complicated and needs to be delayed/based on this series.
Yeah, certainly not good-first-issue.
On Mon Mar 24, 2025 at 10:59 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
- `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
Yeah, suggested the same above. I had hoped this would deal with the char as u32 pattern but it did not.
Aw that's a shame. Maybe we should create a clippy issue for that, thoughts?
Yeah, it's not clear to me why it isn't covered by `cast_lossless`. Might just be a bug. Want to file it?
Done: https://github.com/rust-lang/rust-clippy/issues/14469
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Yes, but there are also these from my original mail: * `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
And the other points (haven't taken a look at the other series you submitted, so I don't know to what extend you fixed the other `as` casts I mentioned). So I figured you might know which ones we still have after applying all your patches :)
--- Cheers, Benno
On Tue, Mar 25, 2025 at 12:05 PM Benno Lossin benno.lossin@proton.me wrote:
Linked in our list:
https://github.com/Rust-for-Linux/linux/issues/349
Cheers, Miguel
On Tue, Mar 25, 2025 at 7:05 AM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 10:59 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
- `cast_lossless` (maybe this catches some of the `num as int_type` conversions I mentioned above)
Yeah, suggested the same above. I had hoped this would deal with the char as u32 pattern but it did not.
Aw that's a shame. Maybe we should create a clippy issue for that, thoughts?
Yeah, it's not clear to me why it isn't covered by `cast_lossless`. Might just be a bug. Want to file it?
Nice, looks like there's already a PR out: https://github.com/rust-lang/rust-clippy/pull/14470.
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Yes, but there are also these from my original mail:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I don't think we should go fixing things for which we don't have a clippy lint. That way lies madness, particularly as patches begin to be carried by other trees.
And the other points (haven't taken a look at the other series you submitted, so I don't know to what extend you fixed the other `as` casts I mentioned). So I figured you might know which ones we still have after applying all your patches :)
Cheers, Benno
On Tue Mar 25, 2025 at 2:34 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 7:05 AM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 10:59 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote:
I'll leave it up to you what you want to do with this: add it to this series, make a new one, or let someone else handle it. If you don't want to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Yes, but there are also these from my original mail:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I don't think we should go fixing things for which we don't have a clippy lint. That way lies madness, particularly as patches begin to be carried by other trees.
There already exists a lint for that: `clippy::ref_as_ptr` (almost created an issue asking for one :)
Here is another lint that we probably want to enable (after the `&raw {const,mut}` series lands): `clippy::borrow_as_ptr`.
--- Cheers, Benno
On Tue, Mar 25, 2025 at 11:33 AM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 2:34 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 7:05 AM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 10:59 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote: > I'll leave it up to you what you want to do with this: add it to this > series, make a new one, or let someone else handle it. If you don't want > to handle it, let me know, then I'll create a good-first-issue :)
I'll add a patch for `cast_lossless` -- the rest should probably go into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Yes, but there are also these from my original mail:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I don't think we should go fixing things for which we don't have a clippy lint. That way lies madness, particularly as patches begin to be carried by other trees.
There already exists a lint for that: `clippy::ref_as_ptr` (almost created an issue asking for one :)
🫡 picked this one up as well.
Here is another lint that we probably want to enable (after the `&raw {const,mut}` series lands): `clippy::borrow_as_ptr`.
This sounds like a good one to file.
On Tue, Mar 25, 2025 at 1:17 PM Tamir Duberstein tamird@gmail.com wrote:
On Tue, Mar 25, 2025 at 11:33 AM Benno Lossin benno.lossin@proton.me wrote:
Here is another lint that we probably want to enable (after the `&raw {const,mut}` series lands): `clippy::borrow_as_ptr`.
This sounds like a good one to file.
Actually I just enabled this and it has no incremental effect relative to the lints already enabled in the series. We can enable it now, but it seems to only trigger on patterns already caught by `ref_as_ptr`.
On Tue Mar 25, 2025 at 6:17 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 11:33 AM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 2:34 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 7:05 AM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 10:59 PM CET, Tamir Duberstein wrote:
On Mon, Mar 24, 2025 at 5:55 PM Benno Lossin benno.lossin@proton.me wrote:
On Mon Mar 24, 2025 at 9:55 PM CET, Tamir Duberstein wrote: > On Mon, Mar 24, 2025 at 4:16 PM Benno Lossin benno.lossin@proton.me wrote: >> I'll leave it up to you what you want to do with this: add it to this >> series, make a new one, or let someone else handle it. If you don't want >> to handle it, let me know, then I'll create a good-first-issue :) > > I'll add a patch for `cast_lossless` -- the rest should probably go > into an issue.
Do you mind filing the issue? Then you can decide yourself what you want to do yourself vs what you want to leave for others. Feel free to copy from my mail summary.
Well, I don't really know what's left to do. We're pretty close at this point to having enabled everything but the nukes. Then there's the strict provenance thing, which I suppose we can write down.
Yes, but there are also these from my original mail:
- `shared_ref as *const _` (for example in rust/kernel/uaccess.rs:247, rust/kernel/str.rs:32 and rust/kernel/fs/file.rs:367), these we can replace with `let ptr: *const ... = shared_ref;`. Don't know if there is a clippy lint for this.
I don't think we should go fixing things for which we don't have a clippy lint. That way lies madness, particularly as patches begin to be carried by other trees.
There already exists a lint for that: `clippy::ref_as_ptr` (almost created an issue asking for one :)
🫡 picked this one up as well.
Sniped yet again :)
Thanks a lot for adding this one as well.
Here is another lint that we probably want to enable (after the `&raw {const,mut}` series lands): `clippy::borrow_as_ptr`.
This sounds like a good one to file.
Since `raw_ref_op` is already enabled in rust-next, I just filed it:
https://github.com/Rust-for-Linux/linux/issues/1152
--- Cheers, Benno
linux-kselftest-mirror@lists.linaro.org
-
Alice Ryhl -
Benno Lossin -
Boqun Feng -
Miguel Ojeda -
Tamir Duberstein