diff mbox series

[RFC,06/11] rust: sync: Replace static LockClassKey refs with a pointer wrapper

Message ID 20230714-classless_lockdep-v1-6-229b9671ce31@asahilina.net (mailing list archive)
State New, archived
Headers show
Series rust: Implicit lock class creation & Arc Lockdep integration | expand

Commit Message

Asahi Lina July 14, 2023, 9:13 a.m. UTC
We want to be able to handle dynamic lock class creation and using
pointers to things that aren't a real lock_class_key as lock classes.
Doing this by casting around Rust references is difficult without
accidentally invoking UB.

Instead, switch LockClassKey to being a raw pointer wrapper around a
lock_class_key, which means there is no UB possible on the Rust side
just by creating and consuming these objects. The C code also should
never actually dereference lock classes, only use their address
(possibly with an offset).

We still provide a dummy ZST version of this wrapper, to be used when
lockdep is disabled.

Signed-off-by: Asahi Lina <lina@asahilina.net>
---
 rust/kernel/sync.rs            |  6 +++---
 rust/kernel/sync/condvar.rs    |  2 +-
 rust/kernel/sync/lock.rs       |  4 ++--
 rust/kernel/sync/lockdep.rs    | 27 ++++++++++++++++++++++-----
 rust/kernel/sync/no_lockdep.rs | 15 +++++++++++++--
 rust/kernel/types.rs           |  2 +-
 6 files changed, 42 insertions(+), 14 deletions(-)

Comments

