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.1On 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.
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.
>
>
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.
>>
>>
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.
> >>
> >>
>
© 2016 - 2026 Red Hat, Inc.