Message-ID: <Z4EqKlucyepSsENy@boqun-archlinux>
Date: Fri, 10 Jan 2025 06:09:46 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Mitchell Levy <levymitchell0@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	linux-block@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: lockdep: Use Pin for all LockClassKey usages

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

Powered by blists - more mailing lists