[PATCH v3 0/2] rust: lockdep: Fix soundness issue affecting LockClassKeys

[PATCH v3 0/2] rust: lockdep: Fix soundness issue affecting LockClassKeys

[Mitchell Levy]

This series is aimed at fixing a soundness issue with how dynamically
allocated LockClassKeys are handled. Currently, LockClassKeys can be
used without being Pin'd, which can break lockdep since it relies on
address stability. Similarly, these keys are not automatically
(de)registered with lockdep.

At the suggestion of Alice Ryhl, this series includes a patch for
-stable kernels that disables dynamically allocated keys. This prevents
backported patches from using the unsound implementation.

Currently, this series requires that all dynamically allocated
LockClassKeys have a lifetime of 'static (i.e., they must be leaked
after allocation). This is because Lock does not currently keep a
reference to the LockClassKey, instead passing it to C via FFI. This
causes a problem because the rust compiler would allow creating a
'static Lock with a 'a LockClassKey (with 'a < 'static) while C would
expect the LockClassKey to live as long as the lock. This problem
represents an avenue for future work.

---
Changes in v3:
- Rebased on rust-next
- Fixed clippy/compiler warninings (Thanks Boqun Feng)
- Link to v2: https://lore.kernel.org/r/20241219-rust-lockdep-v2-0-f65308fbc5ca@gmail.com

Changes in v2:
- Dropped formatting change that's already fixed upstream (Thanks Dirk
  Behme).
- Moved safety comment to the right point in the patch series (Thanks
  Dirk Behme and Boqun Feng).
- Added an example of dynamic LockClassKey usage (Thanks Boqun Feng).
- Link to v1: https://lore.kernel.org/r/20241004-rust-lockdep-v1-0-e9a5c45721fc@gmail.com

Changes from RFC:
- Split into two commits so that dynamically allocated LockClassKeys are
removed from stable kernels. (Thanks Alice Ryhl)
- Extract calls to C lockdep functions into helpers so things build
properly when LOCKDEP=n. (Thanks Benno Lossin)
- Remove extraneous `get_ref()` calls. (Thanks Benno Lossin)
- Provide better documentation for `new_dynamic()`. (Thanks Benno
Lossin)
- Ran rustfmt to fix formatting and some extraneous changes. (Thanks
Alice Ryhl and Benno Lossin)
- Link to RFC: https://lore.kernel.org/r/20240905-rust-lockdep-v1-1-d2c9c21aa8b2@gmail.com

---
Mitchell Levy (2):
      rust: lockdep: Remove support for dynamically allocated LockClassKeys
      rust: lockdep: Use Pin for all LockClassKey usages

 rust/helpers/helpers.c          |  1 +
 rust/helpers/sync.c             | 13 +++++++++
 rust/kernel/sync.rs             | 63 ++++++++++++++++++++++++++++++++++-------
 rust/kernel/sync/condvar.rs     |  5 ++--
 rust/kernel/sync/lock.rs        |  9 ++----
 rust/kernel/sync/lock/global.rs |  5 ++--
 rust/kernel/sync/poll.rs        |  2 +-
 rust/kernel/workqueue.rs        |  2 +-
 8 files changed, 77 insertions(+), 23 deletions(-)
---
base-commit: ceff0757f5dafb5be5205988171809c877b1d3e3
change-id: 20240905-rust-lockdep-d3e30521c8ba

Best regards,
-- 
Mitchell Levy <levymitchell0@gmail.com>

[PATCH v3 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys

[Mitchell Levy]

Currently, dynamically allocated LockCLassKeys can be used from the Rust
side without having them registered. This is a soundness issue, so
remove them.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-3-nmi@metaspace.dk/
Cc: stable@vger.kernel.org
Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
 rust/kernel/sync.rs | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 1eab7ebf25fd..cb92f2c323e5 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -29,28 +29,20 @@
 unsafe impl Sync for LockClassKey {}
 
 impl LockClassKey {
-    /// Creates a new lock class key.
-    pub const fn new() -> Self {
-        Self(Opaque::uninit())
-    }
-
     pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
         self.0.get()
     }
 }
 
