[v4,1/9] rust: types: add `NotThreadSafe`
Commit Message
This introduces a new marker type for types that shouldn't be thread
safe. By adding a field of this type to a struct, it becomes non-Send
and non-Sync, which means that it cannot be accessed in any way from
threads other than the one it was created on.
This is useful for APIs that require globals such as `current` to remain
constant while the value exists.
We update two existing users in the Kernel to use this helper:
* `Task::current()` - moving the return type of this value to a
different thread would not be safe as you can no longer be guaranteed
that the `current` pointer remains valid.
* Lock guards. Mutexes and spinlocks should be unlocked on the same
thread as where they were locked, so we enforce this using the Send
trait.
There are also additional users in later patches of this patchset. See
[1] and [2] for the discussion that led to the introducion of this
patch.
Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [1]
Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [2]
Suggested-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/sync/lock.rs | 15 +++++++++++----
rust/kernel/task.rs | 10 ++++++----
rust/kernel/types.rs | 17 +++++++++++++++++
3 files changed, 34 insertions(+), 8 deletions(-)
Comments
On Fri, Feb 2, 2024 at 5:56 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> This introduces a new marker type for types that shouldn't be thread
> safe. By adding a field of this type to a struct, it becomes non-Send
> and non-Sync, which means that it cannot be accessed in any way from
> threads other than the one it was created on.
>
> This is useful for APIs that require globals such as `current` to remain
> constant while the value exists.
>
> We update two existing users in the Kernel to use this helper:
>
> * `Task::current()` - moving the return type of this value to a
> different thread would not be safe as you can no longer be guaranteed
> that the `current` pointer remains valid.
> * Lock guards. Mutexes and spinlocks should be unlocked on the same
> thread as where they were locked, so we enforce this using the Send
> trait.
>
> There are also additional users in later patches of this patchset. See
> [1] and [2] for the discussion that led to the introducion of this
s/introducion/introduction
> patch.
>
> Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [1]
> Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [2]
> Suggested-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/sync/lock.rs | 15 +++++++++++----
> rust/kernel/task.rs | 10 ++++++----
> rust/kernel/types.rs | 17 +++++++++++++++++
> 3 files changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 149a5259d431..d08d9d32915a 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -6,8 +6,15 @@
> //! spinlocks, raw spinlocks) to be provided with minimal effort.
>
> use super::LockClassKey;
> -use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
> -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
> +use crate::{
> + bindings,
> + init::PinInit,
> + pin_init,
> + str::CStr,
> + types::ScopeGuard,
> + types::{NotThreadSafe, Opaque},
Formatting nit: ScopeGuard could probably be placed in the same group
as NotThreadSafe & Opaque
> [...]
> +
> +/// Zero-sized type to mark types not [`Send`].
> +///
> +/// Add this type as a field to your struct if your type should not be sent to a different task.
> +/// Since [`Send`] is an auto trait, adding a single field that is `!Send` will ensure that the
> +/// whole type is `!Send`.
> +///
> +/// If a type is `!Send` it is impossible to give control over an instance of the type to another
> +/// task. This is useful when a type stores task-local information for example file descriptors.
I initially read this thinking it meant to include this type if your
struct also had a FD rather than being part of the FD. Maybe
This is useful to include in types that store or reference task-local
information. A file descriptor is an example of one such type.
> +pub type NotThreadSafe = PhantomData<*mut ()>;
> +
> +/// Used to construct instances of type [`NotThreadSafe`] similar to how we construct
> +/// `PhantomData`.
I think it sounds slightly better reworded from personal to passive, i.e.
... similar to how `PhantomData` is constructed.
> +/// [`NotThreadSafe`]: type@NotThreadSafe
> +#[allow(non_upper_case_globals)]
> +pub const NotThreadSafe: NotThreadSafe = PhantomData;
> --
> 2.43.0.594.gd9cf4e227d-goog
This looks good, sounds nice to make the intent more clear. Nothing
that isn't optional, so
Reviewed-by: Trevor Gross <tmgross@umich.edu>
On 2/2/24 07:55, Alice Ryhl wrote:
> This introduces a new marker type for types that shouldn't be thread
> safe. By adding a field of this type to a struct, it becomes non-Send
> and non-Sync, which means that it cannot be accessed in any way from
> threads other than the one it was created on.
>
> This is useful for APIs that require globals such as `current` to remain
> constant while the value exists.
>
> We update two existing users in the Kernel to use this helper:
>
> * `Task::current()` - moving the return type of this value to a
> different thread would not be safe as you can no longer be guaranteed
> that the `current` pointer remains valid.
> * Lock guards. Mutexes and spinlocks should be unlocked on the same
> thread as where they were locked, so we enforce this using the Send
> trait.
>
> There are also additional users in later patches of this patchset. See
> [1] and [2] for the discussion that led to the introducion of this
> patch.
>
> Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [1]
> Link: https://lore.kernel.org/all/nFDPJFnzE9Q5cqY7FwSMByRH2OAn_BpI4H53NQfWIlN6I2qfmAqnkp2wRqn0XjMO65OyZY4h6P4K2nAGKJpAOSzksYXaiAK_FoH_8QbgBI4=@proton.me/ [2]
> Suggested-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> [...]
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
@@ -6,8 +6,15 @@
//! spinlocks, raw spinlocks) to be provided with minimal effort.
use super::LockClassKey;
-use crate::{bindings, init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard};
-use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned};
+use crate::{
+ bindings,
+ init::PinInit,
+ pin_init,
+ str::CStr,
+ types::ScopeGuard,
+ types::{NotThreadSafe, Opaque},
+};
+use core::{cell::UnsafeCell, marker::PhantomPinned};
use macros::pin_data;
pub mod mutex;
@@ -132,7 +139,7 @@ pub fn lock(&self) -> Guard<'_, T, B> {
pub struct Guard<'a, T: ?Sized, B: Backend> {
pub(crate) lock: &'a Lock<T, B>,
pub(crate) state: B::GuardState,
- _not_send: PhantomData<*mut ()>,
+ _not_send: NotThreadSafe,
}
// SAFETY: `Guard` is sync when the data protected by the lock is also sync.
@@ -184,7 +191,7 @@ pub(crate) unsafe fn new(lock: &'a Lock<T, B>, state: B::GuardState) -> Self {
Self {
lock,
state,
- _not_send: PhantomData,
+ _not_send: NotThreadSafe,
}
}
}
@@ -4,10 +4,12 @@
//!
//! C header: [`include/linux/sched.h`](srctree/include/linux/sched.h).
-use crate::{bindings, types::Opaque};
+use crate::{
+ bindings,
+ types::{NotThreadSafe, Opaque},
+};
use core::{
ffi::{c_int, c_long, c_uint},
- marker::PhantomData,
ops::Deref,
ptr,
};
@@ -106,7 +108,7 @@ impl Task {
pub unsafe fn current() -> impl Deref<Target = Task> {
struct TaskRef<'a> {
task: &'a Task,
- _not_send: PhantomData<*mut ()>,
+ _not_send: NotThreadSafe,
}
impl Deref for TaskRef<'_> {
@@ -125,7 +127,7 @@ fn deref(&self) -> &Self::Target {
// that `TaskRef` is not `Send`, we know it cannot be transferred to another thread
// (where it could potentially outlive the caller).
task: unsafe { &*ptr.cast() },
- _not_send: PhantomData,
+ _not_send: NotThreadSafe,
}
}
@@ -387,3 +387,20 @@ pub enum Either<L, R> {
/// Constructs an instance of [`Either`] containing a value of type `R`.
Right(R),
}
+
+/// Zero-sized type to mark types not [`Send`].
+///
+/// Add this type as a field to your struct if your type should not be sent to a different task.
+/// Since [`Send`] is an auto trait, adding a single field that is `!Send` will ensure that the
+/// whole type is `!Send`.
+///
+/// If a type is `!Send` it is impossible to give control over an instance of the type to another
+/// task. This is useful when a type stores task-local information for example file descriptors.
+pub type NotThreadSafe = PhantomData<*mut ()>;
+
+/// Used to construct instances of type [`NotThreadSafe`] similar to how we construct
+/// `PhantomData`.
+///
+/// [`NotThreadSafe`]: type@NotThreadSafe
+#[allow(non_upper_case_globals)]
+pub const NotThreadSafe: NotThreadSafe = PhantomData;