By introducing a new_static() constructor, the macro does not need to go
through MaybeUninit::uninit().assume_init(), which is a pattern that is
best avoided when possible.

That the destructor must never run is a sufficient safety requirement
for new_static() because to actually use it you must also pin it.
This implies that the memory location remains valid forever, which is
what is needed when using a statically allocated lock class key.

Suggested-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync.rs | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 00f9b558a3ade19e442b32b46d05885b67e1d830..9545bedf47b67976ab8c22d8368991cf1f382e42 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -39,6 +39,20 @@ pub struct LockClassKey {
 unsafe impl Sync for LockClassKey {}
 
 impl LockClassKey {
+    /// Initializes a statically allocated lock class key.
+    ///
+    /// This is usually used indirectly through the [`static_lock_class!`] macro.
+    ///
+    /// # Safety
+    ///
+    /// The destructor must never run on the returned `LockClassKey`.
+    #[doc(hidden)]
+    pub const unsafe fn new_static() -> Self {
+        LockClassKey {
+            inner: Opaque::uninit(),
+        }
+    }
+
     /// 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.
     ///
@@ -95,13 +109,11 @@ fn drop(self: Pin<&mut Self>) {
 #[macro_export]
 macro_rules! static_lock_class {
     () => {{
-        static CLASS: $crate::sync::LockClassKey =
-            // Lockdep expects uninitialized memory when it's handed a statically allocated `struct
-            // lock_class_key`.
-            //
-            // SAFETY: `LockClassKey` transparently wraps `Opaque` which permits uninitialized
-            // memory.
-            unsafe { ::core::mem::MaybeUninit::uninit().assume_init() };
+        // SAFETY: The returned `LockClassKey` is stored in static memory, so its destructor will
+        // not run.
+        static CLASS: $crate::sync::LockClassKey = unsafe {
+            $crate::sync::LockClassKey::new_static()
+        };
         $crate::prelude::Pin::static_ref(&CLASS)
     }};
 }

-- 
2.50.0.727.gbf7dc18ff4-goog

On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
>  impl LockClassKey {
> +    /// Initializes a statically allocated lock class key.
> +    ///
> +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The destructor must never run on the returned `LockClassKey`.

I don't know how lockdep works, but Boqun mentioned in the other thread
that it uses the address of static keys. But AFAIK there is no mechanism
to differentiate them, so does lockdep just check the address and if it
is in a static segment it uses different behavior?

Because from the safety requirements on this function, I could just do
this:

    // SAFETY: we leak the box below, so the destructor never runs.
    let class = KBox::new(unsafe { LockClassKey::new_static() });
    let class = Pin::static_ref(KBox::leak(class));
    let lock = SpinLock::new(42, c_str!("test"), class);
    let _ = lock.lock();

Because if lockdep then expects this to be initialized, we need to
change the requirement to only be used from statics.

---
Cheers,
Benno

> +    #[doc(hidden)]
> +    pub const unsafe fn new_static() -> Self {
> +        LockClassKey {
> +            inner: Opaque::uninit(),
> +        }
> +    }

On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> >  impl LockClassKey {
> > +    /// Initializes a statically allocated lock class key.
> > +    ///
> > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// The destructor must never run on the returned `LockClassKey`.
>
> I don't know how lockdep works, but Boqun mentioned in the other thread
> that it uses the address of static keys. But AFAIK there is no mechanism
> to differentiate them, so does lockdep just check the address and if it
> is in a static segment it uses different behavior?
>
> Because from the safety requirements on this function, I could just do
> this:
>
>     // SAFETY: we leak the box below, so the destructor never runs.
>     let class = KBox::new(unsafe { LockClassKey::new_static() });
>     let class = Pin::static_ref(KBox::leak(class));
>     let lock = SpinLock::new(42, c_str!("test"), class);
>     let _ = lock.lock();
>
> Because if lockdep then expects this to be initialized, we need to
> change the requirement to only be used from statics.

My understanding is that it has to with correctly handling reuse of
the same key. In your scenario it's not reused.

Alice

On Wed, Jul 23, 2025 at 05:01:39PM +0200, Alice Ryhl wrote:
> On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
> >
> > On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> > >  impl LockClassKey {
> > > +    /// Initializes a statically allocated lock class key.
> > > +    ///
> > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> > > +    ///
> > > +    /// # Safety
> > > +    ///
> > > +    /// The destructor must never run on the returned `LockClassKey`.
> >
> > I don't know how lockdep works, but Boqun mentioned in the other thread
> > that it uses the address of static keys. But AFAIK there is no mechanism
> > to differentiate them, so does lockdep just check the address and if it

In lockdep, we use `static_obj()` to tell whether it's a static obj or a
dynamic allocated one.

> > is in a static segment it uses different behavior?
> >
> > Because from the safety requirements on this function, I could just do
> > this:
> >
> >     // SAFETY: we leak the box below, so the destructor never runs.
> >     let class = KBox::new(unsafe { LockClassKey::new_static() });
> >     let class = Pin::static_ref(KBox::leak(class));
> >     let lock = SpinLock::new(42, c_str!("test"), class);

This will trigger a runtime error because `class` is not static, but
technically, it won't trigger UB, at least lockdep should be able to
handle this case.

Regards,
Boqun

> >     let _ = lock.lock();
> >
> > Because if lockdep then expects this to be initialized, we need to
> > change the requirement to only be used from statics.
> 
> My understanding is that it has to with correctly handling reuse of
> the same key. In your scenario it's not reused.
> 
> Alice

On Wed, Jul 23, 2025 at 6:20 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Wed, Jul 23, 2025 at 05:01:39PM +0200, Alice Ryhl wrote:
> > On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
> > >
> > > On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> > > >  impl LockClassKey {
> > > > +    /// Initializes a statically allocated lock class key.
> > > > +    ///
> > > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> > > > +    ///
> > > > +    /// # Safety
> > > > +    ///
> > > > +    /// The destructor must never run on the returned `LockClassKey`.
> > >
> > > I don't know how lockdep works, but Boqun mentioned in the other thread
> > > that it uses the address of static keys. But AFAIK there is no mechanism
> > > to differentiate them, so does lockdep just check the address and if it
>
> In lockdep, we use `static_obj()` to tell whether it's a static obj or a
> dynamic allocated one.

Oh, ok. In that case I will adjust the safety comment.

Alice

On Wed Jul 23, 2025 at 6:20 PM CEST, Boqun Feng wrote:
> On Wed, Jul 23, 2025 at 05:01:39PM +0200, Alice Ryhl wrote:
>> On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
>> > On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
>> > >  impl LockClassKey {
>> > > +    /// Initializes a statically allocated lock class key.
>> > > +    ///
>> > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
>> > > +    ///
>> > > +    /// # Safety
>> > > +    ///
>> > > +    /// The destructor must never run on the returned `LockClassKey`.
>> >
>> > I don't know how lockdep works, but Boqun mentioned in the other thread
>> > that it uses the address of static keys. But AFAIK there is no mechanism
>> > to differentiate them, so does lockdep just check the address and if it
>
> In lockdep, we use `static_obj()` to tell whether it's a static obj or a
> dynamic allocated one.

So the code below will go in the non-static code path. Why doesn't it
need to be initialized/registered? (but other cases need it?)

>> > is in a static segment it uses different behavior?
>> >
>> > Because from the safety requirements on this function, I could just do
>> > this:
>> >
>> >     // SAFETY: we leak the box below, so the destructor never runs.
>> >     let class = KBox::new(unsafe { LockClassKey::new_static() });
>> >     let class = Pin::static_ref(KBox::leak(class));
>> >     let lock = SpinLock::new(42, c_str!("test"), class);
>
> This will trigger a runtime error because `class` is not static, but
> technically, it won't trigger UB, at least lockdep should be able to
> handle this case.

Could you go into more details? What is the "technically it won't
trigger UB" part about?

---
Cheers,
Benno

On Wed, Jul 23, 2025 at 09:46:03PM +0200, Benno Lossin wrote:
> On Wed Jul 23, 2025 at 6:20 PM CEST, Boqun Feng wrote:
> > On Wed, Jul 23, 2025 at 05:01:39PM +0200, Alice Ryhl wrote:
> >> On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
> >> > On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
> >> > >  impl LockClassKey {
> >> > > +    /// Initializes a statically allocated lock class key.
> >> > > +    ///
> >> > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
> >> > > +    ///
> >> > > +    /// # Safety
> >> > > +    ///
> >> > > +    /// The destructor must never run on the returned `LockClassKey`.
> >> >
> >> > I don't know how lockdep works, but Boqun mentioned in the other thread
> >> > that it uses the address of static keys. But AFAIK there is no mechanism
> >> > to differentiate them, so does lockdep just check the address and if it
> >
> > In lockdep, we use `static_obj()` to tell whether it's a static obj or a
> > dynamic allocated one.
> 
> So the code below will go in the non-static code path. Why doesn't it
> need to be initialized/registered? (but other cases need it?)
> 

Becasue all the dynamic lock class keys are put in a hash list (using an
intrusive single linked list), so you have to register it before use and
unregister after use.

> >> > is in a static segment it uses different behavior?
> >> >
> >> > Because from the safety requirements on this function, I could just do
> >> > this:
> >> >
> >> >     // SAFETY: we leak the box below, so the destructor never runs.
> >> >     let class = KBox::new(unsafe { LockClassKey::new_static() });
> >> >     let class = Pin::static_ref(KBox::leak(class));
> >> >     let lock = SpinLock::new(42, c_str!("test"), class);
> >
> > This will trigger a runtime error because `class` is not static, but
> > technically, it won't trigger UB, at least lockdep should be able to
> > handle this case.
> 
> Could you go into more details? What is the "technically it won't
> trigger UB" part about?
> 

If a dynamic key is not registered, lockdep will simply just skip the
initialization of locks, report an error and disable itself entirely. So
it won't cause UB.

Regards,
Boqun

> ---
> Cheers,
> Benno

On Wed Jul 23, 2025 at 10:07 PM CEST, Boqun Feng wrote:
> On Wed, Jul 23, 2025 at 09:46:03PM +0200, Benno Lossin wrote:
>> On Wed Jul 23, 2025 at 6:20 PM CEST, Boqun Feng wrote:
>> > On Wed, Jul 23, 2025 at 05:01:39PM +0200, Alice Ryhl wrote:
>> >> On Wed, Jul 23, 2025 at 4:36 PM Benno Lossin <lossin@kernel.org> wrote:
>> >> > On Wed Jul 23, 2025 at 1:49 PM CEST, Alice Ryhl wrote:
>> >> > >  impl LockClassKey {
>> >> > > +    /// Initializes a statically allocated lock class key.
>> >> > > +    ///
>> >> > > +    /// This is usually used indirectly through the [`static_lock_class!`] macro.
>> >> > > +    ///
>> >> > > +    /// # Safety
>> >> > > +    ///
>> >> > > +    /// The destructor must never run on the returned `LockClassKey`.
>> >> >
>> >> > I don't know how lockdep works, but Boqun mentioned in the other thread
>> >> > that it uses the address of static keys. But AFAIK there is no mechanism
>> >> > to differentiate them, so does lockdep just check the address and if it
>> >
>> > In lockdep, we use `static_obj()` to tell whether it's a static obj or a
>> > dynamic allocated one.
>> 
>> So the code below will go in the non-static code path. Why doesn't it
>> need to be initialized/registered? (but other cases need it?)
>> 
>
> Becasue all the dynamic lock class keys are put in a hash list (using an
> intrusive single linked list), so you have to register it before use and
> unregister after use.

Gotcha.

>> >> > is in a static segment it uses different behavior?
>> >> >
>> >> > Because from the safety requirements on this function, I could just do
>> >> > this:
>> >> >
>> >> >     // SAFETY: we leak the box below, so the destructor never runs.
>> >> >     let class = KBox::new(unsafe { LockClassKey::new_static() });
>> >> >     let class = Pin::static_ref(KBox::leak(class));
>> >> >     let lock = SpinLock::new(42, c_str!("test"), class);
>> >
>> > This will trigger a runtime error because `class` is not static, but
>> > technically, it won't trigger UB, at least lockdep should be able to
>> > handle this case.
>> 
>> Could you go into more details? What is the "technically it won't
>> trigger UB" part about?
>> 
>
> If a dynamic key is not registered, lockdep will simply just skip the
> initialization of locks, report an error and disable itself entirely. So
> it won't cause UB.

So the code above would trigger lockdep to disable itself?

And how does it detect that the class isn't registered? By checking for
the address in the hash list? Otherwise it would be UB, right? Could
there be a hash collision that induces UB?

---
Cheers,
Benno

On Wed, Jul 23, 2025 at 10:41:18PM +0200, Benno Lossin wrote:
[...]
> >> >> > is in a static segment it uses different behavior?
> >> >> >
> >> >> > Because from the safety requirements on this function, I could just do
> >> >> > this:
> >> >> >
> >> >> >     // SAFETY: we leak the box below, so the destructor never runs.
> >> >> >     let class = KBox::new(unsafe { LockClassKey::new_static() });
> >> >> >     let class = Pin::static_ref(KBox::leak(class));
> >> >> >     let lock = SpinLock::new(42, c_str!("test"), class);
> >> >
> >> > This will trigger a runtime error because `class` is not static, but
> >> > technically, it won't trigger UB, at least lockdep should be able to
> >> > handle this case.
> >> 
> >> Could you go into more details? What is the "technically it won't
> >> trigger UB" part about?
> >> 
> >
> > If a dynamic key is not registered, lockdep will simply just skip the
> > initialization of locks, report an error and disable itself entirely. So
> > it won't cause UB.
> 
> So the code above would trigger lockdep to disable itself?
> 

Yes.

> And how does it detect that the class isn't registered? By checking for
> the address in the hash list?

Right, in is_dynamice_key(), the hash list is scanned to see the key has
been registered.

> Otherwise it would be UB, right? Could there be a hash collision that
> induces UB?
> 

I don't think a hash collision will cause an UB, because the checking
only uses the address of the key, so even the key is uninitialized, it's
fine.

Regards,
Boqun

> ---
> Cheers,
> Benno