-impl Default for LockClassKey {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 /// Defines a new static lock class and returns a pointer to it.
 #[doc(hidden)]
 #[macro_export]
 macro_rules! static_lock_class {
     () => {{
-        static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new();
+        static CLASS: $crate::sync::LockClassKey =
+            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
+            // lock_class_key
+            unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
         &CLASS
     }};
 }

-- 
2.34.1

Re: [PATCH v3 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys

[Miguel Ojeda]

On Wed, Feb 5, 2025 at 8:59 PM Mitchell Levy <levymitchell0@gmail.com> wrote:
>
> Currently, dynamically allocated LockCLassKeys can be used from the Rust
> side without having them registered. This is a soundness issue, so
> remove them.
>
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-3-nmi@metaspace.dk/
> Cc: stable@vger.kernel.org
> Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>

I imagine we should have:

    Fixes: 6ea5aa08857a ("rust: sync: introduce `LockClassKey`")

Is that right?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys

[Mitchell Levy]

On Fri, Feb 07, 2025 at 12:27:58AM +0100, Miguel Ojeda wrote:
> On Wed, Feb 5, 2025 at 8:59 PM Mitchell Levy <levymitchell0@gmail.com> wrote:
> >
> > Currently, dynamically allocated LockCLassKeys can be used from the Rust
> > side without having them registered. This is a soundness issue, so
> > remove them.
> >
> > Suggested-by: Alice Ryhl <aliceryhl@google.com>
> > Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-3-nmi@metaspace.dk/
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> 
> I imagine we should have:
> 
>     Fixes: 6ea5aa08857a ("rust: sync: introduce `LockClassKey`")
> 
> Is that right?

That's correct. Thank you for catching this! I will include this on a
resend.

Mitchell

> Thanks!
> 
> Cheers,
> Miguel

Re: [PATCH v3 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys

[Benno Lossin]

On 05.02.25 20:59, Mitchell Levy wrote:
> Currently, dynamically allocated LockCLassKeys can be used from the Rust
> side without having them registered. This is a soundness issue, so
> remove them.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-3-nmi@metaspace.dk/
> Cc: stable@vger.kernel.org
> Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
> ---
>  rust/kernel/sync.rs | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

[PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Mitchell Levy]

Reintroduce dynamically-allocated LockClassKeys such that they are
automatically (de)registered. Require that all usages of LockClassKeys
ensure that they are Pin'd.

Closes: https://github.com/Rust-for-Linux/linux/issues/1102
Suggested-by: Benno Lossin <benno.lossin@proton.me>
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Mitchell Levy <levymitchell0@gmail.com>
---
 rust/helpers/helpers.c          |  1 +
 rust/helpers/sync.c             | 13 ++++++++++
 rust/kernel/sync.rs             | 57 ++++++++++++++++++++++++++++++++++++++---
 rust/kernel/sync/condvar.rs     |  5 ++--
 rust/kernel/sync/lock.rs        |  9 +++----
 rust/kernel/sync/lock/global.rs |  5 ++--
 rust/kernel/sync/poll.rs        |  2 +-
 rust/kernel/workqueue.rs        |  2 +-
 8 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..572af343212c 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -25,6 +25,7 @@
 #include "signal.c"
 #include "slab.c"
 #include "spinlock.c"
+#include "sync.c"
 #include "task.c"
 #include "uaccess.c"
 #include "vmalloc.c"
diff --git a/rust/helpers/sync.c b/rust/helpers/sync.c
new file mode 100644
index 000000000000..ff7e68b48810
--- /dev/null
+++ b/rust/helpers/sync.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/lockdep.h>
+
+void rust_helper_lockdep_register_key(struct lock_class_key *k)
+{
+	lockdep_register_key(k);
+}
+
+void rust_helper_lockdep_unregister_key(struct lock_class_key *k)
+{
+	lockdep_unregister_key(k);
+}
diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index cb92f2c323e5..a02acde1f22e 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -5,6 +5,8 @@
 //! This module contains the kernel APIs related to synchronisation that have been ported or
 //! wrapped for usage by Rust code in the kernel.
 
+use crate::pin_init;
+use crate::prelude::*;
 use crate::types::Opaque;
 
 mod arc;
@@ -22,15 +24,64 @@
 
 /// Represents a lockdep class. It's a wrapper around C's `lock_class_key`.
 #[repr(transparent)]
-pub struct LockClassKey(Opaque<bindings::lock_class_key>);
+#[pin_data(PinnedDrop)]
+pub struct LockClassKey {
+    #[pin]
+    inner: Opaque<bindings::lock_class_key>,
+}
 
 // SAFETY: `bindings::lock_class_key` is designed to be used concurrently from multiple threads and
 // provides its own synchronization.
 unsafe impl Sync for LockClassKey {}
 
 impl LockClassKey {
+    /// Initializes a dynamically allocated lock class key. In the common case of using a
+    /// statically allocated lock class key, the static_lock_class! macro should be used instead.
+    ///
+    /// # Example
+    /// ```
+    /// # use kernel::{c_str, stack_pin_init};
+    /// # use kernel::alloc::KBox;
+    /// # use kernel::types::ForeignOwnable;
+    /// # use kernel::sync::{LockClassKey, SpinLock};
+    ///
+    /// let key = KBox::pin_init(LockClassKey::new_dynamic(), GFP_KERNEL)?;
+    /// let key_ptr = key.into_foreign();
+    ///
+    /// {
+    ///     stack_pin_init!(let num: SpinLock<u32> = SpinLock::new(
+    ///         0,
+    ///         c_str!("my_spinlock"),
+    ///         // SAFETY: `key_ptr` is returned by the above `into_foreign()`, whose
+    ///         // `from_foreign()` has not yet been called.
+    ///         unsafe { <Pin<KBox<LockClassKey>> as ForeignOwnable>::borrow(key_ptr) }
+    ///     ));
+    /// }
+    ///
+    /// // SAFETY: We dropped `num`, the only use of the key, so the result of the previous
+    /// // `borrow` has also been dropped. Thus, it's safe to use from_foreign.
+    /// unsafe { drop(<Pin<KBox<LockClassKey>> as ForeignOwnable>::from_foreign(key_ptr)) };
+    ///
+    /// # Ok::<(), Error>(())
+    /// ```
+    pub fn new_dynamic() -> impl PinInit<Self> {
+        pin_init!(Self {
+            // SAFETY: lockdep_register_key expects an uninitialized block of memory
+            inner <- Opaque::ffi_init(|slot| unsafe { bindings::lockdep_register_key(slot) })
+        })
+    }
+
     pub(crate) fn as_ptr(&self) -> *mut bindings::lock_class_key {
-        self.0.get()
+        self.inner.get()
+    }
+}
+
+#[pinned_drop]
+impl PinnedDrop for LockClassKey {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: self.as_ptr was registered with lockdep and self is pinned, so the address
+        // hasn't changed. Thus, it's safe to pass to unregister.
+        unsafe { bindings::lockdep_unregister_key(self.as_ptr()) }
     }
 }
 
@@ -43,7 +94,7 @@ macro_rules! static_lock_class {
             // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
             // lock_class_key
             unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
-        &CLASS
+        $crate::prelude::Pin::static_ref(&CLASS)
     }};
 }
 
diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs
index 7df565038d7d..29289ccf55cc 100644
--- a/rust/kernel/sync/condvar.rs
+++ b/rust/kernel/sync/condvar.rs
@@ -15,8 +15,7 @@
     time::Jiffies,
     types::Opaque,
 };
-use core::marker::PhantomPinned;
-use core::ptr;
+use core::{marker::PhantomPinned, pin::Pin, ptr};
 use macros::pin_data;
 
 /// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class.
@@ -101,7 +100,7 @@ unsafe impl Sync for CondVar {}
 
 impl CondVar {
     /// Constructs a new condvar initialiser.
-    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+    pub fn new(name: &'static CStr, key: Pin<&'static 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 41dcddac69e2..119e5f569bdb 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -7,12 +7,9 @@
 
 use super::LockClassKey;
 use crate::{
-    init::PinInit,
-    pin_init,
-    str::CStr,
-    types::{NotThreadSafe, Opaque, ScopeGuard},
+    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
 };
-use core::{cell::UnsafeCell, marker::PhantomPinned};
+use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
 use macros::pin_data;
 
 pub mod mutex;
@@ -121,7 +118,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.
-    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
         pin_init!(Self {
             data: UnsafeCell::new(t),
             _pin: PhantomPinned,
diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
index 480ee724e3cc..d65f94b5caf2 100644
--- a/rust/kernel/sync/lock/global.rs
+++ b/rust/kernel/sync/lock/global.rs
@@ -13,6 +13,7 @@
 use core::{
     cell::UnsafeCell,
     marker::{PhantomData, PhantomPinned},
+    pin::Pin,
 };
 
 /// Trait implemented for marker types for global locks.
@@ -26,7 +27,7 @@ pub trait GlobalLockBackend {
     /// The backend used for this global lock.
     type Backend: Backend + 'static;
     /// The class for this global lock.
-    fn get_lock_class() -> &'static LockClassKey;
+    fn get_lock_class() -> Pin<&'static LockClassKey>;
 }
 
 /// Type used for global locks.
@@ -270,7 +271,7 @@ impl $crate::sync::lock::GlobalLockBackend for $name {
             type Item = $valuety;
             type Backend = $crate::global_lock_inner!(backend $kind);
 
-            fn get_lock_class() -> &'static $crate::sync::LockClassKey {
+            fn get_lock_class() -> Pin<&'static $crate::sync::LockClassKey> {
                 $crate::static_lock_class!()
             }
         }
diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs
index d5f17153b424..c4934f82d68b 100644
--- a/rust/kernel/sync/poll.rs
+++ b/rust/kernel/sync/poll.rs
@@ -89,7 +89,7 @@ pub struct PollCondVar {
 
 impl PollCondVar {
     /// Constructs a new condvar initialiser.
-    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
+    pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
         pin_init!(Self {
             inner <- CondVar::new(name, key),
         })
diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs
index 1dcd53478edd..88154f55b329 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -369,7 +369,7 @@ unsafe impl<T: ?Sized, const ID: u64> Sync for Work<T, ID> {}
 impl<T: ?Sized, const ID: u64> Work<T, ID> {
     /// Creates a new instance of [`Work`].
     #[inline]
-    pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self>
+    pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self>
     where
         T: WorkItem<ID>,
     {

-- 
2.34.1

Re: [PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Benno Lossin]

On 05.02.25 20:59, Mitchell Levy wrote:
> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> index 41dcddac69e2..119e5f569bdb 100644
> --- a/rust/kernel/sync/lock.rs
> +++ b/rust/kernel/sync/lock.rs
> @@ -7,12 +7,9 @@
> 
>  use super::LockClassKey;
>  use crate::{
> -    init::PinInit,
> -    pin_init,
> -    str::CStr,
> -    types::{NotThreadSafe, Opaque, ScopeGuard},
> +    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
>  };
> -use core::{cell::UnsafeCell, marker::PhantomPinned};
> +use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
>  use macros::pin_data;
> 
>  pub mod mutex;
> @@ -121,7 +118,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.
> -    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> +    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {

Static references do not need `Pin`, since `Pin::static_ref` [1] exists,
so you can just as well not add the `Pin` here and the other places
where you have `Pin<&'static T>`.

The reasoning is that since the data lives for `'static` at that
location, it will never move (since it is borrowed for `'static` after
all).

[1]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.static_ref

---
Cheers,
Benno

>          pin_init!(Self {
>              data: UnsafeCell::new(t),
>              _pin: PhantomPinned,
> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> index 480ee724e3cc..d65f94b5caf2 100644
> --- a/rust/kernel/sync/lock/global.rs
> +++ b/rust/kernel/sync/lock/global.rs
> @@ -13,6 +13,7 @@
>  use core::{
>      cell::UnsafeCell,
>      marker::{PhantomData, PhantomPinned},
> +    pin::Pin,
>  };
> 
>  /// Trait implemented for marker types for global locks.

Re: [PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Boqun Feng]

On Wed, Feb 05, 2025 at 08:30:50PM +0000, Benno Lossin wrote:
> On 05.02.25 20:59, Mitchell Levy wrote:
> > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> > index 41dcddac69e2..119e5f569bdb 100644
> > --- a/rust/kernel/sync/lock.rs
> > +++ b/rust/kernel/sync/lock.rs
> > @@ -7,12 +7,9 @@
> > 
> >  use super::LockClassKey;
> >  use crate::{
> > -    init::PinInit,
> > -    pin_init,
> > -    str::CStr,
> > -    types::{NotThreadSafe, Opaque, ScopeGuard},
> > +    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
> >  };
> > -use core::{cell::UnsafeCell, marker::PhantomPinned};
> > +use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
> >  use macros::pin_data;
> > 
> >  pub mod mutex;
> > @@ -121,7 +118,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.
> > -    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> > +    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
> 
> Static references do not need `Pin`, since `Pin::static_ref` [1] exists,
> so you can just as well not add the `Pin` here and the other places
> where you have `Pin<&'static T>`.
> 

You're right about `Pin` not needed for 'static. However, the
`Pin<&'static LockClassKey>` signature is the intermediate state, and
eventually we will need to support initializing a lock (and others) with
a dynamically allocated `LockClassKey`, and when that is available, the
type of `key` will become `Pin<&'a LockClassKey>`, so `Pin` is needed.

So I would like to keep this patch as it is. Works for you?

Regards,
Boqun

> The reasoning is that since the data lives for `'static` at that
> location, it will never move (since it is borrowed for `'static` after
> all).
> 
> [1]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.static_ref
> 
> ---
> Cheers,
> Benno
> 
> >          pin_init!(Self {
> >              data: UnsafeCell::new(t),
> >              _pin: PhantomPinned,
> > diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> > index 480ee724e3cc..d65f94b5caf2 100644
> > --- a/rust/kernel/sync/lock/global.rs
> > +++ b/rust/kernel/sync/lock/global.rs
> > @@ -13,6 +13,7 @@
> >  use core::{
> >      cell::UnsafeCell,
> >      marker::{PhantomData, PhantomPinned},
> > +    pin::Pin,
> >  };
> > 
> >  /// Trait implemented for marker types for global locks.
> 
> 

Re: [PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Benno Lossin]

On 06.02.25 22:27, Boqun Feng wrote:
> On Wed, Feb 05, 2025 at 08:30:50PM +0000, Benno Lossin wrote:
>> On 05.02.25 20:59, Mitchell Levy wrote:
>>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
>>> index 41dcddac69e2..119e5f569bdb 100644
>>> --- a/rust/kernel/sync/lock.rs
>>> +++ b/rust/kernel/sync/lock.rs
>>> @@ -7,12 +7,9 @@
>>>
>>>  use super::LockClassKey;
>>>  use crate::{
>>> -    init::PinInit,
>>> -    pin_init,
>>> -    str::CStr,
>>> -    types::{NotThreadSafe, Opaque, ScopeGuard},
>>> +    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
>>>  };
>>> -use core::{cell::UnsafeCell, marker::PhantomPinned};
>>> +use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
>>>  use macros::pin_data;
>>>
>>>  pub mod mutex;
>>> @@ -121,7 +118,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.
>>> -    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
>>> +    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
>>
>> Static references do not need `Pin`, since `Pin::static_ref` [1] exists,
>> so you can just as well not add the `Pin` here and the other places
>> where you have `Pin<&'static T>`.
>>
> 
> You're right about `Pin` not needed for 'static. However, the
> `Pin<&'static LockClassKey>` signature is the intermediate state, and
> eventually we will need to support initializing a lock (and others) with
> a dynamically allocated `LockClassKey`, and when that is available, the
> type of `key` will become `Pin<&'a LockClassKey>`, so `Pin` is needed.
> 
> So I would like to keep this patch as it is. Works for you?

Makes sense and fine by me. It would be nice if it was also mentioned in
the commit message. Thanks!

---
Cheers,
Benno

> 
> Regards,
> Boqun
> 
>> The reasoning is that since the data lives for `'static` at that
>> location, it will never move (since it is borrowed for `'static` after
>> all).
>>
>> [1]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.static_ref
>>
>> ---
>> Cheers,
>> Benno
>>
>>>          pin_init!(Self {
>>>              data: UnsafeCell::new(t),
>>>              _pin: PhantomPinned,
>>> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
>>> index 480ee724e3cc..d65f94b5caf2 100644
>>> --- a/rust/kernel/sync/lock/global.rs
>>> +++ b/rust/kernel/sync/lock/global.rs
>>> @@ -13,6 +13,7 @@
>>>  use core::{
>>>      cell::UnsafeCell,
>>>      marker::{PhantomData, PhantomPinned},
>>> +    pin::Pin,
>>>  };
>>>
>>>  /// Trait implemented for marker types for global locks.
>>
>>

Re: [PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Mitchell Levy]

On Fri, Feb 07, 2025 at 12:22:19AM +0000, Benno Lossin wrote:
> On 06.02.25 22:27, Boqun Feng wrote:
> > On Wed, Feb 05, 2025 at 08:30:50PM +0000, Benno Lossin wrote:
> >> On 05.02.25 20:59, Mitchell Levy wrote:
> >>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> >>> index 41dcddac69e2..119e5f569bdb 100644
> >>> --- a/rust/kernel/sync/lock.rs
> >>> +++ b/rust/kernel/sync/lock.rs
> >>> @@ -7,12 +7,9 @@
> >>>
> >>>  use super::LockClassKey;
> >>>  use crate::{
> >>> -    init::PinInit,
> >>> -    pin_init,
> >>> -    str::CStr,
> >>> -    types::{NotThreadSafe, Opaque, ScopeGuard},
> >>> +    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
> >>>  };
> >>> -use core::{cell::UnsafeCell, marker::PhantomPinned};
> >>> +use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
> >>>  use macros::pin_data;
> >>>
> >>>  pub mod mutex;
> >>> @@ -121,7 +118,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.
> >>> -    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> >>> +    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
> >>
> >> Static references do not need `Pin`, since `Pin::static_ref` [1] exists,
> >> so you can just as well not add the `Pin` here and the other places
> >> where you have `Pin<&'static T>`.
> >>
> > 
> > You're right about `Pin` not needed for 'static. However, the
> > `Pin<&'static LockClassKey>` signature is the intermediate state, and
> > eventually we will need to support initializing a lock (and others) with
> > a dynamically allocated `LockClassKey`, and when that is available, the
> > type of `key` will become `Pin<&'a LockClassKey>`, so `Pin` is needed.
> > 
> > So I would like to keep this patch as it is. Works for you?
> 
> Makes sense and fine by me. It would be nice if it was also mentioned in
> the commit message. Thanks!

This makes sense and is a good point. Will resend with a fixed commit
message.

Thanks,
Mitchell

> ---
> Cheers,
> Benno
> 
> > 
> > Regards,
> > Boqun
> > 
> >> The reasoning is that since the data lives for `'static` at that
> >> location, it will never move (since it is borrowed for `'static` after
> >> all).
> >>
> >> [1]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.static_ref
> >>
> >> ---
> >> Cheers,
> >> Benno
> >>
> >>>          pin_init!(Self {
> >>>              data: UnsafeCell::new(t),
> >>>              _pin: PhantomPinned,
> >>> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> >>> index 480ee724e3cc..d65f94b5caf2 100644
> >>> --- a/rust/kernel/sync/lock/global.rs
> >>> +++ b/rust/kernel/sync/lock/global.rs
> >>> @@ -13,6 +13,7 @@
> >>>  use core::{
> >>>      cell::UnsafeCell,
> >>>      marker::{PhantomData, PhantomPinned},
> >>> +    pin::Pin,
> >>>  };
> >>>
> >>>  /// Trait implemented for marker types for global locks.
> >>
> >>
>