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].
As a later addition, `clippy::cast_lossless` and `clippy::ref_as_ptr` are also enabled.
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 v7: - Add patch to enable `clippy::ref_as_ptr`. - Link to v6: https://lore.kernel.org/r/20250324-ptr-as-ptr-v6-0-49d1b7fd4290@gmail.com
Changes in v6: - Drop strict provenance patch. - Fix URLs in doc comments. - Add patch to enable `clippy::cast_lossless`. - Rebase on rust-next. - Link to v5: https://lore.kernel.org/r/20250317-ptr-as-ptr-v5-0-5b5f21fa230a@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 (7): 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: enable `clippy::cast_lossless` lint rust: enable `clippy::ref_as_ptr` lint
Makefile | 6 ++++++ drivers/gpu/drm/drm_panic_qr.rs | 10 +++++----- rust/bindings/lib.rs | 3 +++ 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 | 5 +++-- rust/kernel/devres.rs | 19 ++++++++++--------- rust/kernel/dma.rs | 6 +++--- rust/kernel/error.rs | 2 +- rust/kernel/firmware.rs | 3 ++- rust/kernel/fs/file.rs | 3 ++- rust/kernel/io.rs | 18 +++++++++--------- rust/kernel/kunit.rs | 15 +++++++-------- rust/kernel/lib.rs | 5 ++--- rust/kernel/list/impl_list_item_mod.rs | 2 +- rust/kernel/miscdevice.rs | 2 +- rust/kernel/net/phy.rs | 4 ++-- rust/kernel/of.rs | 6 +++--- rust/kernel/pci.rs | 13 ++++++++----- 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 | 14 +++++++------- rust/kernel/sync/poll.rs | 2 +- rust/kernel/uaccess.rs | 5 +++-- rust/kernel/workqueue.rs | 12 ++++++------ rust/uapi/lib.rs | 3 +++ 31 files changed, 120 insertions(+), 101 deletions(-) --- base-commit: 28bb48c4cb34f65a9aa602142e76e1426da31293 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 ba0f3b0297b2..cffa0d837f06 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -190,7 +190,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) }; @@ -199,9 +199,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 5246b2c8a4ff..8d978c896747 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/dma.rs | 4 ++-- 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 + 20 files changed, 42 insertions(+), 36 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 e3240d16040b..30ce85326a50 100644 --- a/rust/kernel/alloc/allocator_test.rs +++ b/rust/kernel/alloc/allocator_test.rs @@ -64,7 +64,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/dma.rs b/rust/kernel/dma.rs index 8cdc76043ee7..f395d1a6fe48 100644 --- a/rust/kernel/dma.rs +++ b/rust/kernel/dma.rs @@ -186,7 +186,7 @@ pub fn alloc_attrs( dev: dev.into(), dma_handle, count, - cpu_addr: ret as *mut T, + cpu_addr: ret.cast(), dma_attrs, }) } @@ -293,7 +293,7 @@ fn drop(&mut self) { bindings::dma_free_attrs( self.dev.as_raw(), size, - self.cpu_addr as _, + self.cpu_addr.cast(), self.dma_handle, self.dma_attrs.as_raw(), ) diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 30014d507ed3..b0e3d1bc0449 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -153,7 +153,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 1604fb6a5b1b..83d15cfcda84 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 878111cb77bc..02863c40c21b 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -237,7 +237,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..f03b7aead35a 100644 --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -14,6 +14,7 @@ #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #![allow( clippy::all, + clippy::ptr_as_ptr, clippy::undocumented_unsafe_blocks, dead_code, missing_docs,
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 +++-- rust/kernel/dma.rs | 2 +- 3 files changed, 5 insertions(+), 3 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 4a5b7ec914ef..c9f8046af65c 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() } } }
diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs index f395d1a6fe48..43ecf3c2e860 100644 --- a/rust/kernel/dma.rs +++ b/rust/kernel/dma.rs @@ -186,7 +186,7 @@ pub fn alloc_attrs( dev: dev.into(), dma_handle, count, - cpu_addr: ret.cast(), + cpu_addr: ret.cast::<T>(), dma_attrs, }) }
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/dma.rs | 2 +- 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 +- 13 files changed, 38 insertions(+), 33 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 c9f8046af65c..807a72de6455 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/dma.rs b/rust/kernel/dma.rs index 43ecf3c2e860..851a6339aa90 100644 --- a/rust/kernel/dma.rs +++ b/rust/kernel/dma.rs @@ -38,7 +38,7 @@ impl Attrs { /// Get the raw representation of this attribute. pub(crate) fn as_raw(self) -> crate::ffi::c_ulong { - self.0 as _ + self.0 as crate::ffi::c_ulong }
/// Check whether `flags` is contained in `self`. diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index b0e3d1bc0449..cff84d427627 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -153,7 +153,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 02863c40c21b..40034f77fc2f 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -738,9 +738,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, } }
@@ -765,7 +765,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, )
Before Rust 1.29.0, Clippy introduced the `cast_lossless` lint [1]:
Rust’s `as` keyword will perform many kinds of conversions, including silently lossy conversions. Conversion functions such as `i32::from` will only perform lossless conversions. Using the conversion functions prevents conversions from becoming silently lossy if the input types ever change, and makes it clear for people reading the code that the conversion is lossless.
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#cast_lossless [1] Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://lore.kernel.org/all/D8ORTXSUTKGL.1KOJAGBM8F8TN@proton.me/ Reviewed-by: Benno Lossin benno.lossin@proton.me Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + drivers/gpu/drm/drm_panic_qr.rs | 10 +++++----- rust/bindings/lib.rs | 1 + rust/kernel/net/phy.rs | 4 ++-- rust/uapi/lib.rs | 1 + 5 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/Makefile b/Makefile index 2af40bfed9ce..2e9eca8b7671 100644 --- a/Makefile +++ b/Makefile @@ -479,6 +479,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::all \ -Wclippy::as_ptr_cast_mut \ -Wclippy::as_underscore \ + -Wclippy::cast_lossless \ -Wclippy::ignored_unit_patterns \ -Wclippy::mut_mut \ -Wclippy::needless_bitwise_bool \ diff --git a/drivers/gpu/drm/drm_panic_qr.rs b/drivers/gpu/drm/drm_panic_qr.rs index ecd87e8ffe05..01337ce896df 100644 --- a/drivers/gpu/drm/drm_panic_qr.rs +++ b/drivers/gpu/drm/drm_panic_qr.rs @@ -305,15 +305,15 @@ fn get_next_13b(data: &[u8], offset: usize) -> Option<(u16, usize)> { // `b` is 20 at max (`bit_off` <= 7 and `size` <= 13). let b = (bit_off + size) as u16;
- let first_byte = (data[byte_off] << bit_off >> bit_off) as u16; + let first_byte = u16::from(data[byte_off] << bit_off >> bit_off);
let number = match b { 0..=8 => first_byte >> (8 - b), - 9..=16 => (first_byte << (b - 8)) + (data[byte_off + 1] >> (16 - b)) as u16, + 9..=16 => (first_byte << (b - 8)) + u16::from(data[byte_off + 1] >> (16 - b)), _ => { (first_byte << (b - 8)) - + ((data[byte_off + 1] as u16) << (b - 16)) - + (data[byte_off + 2] >> (24 - b)) as u16 + + u16::from(data[byte_off + 1] << (b - 16)) + + u16::from(data[byte_off + 2] >> (24 - b)) } }; Some((number, size)) @@ -414,7 +414,7 @@ fn next(&mut self) -> OptionSelf::Item { match self.segment { Segment::Binary(data) => { if self.offset < data.len() { - let byte = data[self.offset] as u16; + let byte = data[self.offset].into(); self.offset += 1; Some((byte, 8)) } else { diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs index 0486a32ed314..b105a0d899cc 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -25,6 +25,7 @@ )]
#[allow(dead_code)] +#[allow(clippy::cast_lossless)] #[allow(clippy::ptr_as_ptr)] #[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index a59469c785e3..abc58b4d1bf4 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -142,7 +142,7 @@ pub fn is_autoneg_enabled(&self) -> bool { // SAFETY: The struct invariant ensures that we may access // this field without additional synchronization. let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; - bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64 + bit_field.get(13, 1) == bindings::AUTONEG_ENABLE.into() }
/// Gets the current auto-negotiation state. @@ -426,7 +426,7 @@ impl<T: Driver> Adapter<T> { // where we hold `phy_device->lock`, so the accessors on // `Device` are okay to call. let dev = unsafe { Device::from_raw(phydev) }; - T::match_phy_device(dev) as i32 + T::match_phy_device(dev).into() }
/// # Safety diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs index f03b7aead35a..d5dab4dfabec 100644 --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -14,6 +14,7 @@ #![cfg_attr(test, allow(unsafe_op_in_unsafe_fn))] #![allow( clippy::all, + clippy::cast_lossless, clippy::ptr_as_ptr, clippy::undocumented_unsafe_blocks, dead_code,
In Rust 1.78.0, Clippy introduced the `ref_as_ptr` lint [1]:
Using `as` casts may result in silently changing mutability or type.
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#ref_as_ptr [1] Suggested-by: Benno Lossin benno.lossin@proton.me Link: https://lore.kernel.org/all/D8PGG7NTWB6U.3SS3A5LN4XWMN@proton.me/ Signed-off-by: Tamir Duberstein tamird@gmail.com --- Makefile | 1 + rust/bindings/lib.rs | 1 + rust/kernel/device_id.rs | 3 ++- rust/kernel/fs/file.rs | 3 ++- rust/kernel/str.rs | 4 ++-- rust/kernel/uaccess.rs | 5 +++-- rust/uapi/lib.rs | 1 + 7 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile index 2e9eca8b7671..081b5fa2aa81 100644 --- a/Makefile +++ b/Makefile @@ -488,6 +488,7 @@ export rust_common_flags := --edition=2021 \ -Wclippy::no_mangle_with_rust_abi \ -Wclippy::ptr_as_ptr \ -Wclippy::ptr_cast_constness \ + -Wclippy::ref_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 b105a0d899cc..2b69016070c6 100644 --- a/rust/bindings/lib.rs +++ b/rust/bindings/lib.rs @@ -27,6 +27,7 @@ #[allow(dead_code)] #[allow(clippy::cast_lossless)] #[allow(clippy::ptr_as_ptr)] +#[allow(clippy::ref_as_ptr)] #[allow(clippy::undocumented_unsafe_blocks)] mod bindings_raw { // Manual definition for blocklisted types. diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs index 4063f09d76d9..37cc03d1df4c 100644 --- a/rust/kernel/device_id.rs +++ b/rust/kernel/device_id.rs @@ -136,7 +136,8 @@ impl<T: RawDeviceId, U, const N: usize> IdTable<T, U> for IdArray<T, U, N> { fn as_ptr(&self) -> *const T::RawType { // This cannot be `self.ids.as_ptr()`, as the return pointer must have correct provenance // to access the sentinel. - (self as *const Self).cast() + let this: *const Self = self; + this.cast() }
fn id(&self, index: usize) -> &T::RawType { diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs index 9e2639aee61a..366e23dc0803 100644 --- a/rust/kernel/fs/file.rs +++ b/rust/kernel/fs/file.rs @@ -359,12 +359,13 @@ impl core::ops::Deref for File { type Target = LocalFile; #[inline] fn deref(&self) -> &LocalFile { + let this: *const Self = self; // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a // valid file for the desired duration. // // 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 Self).cast()) } + unsafe { LocalFile::from_raw_file(this.cast()) } } }
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`. - unsafe { &*(bytes as *const [u8] as *const BStr) } + unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) } }
/// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`]. @@ -290,7 +290,7 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError #[inline] pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { // SAFETY: Properties of `bytes` guaranteed by the safety precondition. - unsafe { &mut *(bytes as *mut [u8] as *mut CStr) } + unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut Self>(bytes)) } }
/// Returns a C pointer to the string. diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..c042b1fe499e 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -242,7 +242,7 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { pub fn read_slice(&mut self, out: &mut [u8]) -> Result { // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to // `out`. - let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; + let out = unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut [MaybeUninit<u8>]>(out)) }; self.read_raw(out) }
@@ -348,6 +348,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { if len > self.length { return Err(EFAULT); } + let value: *const T = value; // SAFETY: The reference points to a value of type `T`, so it is valid for reading // `size_of::<T>()` bytes. // @@ -357,7 +358,7 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Result { let res = unsafe { bindings::_copy_to_user( self.ptr as *mut c_void, - (value as *const T).cast::<c_void>(), + value.cast::<c_void>(), len, ) }; diff --git a/rust/uapi/lib.rs b/rust/uapi/lib.rs index d5dab4dfabec..6230ba48201d 100644 --- a/rust/uapi/lib.rs +++ b/rust/uapi/lib.rs @@ -16,6 +16,7 @@ clippy::all, clippy::cast_lossless, clippy::ptr_as_ptr, + clippy::ref_as_ptr, clippy::undocumented_unsafe_blocks, dead_code, missing_docs,
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }
Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
}/// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`]. @@ -290,7 +290,7 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError #[inline] pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { // SAFETY: Properties of `bytes` guaranteed by the safety precondition.
unsafe { &mut *(bytes as *mut [u8] as *mut CStr) }
unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut Self>(bytes)) }/// Returns a C pointer to the string. diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..c042b1fe499e 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -242,7 +242,7 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { pub fn read_slice(&mut self, out: &mut [u8]) -> Result { // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to // `out`.
let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) };
let out = unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut [MaybeUninit<u8>]>(out)) };
I have a patch that adds a `cast_slice_mut` method that could be used here, so I can fix it in that series. But let's not use `transmute` here either.
--- Cheers, Benno
self.read_raw(out) }
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
} /// Strip a prefix from `self`. Delegates to [`slice::strip_prefix`].@@ -290,7 +290,7 @@ pub const fn from_bytes_with_nul(bytes: &[u8]) -> Result<&Self, CStrConvertError #[inline] pub unsafe fn from_bytes_with_nul_unchecked_mut(bytes: &mut [u8]) -> &mut CStr { // SAFETY: Properties of `bytes` guaranteed by the safety precondition.
unsafe { &mut *(bytes as *mut [u8] as *mut CStr) }
unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut Self>(bytes)) }}
/// Returns a C pointer to the string.
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..c042b1fe499e 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -242,7 +242,7 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { pub fn read_slice(&mut self, out: &mut [u8]) -> Result { // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to // `out`.
let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) };
let out = unsafe { &mut *(core::mem::transmute::<*mut [u8], *mut [MaybeUninit<u8>]>(out)) };I have a patch that adds a `cast_slice_mut` method that could be used here, so I can fix it in that series. But let's not use `transmute` here either.
See above - I don't know what else I could write here.
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }
IMO a `transmute` is worse than an `as` cast :)
--- Cheers, Benno
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
On Wed Mar 26, 2025 at 12:54 AM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
It's right in the docs: "`transmute` should be the absolute last resort." [1]. IIRC, Gary was a bit more lenient in its use, but I think we should avoid it as much as possible such that people copying code or taking inspiration also don't use it.
So for both cases I'd prefer an `as` cast.
[1]: https://doc.rust-lang.org/std/mem/fn.transmute.html
--- Cheers, Benno
On Wed, Mar 26, 2025 at 6:31 AM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 12:54 AM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote:
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs index 40034f77fc2f..6233af50bab7 100644 --- a/rust/kernel/str.rs +++ b/rust/kernel/str.rs @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { #[inline] pub const fn from_bytes(bytes: &[u8]) -> &Self { // SAFETY: `BStr` is transparent to `[u8]`.
unsafe { &*(bytes as *const [u8] as *const BStr) }
unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
It's right in the docs: "`transmute` should be the absolute last resort." [1]. IIRC, Gary was a bit more lenient in its use, but I think we should avoid it as much as possible such that people copying code or taking inspiration also don't use it.
So for both cases I'd prefer an `as` cast.
I don't follow the logic. The trouble with `as` casts is that they are very lenient in what they allow, and to do these conversions with `as` casts requires ref -> pointer -> pointer -> pointer deref versus a single transmute. The safety comment perfectly describes why it's OK to do: the types are transparent. So why is `as` casting pointers better? It's just as unchecked as transmuting, and worse, it requires a raw pointer dereference.
On Wed Mar 26, 2025 at 11:35 AM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 6:31 AM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 12:54 AM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote: > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > index 40034f77fc2f..6233af50bab7 100644 > --- a/rust/kernel/str.rs > +++ b/rust/kernel/str.rs > @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { > #[inline] > pub const fn from_bytes(bytes: &[u8]) -> &Self { > // SAFETY: `BStr` is transparent to `[u8]`. > - unsafe { &*(bytes as *const [u8] as *const BStr) } > + unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) }
Hmm I'm not sure about using `transmute` here. Yes the types are transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
It's right in the docs: "`transmute` should be the absolute last resort." [1]. IIRC, Gary was a bit more lenient in its use, but I think we should avoid it as much as possible such that people copying code or taking inspiration also don't use it.
So for both cases I'd prefer an `as` cast.
I don't follow the logic. The trouble with `as` casts is that they are very lenient in what they allow, and to do these conversions with `as` casts requires ref -> pointer -> pointer -> pointer deref versus a single transmute. The safety comment perfectly describes why it's OK to do: the types are transparent. So why is `as` casting pointers better? It's just as unchecked as transmuting, and worse, it requires a raw pointer dereference.
Note that you're not transmuting `[u8]` to `BStr`, but `*const [u8]` to `*const BStr`. Those pointers have provenance and I'm not sure if transmuting them preserves it.
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
[1]: https://github.com/rust-lang/rust-clippy/issues/6372
--- Cheers, Benno
On Wed, Mar 26, 2025 at 12:43 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 11:35 AM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 6:31 AM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 12:54 AM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote: > On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote: > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs > > index 40034f77fc2f..6233af50bab7 100644 > > --- a/rust/kernel/str.rs > > +++ b/rust/kernel/str.rs > > @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { > > #[inline] > > pub const fn from_bytes(bytes: &[u8]) -> &Self { > > // SAFETY: `BStr` is transparent to `[u8]`. > > - unsafe { &*(bytes as *const [u8] as *const BStr) } > > + unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) } > > Hmm I'm not sure about using `transmute` here. Yes the types are > transparent, but I don't think that we should use it here.
What's your suggestion? I initially tried
let bytes: *const [u8] = bytes; unsafe { &*bytes.cast() }
but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
It's right in the docs: "`transmute` should be the absolute last resort." [1]. IIRC, Gary was a bit more lenient in its use, but I think we should avoid it as much as possible such that people copying code or taking inspiration also don't use it.
So for both cases I'd prefer an `as` cast.
I don't follow the logic. The trouble with `as` casts is that they are very lenient in what they allow, and to do these conversions with `as` casts requires ref -> pointer -> pointer -> pointer deref versus a single transmute. The safety comment perfectly describes why it's OK to do: the types are transparent. So why is `as` casting pointers better? It's just as unchecked as transmuting, and worse, it requires a raw pointer dereference.
Note that you're not transmuting `[u8]` to `BStr`, but `*const [u8]` to `*const BStr`. Those pointers have provenance and I'm not sure if transmuting them preserves it.
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 12:43 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 11:35 AM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 6:31 AM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 12:54 AM CET, Tamir Duberstein wrote:
On Tue, Mar 25, 2025 at 6:40 PM Benno Lossin benno.lossin@proton.me wrote:
On Tue Mar 25, 2025 at 11:33 PM CET, Tamir Duberstein wrote: > On Tue, Mar 25, 2025 at 6:11 PM Benno Lossin benno.lossin@proton.me wrote: >> On Tue Mar 25, 2025 at 9:07 PM CET, Tamir Duberstein wrote: >> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs >> > index 40034f77fc2f..6233af50bab7 100644 >> > --- a/rust/kernel/str.rs >> > +++ b/rust/kernel/str.rs >> > @@ -29,7 +29,7 @@ pub const fn is_empty(&self) -> bool { >> > #[inline] >> > pub const fn from_bytes(bytes: &[u8]) -> &Self { >> > // SAFETY: `BStr` is transparent to `[u8]`. >> > - unsafe { &*(bytes as *const [u8] as *const BStr) } >> > + unsafe { &*(core::mem::transmute::<*const [u8], *const Self>(bytes)) } >> >> Hmm I'm not sure about using `transmute` here. Yes the types are >> transparent, but I don't think that we should use it here. > > What's your suggestion? I initially tried > > let bytes: *const [u8] = bytes; > unsafe { &*bytes.cast() } > > but that doesn't compile because of the implicit Sized bound on pointer::cast.
This is AFAIK one of the only places where we cannot get rid of the `as` cast. So:
let bytes: *const [u8] = bytes; // CAST: `BStr` transparently wraps `[u8]`. let bytes = bytes as *const BStr; // SAFETY: `bytes` is derived from a reference. unsafe { &*bytes }IMO a `transmute` is worse than an `as` cast :)
Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`
Why is transmute worse than an `as` cast?
It's right in the docs: "`transmute` should be the absolute last resort." [1]. IIRC, Gary was a bit more lenient in its use, but I think we should avoid it as much as possible such that people copying code or taking inspiration also don't use it.
So for both cases I'd prefer an `as` cast.
I don't follow the logic. The trouble with `as` casts is that they are very lenient in what they allow, and to do these conversions with `as` casts requires ref -> pointer -> pointer -> pointer deref versus a single transmute. The safety comment perfectly describes why it's OK to do: the types are transparent. So why is `as` casting pointers better? It's just as unchecked as transmuting, and worse, it requires a raw pointer dereference.
Note that you're not transmuting `[u8]` to `BStr`, but `*const [u8]` to `*const BStr`. Those pointers have provenance and I'm not sure if transmuting them preserves it.
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
`CStr::from_bytes_with_nul_unchecked` does the transmute with references. That is a usage that the docs of `transmute` explicitly recommend to change to an `as` cast [1].
No idea about provenance still.
[1]: https://doc.rust-lang.org/std/mem/fn.transmute.html#alternatives
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
--- Cheers, Benno
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
`CStr::from_bytes_with_nul_unchecked` does the transmute with references. That is a usage that the docs of `transmute` explicitly recommend to change to an `as` cast [1].
RIght. That guidance was written in 2016 (https://github.com/rust-lang/rust/pull/34609) and doesn't present any rationale for `as` casts being preferred to transmute. I posted a comment in the most relevant issue I could find: https://github.com/rust-lang/rust/issues/34249#issuecomment-2755316610.
No idea about provenance still.
Well that's not surprising, nobody was thinking about provenance in 2016. But I really don't think we should blindly follow the advice in this case. It doesn't make an iota of sense to me - does it make sense to you?
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are: 1. transmute reference to reference. 2. coerce reference to pointer, `as` cast pointer to pointer (triggers `ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
On Wed, Mar 26, 2025 at 3:06 PM Tamir Duberstein tamird@gmail.com wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
`CStr::from_bytes_with_nul_unchecked` does the transmute with references. That is a usage that the docs of `transmute` explicitly recommend to change to an `as` cast [1].
RIght. That guidance was written in 2016 (https://github.com/rust-lang/rust/pull/34609) and doesn't present any rationale for `as` casts being preferred to transmute. I posted a comment in the most relevant issue I could find: https://github.com/rust-lang/rust/issues/34249#issuecomment-2755316610.
No idea about provenance still.
Well that's not surprising, nobody was thinking about provenance in 2016. But I really don't think we should blindly follow the advice in this case. It doesn't make an iota of sense to me - does it make sense to you?
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
Turns out there's a tortured past even in the standard library. In 2017 someone replaces trasmutes with pointer casts:
https://github.com/rust-lang/rust/commit/2633b85ab2c89822d2c227fc9e81c6ec1c0...
In 2020 someone changes it back to transmute:
https://github.com/rust-lang/rust/pull/75157/files
See also https://github.com/rust-lang/rust/pull/34609#issuecomment-230559871 which makes my point better than I have, particularly this snippet: "In addition, casting through raw pointers removes the check that both types have the same size that transmute does provide.".
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
`CStr::from_bytes_with_nul_unchecked` does the transmute with references. That is a usage that the docs of `transmute` explicitly recommend to change to an `as` cast [1].
RIght. That guidance was written in 2016 (https://github.com/rust-lang/rust/pull/34609) and doesn't present any rationale for `as` casts being preferred to transmute. I posted a comment in the most relevant issue I could find: https://github.com/rust-lang/rust/issues/34249#issuecomment-2755316610.
Not sure if that's the correct issue, maybe we should post one on the UCG (unsafe code guidelines). But before that we probably should ask on zulip...
No idea about provenance still.
Well that's not surprising, nobody was thinking about provenance in 2016. But I really don't think we should blindly follow the advice in this case. It doesn't make an iota of sense to me - does it make sense to you?
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
--- Cheers, Benno
On Wed, Mar 26, 2025 at 5:09 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
In the current code you're looking at, yes. But in the code I have locally I'm transmuting `[u8]` to `BStr`. See my earlier reply where I said "Hmm, looking at this again we can just transmute ref-to-ref and avoid pointers entirely. We're already doing that in `CStr::from_bytes_with_nul_unchecked`".
`CStr::from_bytes_with_nul_unchecked` does the transmute with references. That is a usage that the docs of `transmute` explicitly recommend to change to an `as` cast [1].
RIght. That guidance was written in 2016 (https://github.com/rust-lang/rust/pull/34609) and doesn't present any rationale for `as` casts being preferred to transmute. I posted a comment in the most relevant issue I could find: https://github.com/rust-lang/rust/issues/34249#issuecomment-2755316610.
Not sure if that's the correct issue, maybe we should post one on the UCG (unsafe code guidelines). But before that we probably should ask on zulip...
No idea about provenance still.
Well that's not surprising, nobody was thinking about provenance in 2016. But I really don't think we should blindly follow the advice in this case. It doesn't make an iota of sense to me - does it make sense to you?
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.
I tried to find some existing issues about the topic and found that there exists a clippy lint `transmute_ptr_to_ptr`. There is an issue asking for a better justification [1] and it seems like nobody provided one there. Maybe we should ask the opsem team what happens to provenance when transmuting?
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
Can you help me understand why you're confident about (2) but not (1)?
On Wed Mar 26, 2025 at 11:09 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 5:09 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
Can you help me understand why you're confident about (2) but not (1)?
My explanation from above explains why I'm not confident about (1):
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.
The reason I'm confident about (2) is that that is the canonical way to cast the type of a reference pointing to an `!Sized` value.
--- Cheers, Benno
On Wed, Mar 26, 2025 at 6:15 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 11:09 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 5:09 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote:
Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
Can you help me understand why you're confident about (2) but not (1)?
My explanation from above explains why I'm not confident about (1):
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.The reason I'm confident about (2) is that that is the canonical way to cast the type of a reference pointing to an `!Sized` value.
Do you have a citation, other than the transmute doc?
On Thu, Mar 27, 2025 at 10:15 AM Tamir Duberstein tamird@gmail.com wrote:
On Wed, Mar 26, 2025 at 6:15 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 11:09 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 5:09 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote: > > Yeah, we should do this - but again: not relevant in this discussion.
I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
Can you help me understand why you're confident about (2) but not (1)?
My explanation from above explains why I'm not confident about (1):
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.The reason I'm confident about (2) is that that is the canonical way to cast the type of a reference pointing to an `!Sized` value.
Do you have a citation, other than the transmute doc?
Turns out this appeases clippy:
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..7a6fc78fc314 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -240,9 +240,10 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error. pub fn read_slice(&mut self, out: &mut [u8]) -> Result { + let out: *mut [u8] = out; // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to // `out`. - let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; + let out = unsafe { &mut *(out as *mut [MaybeUninit<u8>]) }; self.read_raw(out) }
Benno, would that work for you? Same in str.rs, of course.
On Thu Mar 27, 2025 at 8:44 PM CET, Tamir Duberstein wrote:
On Thu, Mar 27, 2025 at 10:15 AM Tamir Duberstein tamird@gmail.com wrote:
On Wed, Mar 26, 2025 at 6:15 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 11:09 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 5:09 PM Benno Lossin benno.lossin@proton.me wrote:
On Wed Mar 26, 2025 at 8:06 PM CET, Tamir Duberstein wrote:
On Wed, Mar 26, 2025 at 1:36 PM Benno Lossin benno.lossin@proton.me wrote: > On Wed Mar 26, 2025 at 5:57 PM CET, Tamir Duberstein wrote: > > > > Yeah, we should do this - but again: not relevant in this discussion. > > I think it's pretty relevant.
It's not relevant because we're no longer talking about transmuting pointer to pointer. The two options are:
- transmute reference to reference.
- coerce reference to pointer, `as` cast pointer to pointer (triggers
`ptr_as_ptr`), reborrow pointer to reference.
If anyone can help me understand why (2) is better than (1), I'd certainly appreciate it.
I am very confident that (2) is correct. With (1) I'm not sure (see above), so that's why I mentioned it.
Can you help me understand why you're confident about (2) but not (1)?
My explanation from above explains why I'm not confident about (1):
For ptr-to-int transmutes, I know that they will probably remove provenance, hence I am a bit cautious about using them for ptr-to-ptr or ref-to-ref.The reason I'm confident about (2) is that that is the canonical way to cast the type of a reference pointing to an `!Sized` value.
Do you have a citation, other than the transmute doc?
Not that I am aware of anything.
Turns out this appeases clippy:
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 80a9782b1c6e..7a6fc78fc314 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -240,9 +240,10 @@ pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result { /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error. pub fn read_slice(&mut self, out: &mut [u8]) -> Result {
let out: *mut [u8] = out; // SAFETY: The types are compatible and `read_raw` doesn'twrite uninitialized bytes to // `out`.
let out = unsafe { &mut *(out as *mut [u8] as *mut[MaybeUninit<u8>]) };
let out = unsafe { &mut *(out as *mut [MaybeUninit<u8>]) }; self.read_raw(out)
Seems like your email client auto-wrapped that :(
Benno, would that work for you? Same in str.rs, of course.
For this specific case, I do have a `cast_slice_mut` function I mentioned in the other thread, but that is still stuck in the untrusted data series, I hope that it's ready tomorrow or maybe next week. I'd prefer if we use that (since its implementation also doesn't use `as` casts :). But if you can't wait, then the above is fine.
--- Cheers, Benno
On Tue, Mar 25, 2025 at 9:07 PM Tamir Duberstein tamird@gmail.com wrote:
Changes in v7:
- Add patch to enable `clippy::ref_as_ptr`.
- Link to v6: https://lore.kernel.org/r/20250324-ptr-as-ptr-v6-0-49d1b7fd4290@gmail.com
Please slow down -- at least wait a few days between revisions (unless there is a particular reason that requires it, of course).
We are in the merge window anyway, so there is no urgency to resend since these cannot go in, and you may want to rebase on top of -rc1 when it gets released so that you can cover most/all cases added by then.
Thanks!
Cheers, Miguel
On Tue, Mar 25, 2025 at 4:23 PM Miguel Ojeda miguel.ojeda.sandonis@gmail.com wrote:
On Tue, Mar 25, 2025 at 9:07 PM Tamir Duberstein tamird@gmail.com wrote:
Changes in v7:
- Add patch to enable `clippy::ref_as_ptr`.
- Link to v6: https://lore.kernel.org/r/20250324-ptr-as-ptr-v6-0-49d1b7fd4290@gmail.com
Please slow down -- at least wait a few days between revisions (unless there is a particular reason that requires it, of course).
Thanks, certainly no urgency here. In this particular case this isn't a true revision: the difference between v7 and v6 is the presence of an additional patch.
We are in the merge window anyway, so there is no urgency to resend since these cannot go in, and you may want to rebase on top of -rc1 when it gets released so that you can cover most/all cases added by then.
While it's true that this won't be picked up for some time (and that's ok), I wanted to get Benno's eyes on it sooner than later. Is there a workflow (within the mailing list) for such a case, or do folks go out of band in this situation?
Thanks! Tamir
linux-kselftest-mirror@lists.linaro.org
-
Benno Lossin -
Miguel Ojeda -
Tamir Duberstein