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@xxxxxxxxx> > Suggested-by: Boqun Feng <boqun.feng@xxxxxxxxx> > Signed-off-by: Mitchell Levy <levymitchell0@xxxxxxxxx> > --- > 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 >
