rust: sync: fix safety comment for `static_lock_class`

PATCH Applied Benno Lossin 4 participants 9 messages May 20, 2025 – Jul 22, 2025

Subsystems (2)

Rust Rust sync

lore

Share & tools

lore

Benno Lossin

author reviewer

2025-05-20 23:17 UTC #0 lore raw

PATCH 1/1 rust: sync: fix safety comment for `static_lock_class`

The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.

Signed-off-by Benno Lossin <[email protected]>

---

I don't think we need to backport this.

---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

rust/kernel/sync.rs +5 -2

index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {
     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };
         $crate::prelude::Pin::static_ref(&CLASS)
     }};

base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21

Signature · 1 line

2.49.0

Alice Ryhl

maintainer Reviewed-by on v1 1/1

2025-07-22 11:14 UTC #1 lore raw

Quoted 6 lines · Benno Lossin

The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>

Reviewed-by Alice Ryhl <[email protected]>

Benno Lossin

author reviewer

2025-07-22 11:21 UTC #2 lore raw

In reply to Benno Lossin

Quoted 19 lines · Benno Lossin

The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>
---
I don't think we need to backport this.
---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
rust/kernel/sync.rs +0 -0
index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {

rust/kernel/sync.rs fn drop(self: Pin<&mut Self>) { · line 101

Quoted 10 lines

rust/kernel/sync.rs +5 -2

     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };

Looking at this patch with fresh eyes (thanks for the bump, Alice :) I think we should rather have a public unsafe function on LockClassKey that creates an uninitialized lock class key. I'd like to avoid the MaybeUninit::uninit().assume_init() pattern, as it might confuse people & it looks very wrong.

We can take this patch, as it definitely is an improvement, but I think we should also just fix this properly. Any thoughts?

---
Cheers,
Benno

Quoted 4 lines

rust/kernel/sync.rs +0 -0
         $crate::prelude::Pin::static_ref(&CLASS)
     }};
base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21

Alice Ryhl

maintainer

2025-07-22 11:34 UTC #3 lore raw

Quoted 38 lines · Benno Lossin

