From 2dde1c8b04a5c415912bc3ffa8b677eb364dbcb7 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Thu, 7 Nov 2024 05:36:46 -0500 Subject: rust: sync: document `PhantomData` in `Arc` Add a comment explaining the relevant semantics of `PhantomData`. This should help future readers who may, as I did, assume that this field is redundant at first glance. Signed-off-by: Tamir Duberstein Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20241107-simplify-arc-v2-1-7256e638aac1@gmail.com Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index fa4509406ee9..9f0b04400e8e 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -127,6 +127,14 @@ mod std_vendor; /// ``` pub struct Arc { ptr: NonNull>, + // NB: this informs dropck that objects of type `ArcInner` may be used in ` as + // Drop>::drop`. Note that dropck already assumes that objects of type `T` may be used in + // ` as Drop>::drop` and the distinction between `T` and `ArcInner` is not presently + // meaningful with respect to dropck - but this may change in the future so this is left here + // out of an abundance of caution. + // + // See https://doc.rust-lang.org/nomicon/phantom-data.html#generic-parameters-and-drop-checking + // for more detail on the semantics of dropck in the presence of `PhantomData`. _p: PhantomData>, } -- cgit v1.2.3 From 47cb6bf7860ce33bdd000198f8b65cf9fb3324b4 Mon Sep 17 00:00:00 2001 From: Xiangfei Ding Date: Wed, 4 Dec 2024 04:47:49 +0800 Subject: rust: use derive(CoercePointee) on rustc >= 1.84.0 The `kernel` crate relies on both `coerce_unsized` and `dispatch_from_dyn` unstable features. Alice Ryhl has proposed [1] the introduction of the unstable macro `SmartPointer` to reduce such dependence, along with a RFC patch [2]. Since Rust 1.81.0 this macro, later renamed to `CoercePointee` in Rust 1.84.0 [3], has been fully implemented with the naming discussion resolved. This feature is now on track to stabilization in the language. In order to do so, we shall start using this macro in the `kernel` crate to prove the functionality and utility of the macro as the justification of its stabilization. This patch makes this switch in such a way that the crate remains backward compatible with older Rust compiler versions, via the new Kconfig option `RUSTC_HAS_COERCE_POINTEE`. A minimal demonstration example is added to the `samples/rust/rust_print_main.rs` module. Link: https://rust-lang.github.io/rfcs/3621-derive-smart-pointer.html [1] Link: https://lore.kernel.org/all/20240823-derive-smart-pointer-v1-1-53769cd37239@google.com/ [2] Link: https://github.com/rust-lang/rust/pull/131284 [3] Signed-off-by: Xiangfei Ding Reviewed-by: Fiona Behrens Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20241203205050.679106-2-dingxiangfei2009@gmail.com [ Fixed version to 1.84. Renamed option to `RUSTC_HAS_COERCE_POINTEE` to match `CC_HAS_*` ones. Moved up new config option, closer to the `CC_HAS_*` ones. Simplified Kconfig line. Fixed typos and slightly reworded example and commit. Added Link to PR. - Miguel ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 9f0b04400e8e..25d2185df4cb 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -26,7 +26,7 @@ use crate::{ use core::{ alloc::Layout, fmt, - marker::{PhantomData, Unsize}, + marker::PhantomData, mem::{ManuallyDrop, MaybeUninit}, ops::{Deref, DerefMut}, pin::Pin, @@ -125,6 +125,8 @@ mod std_vendor; /// let coerced: Arc = obj; /// # Ok::<(), Error>(()) /// ``` +#[repr(transparent)] +#[cfg_attr(CONFIG_RUSTC_HAS_COERCE_POINTEE, derive(core::marker::CoercePointee))] pub struct Arc { ptr: NonNull>, // NB: this informs dropck that objects of type `ArcInner` may be used in ` as @@ -180,10 +182,12 @@ impl ArcInner { // This is to allow coercion from `Arc` to `Arc` if `T` can be converted to the // dynamically-sized type (DST) `U`. -impl, U: ?Sized> core::ops::CoerceUnsized> for Arc {} +#[cfg(not(CONFIG_RUSTC_HAS_COERCE_POINTEE))] +impl, U: ?Sized> core::ops::CoerceUnsized> for Arc {} // This is to allow `Arc` to be dispatched on when `Arc` can be coerced into `Arc`. -impl, U: ?Sized> core::ops::DispatchFromDyn> for Arc {} +#[cfg(not(CONFIG_RUSTC_HAS_COERCE_POINTEE))] +impl, U: ?Sized> core::ops::DispatchFromDyn> for Arc {} // SAFETY: It is safe to send `Arc` to another thread when the underlying `T` is `Sync` because // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs @@ -479,6 +483,8 @@ impl From>> for Arc { /// obj.as_arc_borrow().use_reference(); /// # Ok::<(), Error>(()) /// ``` +#[repr(transparent)] +#[cfg_attr(CONFIG_RUSTC_HAS_COERCE_POINTEE, derive(core::marker::CoercePointee))] pub struct ArcBorrow<'a, T: ?Sized + 'a> { inner: NonNull>, _p: PhantomData<&'a ()>, @@ -486,7 +492,8 @@ pub struct ArcBorrow<'a, T: ?Sized + 'a> { // This is to allow `ArcBorrow` to be dispatched on when `ArcBorrow` can be coerced into // `ArcBorrow`. -impl, U: ?Sized> core::ops::DispatchFromDyn> +#[cfg(not(CONFIG_RUSTC_HAS_COERCE_POINTEE))] +impl, U: ?Sized> core::ops::DispatchFromDyn> for ArcBorrow<'_, T> { } -- cgit v1.2.3 From c6340da3d254ee491fc113d4dc5566bea7bebdf3 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 20 Nov 2024 06:46:00 -0500 Subject: rust: arc: use `NonNull::new_unchecked` There is no need to check (and panic on violations of) the safety requirements on `ForeignOwnable` functions. Avoiding the check is consistent with the implementation of `ForeignOwnable` for `Box`. Reviewed-by: Alice Ryhl Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-1-80dbadd00951@gmail.com Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 25d2185df4cb..2f3156b6aacf 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -349,9 +349,9 @@ impl ForeignOwnable for Arc { } unsafe fn borrow<'a>(ptr: *const crate::ffi::c_void) -> ArcBorrow<'a, T> { - // By the safety requirement of this function, we know that `ptr` came from - // a previous call to `Arc::into_foreign`. - let inner = NonNull::new(ptr as *mut ArcInner).unwrap(); + // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous + // call to `Self::into_foreign`. + let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner) }; // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive // for the lifetime of the returned value. @@ -359,10 +359,14 @@ impl ForeignOwnable for Arc { } unsafe fn from_foreign(ptr: *const crate::ffi::c_void) -> Self { + // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous + // call to `Self::into_foreign`. + let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner) }; + // SAFETY: By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and // holds a reference count increment that is transferrable to us. - unsafe { Self::from_inner(NonNull::new(ptr as _).unwrap()) } + unsafe { Self::from_inner(inner) } } } -- cgit v1.2.3 From aa991a2a819535a0014e1159b455b64e3db87510 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 20 Nov 2024 06:46:01 -0500 Subject: rust: types: avoid `as` casts Replace `as` casts with `cast{,_mut}` calls which are a bit safer. In one instance, remove an unnecessary `as` cast without replacement. Reviewed-by: Alice Ryhl Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-2-80dbadd00951@gmail.com Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 2f3156b6aacf..378925e553ec 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -213,10 +213,11 @@ impl Arc { }; let inner = KBox::new(value, flags)?; + let inner = KBox::leak(inner).into(); // SAFETY: We just created `inner` with a reference count of 1, which is owned by the new // `Arc` object. - Ok(unsafe { Self::from_inner(KBox::leak(inner).into()) }) + Ok(unsafe { Self::from_inner(inner) }) } } @@ -345,13 +346,13 @@ impl ForeignOwnable for Arc { type Borrowed<'a> = ArcBorrow<'a, T>; fn into_foreign(self) -> *const crate::ffi::c_void { - ManuallyDrop::new(self).ptr.as_ptr() as _ + ManuallyDrop::new(self).ptr.as_ptr().cast() } unsafe fn borrow<'a>(ptr: *const crate::ffi::c_void) -> ArcBorrow<'a, T> { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. - let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner) }; + let inner = unsafe { NonNull::new_unchecked(ptr.cast_mut().cast::>()) }; // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive // for the lifetime of the returned value. @@ -361,7 +362,7 @@ impl ForeignOwnable for Arc { unsafe fn from_foreign(ptr: *const crate::ffi::c_void) -> Self { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. - let inner = unsafe { NonNull::new_unchecked(ptr as *mut ArcInner) }; + let inner = unsafe { NonNull::new_unchecked(ptr.cast_mut().cast::>()) }; // SAFETY: By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and -- cgit v1.2.3 From 5d385a356f628bd4a1d9f403588272d9626e3245 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 20 Nov 2024 06:46:02 -0500 Subject: rust: arc: split unsafe block, add missing comment The new SAFETY comment style is taken from existing comments in `deref` and `drop. Reviewed-by: Alice Ryhl Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-3-80dbadd00951@gmail.com Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 378925e553ec..93eac650146a 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -389,10 +389,14 @@ impl AsRef for Arc { impl Clone for Arc { fn clone(&self) -> Self { + // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is + // safe to dereference it. + let refcount = unsafe { self.ptr.as_ref() }.refcount.get(); + // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero. // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is // safe to increment the refcount. - unsafe { bindings::refcount_inc(self.ptr.as_ref().refcount.get()) }; + unsafe { bindings::refcount_inc(refcount) }; // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`. unsafe { Self::from_inner(self.ptr) } -- cgit v1.2.3 From 14686571a914833e38eef0f907202f58df4ffcd2 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 20 Nov 2024 06:46:03 -0500 Subject: rust: kernel: change `ForeignOwnable` pointer to mut It is slightly more convenient to operate on mut pointers, and this also properly conveys the desired ownership semantics of the trait. Reviewed-by: Alice Ryhl Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-4-80dbadd00951@gmail.com [ Reworded title slightly. - Miguel ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index 93eac650146a..fe334394c328 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -345,24 +345,24 @@ impl Arc { impl ForeignOwnable for Arc { type Borrowed<'a> = ArcBorrow<'a, T>; - fn into_foreign(self) -> *const crate::ffi::c_void { + fn into_foreign(self) -> *mut crate::ffi::c_void { ManuallyDrop::new(self).ptr.as_ptr().cast() } - unsafe fn borrow<'a>(ptr: *const crate::ffi::c_void) -> ArcBorrow<'a, T> { + unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. - let inner = unsafe { NonNull::new_unchecked(ptr.cast_mut().cast::>()) }; + let inner = unsafe { NonNull::new_unchecked(ptr.cast::>()) }; // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive // for the lifetime of the returned value. unsafe { ArcBorrow::new(inner) } } - unsafe fn from_foreign(ptr: *const crate::ffi::c_void) -> Self { + unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. - let inner = unsafe { NonNull::new_unchecked(ptr.cast_mut().cast::>()) }; + let inner = unsafe { NonNull::new_unchecked(ptr.cast::>()) }; // SAFETY: By the safety requirement of this function, we know that `ptr` came from // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and -- cgit v1.2.3 From c6b97538c2c0f4de5902b5bb78eb89bd34219df9 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 20 Nov 2024 06:46:04 -0500 Subject: rust: kernel: reorder `ForeignOwnable` items `{into,from}_foreign` before `borrow` is slightly more logical. This removes an inconsistency with `kbox.rs` which already uses this ordering. Reviewed-by: Alice Ryhl Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-5-80dbadd00951@gmail.com [ Reworded title slightly. - Miguel ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index fe334394c328..e351c701c166 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -349,25 +349,25 @@ impl ForeignOwnable for Arc { ManuallyDrop::new(self).ptr.as_ptr().cast() } - unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> { + unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. let inner = unsafe { NonNull::new_unchecked(ptr.cast::>()) }; - // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive - // for the lifetime of the returned value. - unsafe { ArcBorrow::new(inner) } + // SAFETY: By the safety requirement of this function, we know that `ptr` came from + // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and + // holds a reference count increment that is transferrable to us. + unsafe { Self::from_inner(inner) } } - unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self { + unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> { // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous // call to `Self::into_foreign`. let inner = unsafe { NonNull::new_unchecked(ptr.cast::>()) }; - // SAFETY: By the safety requirement of this function, we know that `ptr` came from - // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and - // holds a reference count increment that is transferrable to us. - unsafe { Self::from_inner(inner) } + // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive + // for the lifetime of the returned value. + unsafe { ArcBorrow::new(inner) } } } -- cgit v1.2.3 From c27e705cb2b71769dbb4bb1bee1920d2fa01a597 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Wed, 20 Nov 2024 06:46:05 -0500 Subject: rust: kernel: add improved version of `ForeignOwnable::borrow_mut` Previously, the `ForeignOwnable` trait had a method called `borrow_mut` that was intended to provide mutable access to the inner value. However, the method accidentally made it possible to change the address of the object being modified, which usually isn't what we want. (And when we want that, it can be done by calling `from_foreign` and `into_foreign`, like how the old `borrow_mut` was implemented.) In this patch, we introduce an alternate definition of `borrow_mut` that solves the previous problem. Conceptually, given a pointer type `P` that implements `ForeignOwnable`, the `borrow_mut` method gives you the same kind of access as an `&mut P` would, except that it does not let you change the pointer `P` itself. This is analogous to how the existing `borrow` method provides the same kind of access to the inner value as an `&P`. Note that for types like `Arc`, having an `&mut Arc` only gives you immutable access to the inner `T`. This is because mutable references assume exclusive access, but there might be other handles to the same reference counted value, so the access isn't exclusive. The `Arc` type implements this by making `borrow_mut` return the same type as `borrow`. Signed-off-by: Alice Ryhl Reviewed-by: Boqun Feng Reviewed-by: Benno Lossin Reviewed-by: Martin Rodriguez Reboredo Reviewed-by: Andreas Hindborg Signed-off-by: Tamir Duberstein Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20241120-borrow-mut-v6-6-80dbadd00951@gmail.com [ Updated to `crate::ffi::`. Reworded title slightly. - Miguel ] Signed-off-by: Miguel Ojeda --- rust/kernel/sync/arc.rs | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'rust/kernel/sync') diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index e351c701c166..3cefda7a4372 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -344,6 +344,7 @@ impl Arc { impl ForeignOwnable for Arc { type Borrowed<'a> = ArcBorrow<'a, T>; + type BorrowedMut<'a> = Self::Borrowed<'a>; fn into_foreign(self) -> *mut crate::ffi::c_void { ManuallyDrop::new(self).ptr.as_ptr().cast() @@ -369,6 +370,12 @@ impl ForeignOwnable for Arc { // for the lifetime of the returned value. unsafe { ArcBorrow::new(inner) } } + + unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> { + // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety + // requirements for `borrow`. + unsafe { Self::borrow(ptr) } + } } impl Deref for Arc { -- cgit v1.2.3