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

[PATCH v2 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 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

---
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

---
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        |  3 +-
 8 files changed, 78 insertions(+), 23 deletions(-)
---
base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
change-id: 20240905-rust-lockdep-d3e30521c8ba

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

[PATCH v2 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..ae16bfd98de2 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();
+        // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
+        // lock_class_key
+        static CLASS: $crate::sync::LockClassKey =
+            unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
         &CLASS
     }};
 }

-- 
2.34.1

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

[kernel test robot]

Hi Mitchell,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0c5928deada15a8d075516e6e0d9ee19011bb000]

url:    https://github.com/intel-lab-lkp/linux/commits/Mitchell-Levy/rust-lockdep-Remove-support-for-dynamically-allocated-LockClassKeys/20241220-050220
base:   0c5928deada15a8d075516e6e0d9ee19011bb000
patch link:    https://lore.kernel.org/r/20241219-rust-lockdep-v2-1-f65308fbc5ca%40gmail.com
patch subject: [PATCH v2 1/2] rust: lockdep: Remove support for dynamically allocated LockClassKeys
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20241225/202412251433.T3BhO2CQ-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241225/202412251433.T3BhO2CQ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412251433.T3BhO2CQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> warning: unsafe block missing a safety comment
   --> rust/kernel/sync.rs:45:13
   |
   45  |             unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   ::: rust/kernel/block/mq/gen_disk.rs:111:17
   |
   111 |                 static_lock_class!().as_ptr(),
   |                 -------------------- in this macro invocation
   |
   = help: consider adding a safety comment on the preceding line
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
   = note: requested on the command line with `-W clippy::undocumented-unsafe-blocks`
   = note: this warning originates in the macro `static_lock_class` (in Nightly builds, run with -Z macro-backtrace for more info)
--
>> warning: unsafe block missing a safety comment
   --> rust/kernel/sync.rs:45:13
   |
   45  |             unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   ::: rust/kernel/workqueue.rs:218:21
   |
   218 |             work <- new_work!("Queue::try_spawn"),
   |                     ----------------------------- in this macro invocation
   |
   = help: consider adding a safety comment on the preceding line
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks
   = note: this warning originates in the macro `$crate::static_lock_class` which comes from the expansion of the macro `new_work` (in Nightly builds, run with -Z macro-backtrace for more info)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Boqun Feng]

On Thu, Dec 19, 2024 at 12:58:55PM -0800, 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(-)
> 
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 1eab7ebf25fd..ae16bfd98de2 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();
> +        // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
> +        // lock_class_key
> +        static CLASS: $crate::sync::LockClassKey =

About the clippy warning reported by 0day, I think you could resolve
that by moving the above safety comment here.

Regards,
Boqun

> +            unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
>          &CLASS
>      }};
>  }
> 
> -- 
> 2.34.1
> 

[PATCH v2 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        |  3 ++-
 8 files changed, 79 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 ae16bfd98de2..13bfdc647c5b 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();
+    ///
+    /// // SAFETY: `key_ptr` is returned by the above `into_foreign()`, whose `from_foreign()` has
+    /// // not yet been called.
+    /// stack_pin_init!(let num: SpinLock<u32> = SpinLock::new(
+    ///     0,
+    ///     c_str!("my_spinlock"),
+    ///     unsafe { <Pin<KBox<LockClassKey>> as ForeignOwnable>::borrow(key_ptr) }
+    /// ));
+    ///
+    /// drop(num);
+    ///
+    /// // 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 {
         // lock_class_key
         static CLASS: $crate::sync::LockClassKey =
             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..f8f2f971f985 100644
--- a/rust/kernel/workqueue.rs
+++ b/rust/kernel/workqueue.rs
@@ -369,7 +369,8 @@ 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>
+    #[allow(clippy::new_ret_no_self)]
+    pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self>
     where
         T: WorkItem<ID>,
     {

-- 
2.34.1

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

[Boqun Feng]

On Thu, Dec 19, 2024 at 12:58:56PM -0800, Mitchell Levy wrote:
> 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        |  3 ++-
>  8 files changed, 79 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 ae16bfd98de2..13bfdc647c5b 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();
> +    ///
> +    /// // SAFETY: `key_ptr` is returned by the above `into_foreign()`, whose `from_foreign()` has
> +    /// // not yet been called.
> +    /// stack_pin_init!(let num: SpinLock<u32> = SpinLock::new(
> +    ///     0,
> +    ///     c_str!("my_spinlock"),

Clippy complains the following unsafe block doesn't have a safety
comment, please move the above safety comment here.

> +    ///     unsafe { <Pin<KBox<LockClassKey>> as ForeignOwnable>::borrow(key_ptr) }
> +    /// ));
> +    ///
> +    /// drop(num);

Also clippy doesn't like using drop() on types that don't impl `Drop`,
we may need to create an extra scope to make clippy happy (in the same
time it makes no user on `key` anymore, like:

    /// {
    ///     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) }
    ///     ));
    /// }

Make sure to update the comments and format the code afterwards.

Thanks!

Regards,
Boqun

> +    ///
> +    /// // 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 {
>          // lock_class_key
>          static CLASS: $crate::sync::LockClassKey =
>              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..f8f2f971f985 100644
> --- a/rust/kernel/workqueue.rs
> +++ b/rust/kernel/workqueue.rs
> @@ -369,7 +369,8 @@ 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>
> +    #[allow(clippy::new_ret_no_self)]
> +    pub fn new(name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self>
>      where
>          T: WorkItem<ID>,
>      {
> 
> -- 
> 2.34.1
> 

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

[Boqun Feng]

On Thu, Dec 19, 2024 at 12:58:54PM -0800, Mitchell Levy wrote:
> 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.
> 

Thanks for doing this! I found some clippy warnings with the current
version, but overall it looks good to me. That said, appreciate it if
patch #2 gets more reviews on the interface changes, thanks!

Regards,
Boqun

> ---
> 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
> 
> ---
> 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
> 
> ---
> 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        |  3 +-
>  8 files changed, 78 insertions(+), 23 deletions(-)
> ---
> base-commit: 0c5928deada15a8d075516e6e0d9ee19011bb000
> change-id: 20240905-rust-lockdep-d3e30521c8ba
> 
> Best regards,
> -- 
> Mitchell Levy <levymitchell0@gmail.com>
>