Quoted 19 lines · Benno Lossin
The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>
---
I don't think we need to backport this.
---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
rust/kernel/sync.rs +0 -0
index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {
rust/kernel/sync.rs fn drop(self: Pin<&mut Self>) { · line 101
Quoted 10 lines
rust/kernel/sync.rs +5 -2
     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };
Looking at this patch with fresh eyes (thanks for the bump, Alice :) I think we should rather have a public unsafe function on LockClassKey that creates an uninitialized lock class key. I'd like to avoid the MaybeUninit::uninit().assume_init() pattern, as it might confuse people & it looks very wrong.

Quoted 2 lines

We can take this patch, as it definitely is an improvement, but I think we should also just fix this properly. Any thoughts?

Could that constructor be used in non-static cases?

Alice

Benno Lossin

author reviewer

2025-07-22 12:03 UTC #4 lore raw

Quoted 38 lines · Alice Ryhl

Quoted 37 lines · Benno Lossin
Quoted 19 lines · Benno Lossin
The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>
---
I don't think we need to backport this.
---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
rust/kernel/sync.rs +0 -0
index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {
rust/kernel/sync.rs fn drop(self: Pin<&mut Self>) { · line 101
Quoted 10 lines
rust/kernel/sync.rs +5 -2
     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };
Looking at this patch with fresh eyes (thanks for the bump, Alice :) I think we should rather have a public unsafe function on LockClassKey that creates an uninitialized lock class key. I'd like to avoid the MaybeUninit::uninit().assume_init() pattern, as it might confuse people & it looks very wrong.

Quoted 4 lines

Quoted 2 lines
We can take this patch, as it definitely is an improvement, but I think we should also just fix this properly. Any thoughts?
Could that constructor be used in non-static cases?

I don't know lockdep, so maybe yes? Or do you mean that it could be abused?

---
Cheers,
Benno

Boqun Feng

2025-07-22 14:34 UTC #5 lore raw

Quoted 39 lines · Benno Lossin

Quoted 38 lines · Alice Ryhl
Quoted 37 lines · Benno Lossin
Quoted 19 lines · Benno Lossin
The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>
---
I don't think we need to backport this.
---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
rust/kernel/sync.rs +0 -0
index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {
rust/kernel/sync.rs fn drop(self: Pin<&mut Self>) { · line 101
Quoted 10 lines
rust/kernel/sync.rs +5 -2
     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };
Looking at this patch with fresh eyes (thanks for the bump, Alice :) I think we should rather have a public unsafe function on LockClassKey that creates an uninitialized lock class key. I'd like to avoid the MaybeUninit::uninit().assume_init() pattern, as it might confuse people & it looks very wrong.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
We can take this patch, as it definitely is an improvement, but I think we should also just fix this properly. Any thoughts?
Could that constructor be used in non-static cases?
I don't know lockdep, so maybe yes? Or do you mean that it could be

Using in non-static cases is wrong. For static keys, lockdep could use it address as keys but for dynamic keys, since they can be freed, they have to be registered before use (that's what LockClassKey::new_dynamic() is about.

See this:

	https://lore.kernel.org/rust-for-linux/[email protected]/

We would need to add "for static only" for the proposed unsafe function.

Regards,
Boqun

Quoted 5 lines

abused?
---
Cheers,
Benno

Benno Lossin

author reviewer

2025-07-22 18:50 UTC #6 lore raw

Quoted 52 lines · Boqun Feng

Quoted 39 lines · Benno Lossin
Quoted 38 lines · Alice Ryhl
Quoted 37 lines · Benno Lossin
Quoted 19 lines · Benno Lossin
The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>
---
I don't think we need to backport this.
---
 rust/kernel/sync.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
rust/kernel/sync.rs +0 -0
index 36a719015583..a10c812d8777 100644
@@ -93,8 +93,11 @@ fn drop(self: Pin<&mut Self>) {
 macro_rules! static_lock_class {
rust/kernel/sync.rs fn drop(self: Pin<&mut Self>) { · line 101
Quoted 10 lines
rust/kernel/sync.rs +5 -2
     () => {{
         static CLASS: $crate::sync::LockClassKey =
-            // SAFETY: lockdep expects uninitialized memory when it's handed a statically allocated
-            // lock_class_key
+            // 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() };
Looking at this patch with fresh eyes (thanks for the bump, Alice :) I think we should rather have a public unsafe function on LockClassKey that creates an uninitialized lock class key. I'd like to avoid the MaybeUninit::uninit().assume_init() pattern, as it might confuse people & it looks very wrong.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
We can take this patch, as it definitely is an improvement, but I think we should also just fix this properly. Any thoughts?
Could that constructor be used in non-static cases?
I don't know lockdep, so maybe yes? Or do you mean that it could be
Using in non-static cases is wrong. For static keys, lockdep could use it address as keys but for dynamic keys, since they can be freed, they have to be registered before use (that's what LockClassKey::new_dynamic() is about.

Quoted 5 lines

See this:
	https://lore.kernel.org/rust-for-linux/[email protected]/
We would need to add "for static only" for the proposed unsafe function.

Yeah sounds good, I like it better than using the pattern above. We can also make it #[doc(hidden)], so people don't use it :)

---
Cheers,
Benno

Miguel Ojeda

2025-07-22 11:40 UTC #7 lore raw

In reply to Benno Lossin

Quoted 2 lines · Benno Lossin

We can take this patch, as it definitely is an improvement, but I think

Yeah, thanks, that was the intention -- we were checking a few older series :)

Cheers,
Miguel

Miguel Ojeda

Applied

2025-07-22 11:54 UTC #8 lore raw

In reply to Benno Lossin

Quoted 6 lines · Benno Lossin

The safety comment mentions lockdep -- which from a Rust perspective isn't important -- and doesn't mention the real reason for why it's sound to create LockClassKey as uninitialized memory.
Signed-off-by Benno Lossin <[email protected]>

Applied to rust-next -- thanks everyone!

Cheers,
Miguel