On 06.09.24 01:13, Mitchell Levy via B4 Relay wrote: > From: Mitchell Levy <levymitchell0@xxxxxxxxx> > > The current LockClassKey API has soundness issues related to the use of > dynamically allocated LockClassKeys. In particular, these keys can be > used without being registered and don't have address stability. > > This fixes the issue by using Pin<&LockClassKey> and properly > registering/deregistering the keys on init/drop. > > Link: https://lore.kernel.org/rust-for-linux/20240815074519.2684107-1-nmi@xxxxxxxxxxxx/ > Suggested-by: Benno Lossin <benno.lossin@xxxxxxxxx> > Suggested-by: Boqun Feng <boqun.feng@xxxxxxxxx> > Signed-off-by: Mitchell Levy <levymitchell0@xxxxxxxxx> > --- > This change is based on applying the linked patch to the top of > rust-next. > > I'm sending this as an RFC because I'm not sure that using > Pin<&'static LockClassKey> is appropriate as the parameter for, e.g., > Work::new. This should preclude using dynamically allocated > LockClassKeys here, which might not be desirable. Unfortunately, using > Pin<&'a LockClassKey> creates other headaches as the compiler then > requires that T and PinImpl<Self> be bounded by 'a, which also seems > undesirable. I would be especially interested in feedback/ideas along > these lines. > --- > rust/kernel/block/mq/gen_disk.rs | 2 +- > rust/kernel/sync.rs | 30 +++++++++++++++++++++--------- > rust/kernel/sync/condvar.rs | 13 ++++++++----- > rust/kernel/sync/lock.rs | 6 +++--- > rust/kernel/workqueue.rs | 6 +++--- > 5 files changed, 36 insertions(+), 21 deletions(-) When I try to build with LOCKDEP=n, then I get this error: error[E0425]: cannot find function `lockdep_register_key` in crate `bindings` --> rust/kernel/sync.rs:40:65 | 40 | inner <- Opaque::ffi_init(|slot| unsafe { bindings::lockdep_register_key(slot) }) | ^^^^^^^^^^^^^^^^^^^^ not found in `bindings` error[E0425]: cannot find function `lockdep_unregister_key` in crate `bindings` --> rust/kernel/sync.rs:52:28 | 52 | unsafe { bindings::lockdep_unregister_key(self.as_ptr()) } | > diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs > index 708125dce96a..706ac3c532d5 100644 > --- a/rust/kernel/block/mq/gen_disk.rs > +++ b/rust/kernel/block/mq/gen_disk.rs > @@ -108,7 +108,7 @@ pub fn build<T: Operations>( > tagset.raw_tag_set(), > &mut lim, > core::ptr::null_mut(), > - static_lock_class!().as_ptr(), > + static_lock_class!().get_ref().as_ptr(), Why do we need the `get_ref()` calls? `Pin<&T>` implements `Deref<Target = T>`, so I don't think that it is necessary to add the `get_ref` calls. > ) > })?; > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > index 0ab20975a3b5..c46a296cbe6d 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; > @@ -20,7 +22,11 @@ > > /// 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. > @@ -28,18 +34,22 @@ unsafe impl Sync for LockClassKey {} > > impl LockClassKey { > /// Creates a new lock class key. > - pub const fn new() -> Self { > - Self(Opaque::uninit()) > + pub fn new_dynamic() -> impl PinInit<Self> { Can you add some more documentation on this function? For example: - directing people to use `static_lock_class!` if they only need a static key (AFAIK that is the common use-case). - Also would be nice if we had an example here. - I would change the first line to be "Creates a dynamic lock class key.". > + 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() > } > } > > -impl Default for LockClassKey { > - fn default() -> Self { > - Self::new() > +#[pinned_drop] > +impl PinnedDrop for LockClassKey { > + fn drop(self: Pin<&mut Self>) { > + unsafe { bindings::lockdep_unregister_key(self.as_ptr()) } > } > } > > @@ -48,8 +58,10 @@ fn default() -> Self { > #[macro_export] > macro_rules! static_lock_class { > () => {{ > - static CLASS: $crate::sync::LockClassKey = $crate::sync::LockClassKey::new(); > - &CLASS > + static CLASS: $crate::sync::LockClassKey = unsafe { > + ::core::mem::MaybeUninit::uninit().assume_init() > + }; > + $crate::prelude::Pin::static_ref(&CLASS) Just to make sure we get it right this time, is this true: "static `lock_class_key` values don't need to be initialized."? > }}; > } > > diff --git a/rust/kernel/sync/condvar.rs b/rust/kernel/sync/condvar.rs > index 2b306afbe56d..6c40b45e35cd 100644 > --- a/rust/kernel/sync/condvar.rs > +++ b/rust/kernel/sync/condvar.rs > @@ -14,9 +14,12 @@ > time::Jiffies, > types::Opaque, > }; > -use core::ffi::{c_int, c_long}; > -use core::marker::PhantomPinned; > -use core::ptr; > +use core::{ > + ffi::{c_int, c_long}, > + marker::PhantomPinned, > + pin::Pin, > + ptr, > +}; > use macros::pin_data; > > /// Creates a [`CondVar`] initialiser with the given name and a newly-created lock class. > @@ -102,13 +105,13 @@ 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 > // static lifetimes so they live indefinitely. > wait_queue_head <- Opaque::ffi_init(|slot| unsafe { > - bindings::__init_waitqueue_head(slot, name.as_char_ptr(), key.as_ptr()) > + bindings::__init_waitqueue_head(slot, name.as_char_ptr(), key.get_ref().as_ptr()) > }), > }) > } > diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs > index f6c34ca4d819..c6bdbb85a39c 100644 > --- a/rust/kernel/sync/lock.rs > +++ b/rust/kernel/sync/lock.rs > @@ -7,7 +7,7 @@ > > use super::LockClassKey; > use crate::{init::PinInit, pin_init, str::CStr, types::Opaque, types::ScopeGuard}; > -use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned}; > +use core::{cell::UnsafeCell, marker::PhantomData, marker::PhantomPinned, pin::Pin}; > use macros::pin_data; > > pub mod mutex; > @@ -106,14 +106,14 @@ 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, > // SAFETY: `slot` is valid while the closure is called and both `name` and `key` have > // static lifetimes so they live indefinitely. > state <- Opaque::ffi_init(|slot| unsafe { > - B::init(slot, name.as_char_ptr(), key.as_ptr()) > + B::init(slot, name.as_char_ptr(), key.get_ref().as_ptr()) > }), > }) > } > diff --git a/rust/kernel/workqueue.rs b/rust/kernel/workqueue.rs > index 553a5cba2adc..eefc2b7b578c 100644 > --- a/rust/kernel/workqueue.rs > +++ b/rust/kernel/workqueue.rs > @@ -367,9 +367,9 @@ impl<T: ?Sized, const ID: u64> Work<T, ID> { > /// Creates a new instance of [`Work`]. > #[inline] > #[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: Pin<&'static LockClassKey>) -> impl PinInit<Self> > where > - T: WorkItem<ID>, > + T: WorkItem<ID> `rustfmt` re-adds the comma. It also formats other parts of your patch differently. You can run it using the `rustfmt` target. --- Cheers, Benno > { > pin_init!(Self { > work <- Opaque::ffi_init(|slot| { > @@ -381,7 +381,7 @@ pub fn new(name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self > Some(T::Pointer::run), > false, > name.as_char_ptr(), > - key.as_ptr(), > + key.get_ref().as_ptr(), > ) > } > }), > > --- > base-commit: 8edf38a534a38e5d78470a98d43454e3b73e307c > change-id: 20240905-rust-lockdep-d3e30521c8ba > > Best regards, > -- > Mitchell Levy <levymitchell0@xxxxxxxxx> > >