Martin Rodriguez Reboredo July 14, 2023, 3:10 p.m. UTC | #1
On 7/14/23 06:13, Asahi Lina wrote:
> We want to be able to handle dynamic lock class creation and using
> pointers to things that aren't a real lock_class_key as lock classes.
> Doing this by casting around Rust references is difficult without
> accidentally invoking UB.
> 
> Instead, switch LockClassKey to being a raw pointer wrapper around a
> lock_class_key, which means there is no UB possible on the Rust side
> just by creating and consuming these objects. The C code also should
> never actually dereference lock classes, only use their address
> (possibly with an offset).
> 
> We still provide a dummy ZST version of this wrapper, to be used when
> lockdep is disabled.
> 
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
> [...]
> diff --git a/rust/kernel/sync/lockdep.rs b/rust/kernel/sync/lockdep.rs
> index cb68b18dc0ad..d8328f4275fb 100644
> --- a/rust/kernel/sync/lockdep.rs
> +++ b/rust/kernel/sync/lockdep.rs
> @@ -9,19 +9,36 @@
>   
>   /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
>   #[repr(transparent)]
> -pub struct LockClassKey(Opaque<bindings::lock_class_key>);
> +pub struct StaticLockClassKey(Opaque<bindings::lock_class_key>);
>   
> -impl LockClassKey {
> +impl StaticLockClassKey {
>       /// Creates a new lock class key.
>       pub const fn new() -> Self {
>           Self(Opaque::uninit())
>       }
>   
> +    /// Returns the lock class key reference for this static lock class.
> +    pub const fn key(&self) -> LockClassKey {
> +        LockClassKey(self.0.get())

`Opaque::get` is not a `const fn` so this will not compile.

> +    }
> +}
> +
> +// SAFETY: `bindings::lock_class_key` just represents an opaque memory location, and is never
> +// actually dereferenced.
> +unsafe impl Sync for StaticLockClassKey {}
> +
> +/// A reference to a lock class key. This is a raw pointer to a lock_class_key,
> +/// which is required to have a static lifetime.
> +#[derive(Copy, Clone)]
> +pub struct LockClassKey(*mut bindings::lock_class_key);
> +
> +impl LockClassKey {
>       pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {

I think this can be made into a `const fn`. What do you all think?

> -        self.0.get()
> +        self.0
>       }
>   }
>   
> [...]
diff mbox series

Patch

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 352472c6b77a..49286c3e0ff3 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -20,7 +20,7 @@ 
 pub use arc::{Arc, ArcBorrow, UniqueArc};
 pub use condvar::CondVar;
 pub use lock::{mutex::Mutex, spinlock::SpinLock};
-pub use lockdep::LockClassKey;
+pub use lockdep::{LockClassKey, StaticLockClassKey};
 pub use locked_by::LockedBy;
 
 /// Defines a new static lock class and returns a pointer to it.
@@ -28,8 +28,8 @@ 
 #[macro_export]
 macro_rules! static_lock_class {
     () => {{
-        static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
-        &CLASS
+        static CLASS: $crate::sync::StaticLockClassKey = $crate::sync::StaticLockClassKey::new();
+        CLASS.key()
     }};
 }
 
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index ed353399c4e5..3bccb2c6ef84 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -92,7 +92,7 @@  unsafe impl Sync for CondVar {}
 impl CondVar {
     /// Constructs a new condvar initialiser.
     #[allow(clippy::new_ret_no_self)]
-    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+    pub fn new(name: &'static CStr, key: LockClassKey) -> impl PinInit<Self> {
         pin_init!(Self {
             _pin: PhantomPinned,
             // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have
diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index d493c5d19104..8e71e7aa2cc1 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -103,7 +103,7 @@  unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {}
 impl<T, B: Backend> Lock<T, B> {
     /// Constructs a new lock initialiser.
     #[allow(clippy::new_ret_no_self)]
-    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+    pub fn new(t: T, name: &'static CStr, key: LockClassKey) -> impl PinInit<Self> {
         pin_init!(Self {
             data: UnsafeCell::new(t),
             _pin: PhantomPinned,
@@ -119,7 +119,7 @@  pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinIni
     pub fn pin_init<E>(
         t: impl PinInit<T, E>,
         name: &'static CStr,
-        key: &'static LockClassKey,
+        key: LockClassKey,
     ) -> impl PinInit<Self, E>
     where
         E: core::convert::From<core::convert::Infallible>,
diff --git a/rust/kernel/sync/lockdep.rs b/rust/kernel/sync/lockdep.rs
index cb68b18dc0ad..d8328f4275fb 100644
--- a/rust/kernel/sync/lockdep.rs
+++ b/rust/kernel/sync/lockdep.rs
@@ -9,19 +9,36 @@ 
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
-pub struct LockClassKey(Opaque<bindings::lock_class_key>);
+pub struct StaticLockClassKey(Opaque<bindings::lock_class_key>);
 
-impl LockClassKey {
+impl StaticLockClassKey {
     /// Creates a new lock class key.
     pub const fn new() -> Self {
         Self(Opaque::uninit())
     }
 
+    /// Returns the lock class key reference for this static lock class.
+    pub const fn key(&self) -> LockClassKey {
+        LockClassKey(self.0.get())
+    }
+}
+
+// SAFETY: `bindings::lock_class_key` just represents an opaque memory location, and is never
+// actually dereferenced.
+unsafe impl Sync for StaticLockClassKey {}
+
+/// A reference to a lock class key. This is a raw pointer to a lock_class_key,
+/// which is required to have a static lifetime.
+#[derive(Copy, Clone)]
+pub struct LockClassKey(*mut bindings::lock_class_key);
+
+impl LockClassKey {
     pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
-        self.0.get()
+        self.0
     }
 }
 
-// SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
-// provides its own synchronization.
+// SAFETY: `bindings::lock_class_key` just represents an opaque memory location, and is never
+// actually dereferenced.
+unsafe impl Send for LockClassKey {}
 unsafe impl Sync for LockClassKey {}
diff --git a/rust/kernel/sync/no_lockdep.rs b/rust/kernel/sync/no_lockdep.rs
index 69d42e8d9801..518ec0bf9a7d 100644
--- a/rust/kernel/sync/no_lockdep.rs
+++ b/rust/kernel/sync/no_lockdep.rs
@@ -5,14 +5,25 @@ 
 //! Takes the place of the `lockdep` module when lockdep is disabled.
 
 /// A dummy, zero-sized lock class.
-pub struct LockClassKey();
+pub struct StaticLockClassKey();
 
-impl LockClassKey {
+impl StaticLockClassKey {
     /// Creates a new dummy lock class key.
     pub const fn new() -> Self {
         Self()
     }
 
+    /// Returns the lock class key reference for this static lock class.
+    pub const fn key(&self) -> LockClassKey {
+        LockClassKey()
+    }
+}
+
+/// A dummy reference to a lock class key.
+#[derive(Copy, Clone)]
+pub struct LockClassKey();
+
+impl LockClassKey {
     pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
         core::ptr::null_mut()
     }
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 185d3493857e..91739bf71cc3 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -262,7 +262,7 @@  pub fn ffi_init(init_func: impl FnOnce(*mut T)) -> impl PinInit<Self> {
     }
 
     /// Returns a raw pointer to the opaque data.
-    pub fn get(&self) -> *mut T {
+    pub const fn get(&self) -> *mut T {
         UnsafeCell::raw_get(self.0.as_ptr())
     }