[PATCH] rust: sync: fix safety comment for `static_lock_class`

[PATCH] rust: sync: fix safety comment for `static_lock_class`

[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 <lossin@kernel.org>
---

I don't think we need to backport this.

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

diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
index 36a719015583..a10c812d8777 100644
--- a/rust/kernel/sync.rs
+++ b/rust/kernel/sync.rs
@@ -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
-- 
2.49.0

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Alice Ryhl]

On Wed, May 21, 2025 at 1:17 AM Benno Lossin <lossin@kernel.org> wrote:
>
> 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 <lossin@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Benno Lossin]

On Wed May 21, 2025 at 1:17 AM CEST, Benno Lossin wrote:
> 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 <lossin@kernel.org>
> ---
>
> I don't think we need to backport this.
>
> ---
>  rust/kernel/sync.rs | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> index 36a719015583..a10c812d8777 100644
> --- a/rust/kernel/sync.rs
> +++ b/rust/kernel/sync.rs
> @@ -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() };

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

>          $crate::prelude::Pin::static_ref(&CLASS)
>      }};
>
> base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Alice Ryhl]

On Tue, Jul 22, 2025 at 1:21 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Wed May 21, 2025 at 1:17 AM CEST, Benno Lossin wrote:
> > 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 <lossin@kernel.org>
> > ---
> >
> > I don't think we need to backport this.
> >
> > ---
> >  rust/kernel/sync.rs | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> > index 36a719015583..a10c812d8777 100644
> > --- a/rust/kernel/sync.rs
> > +++ b/rust/kernel/sync.rs
> > @@ -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() };
>
> 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?

Could that constructor be used in non-static cases?

Alice

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Miguel Ojeda]

On Tue, Jul 22, 2025 at 1:21 PM Benno Lossin <lossin@kernel.org> wrote:
>
> 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

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Miguel Ojeda]

On Wed, May 21, 2025 at 1:17 AM Benno Lossin <lossin@kernel.org> wrote:
>
> 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 <lossin@kernel.org>

Applied to `rust-next` -- thanks everyone!

Cheers,
Miguel

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Benno Lossin]

On Tue Jul 22, 2025 at 1:34 PM CEST, Alice Ryhl wrote:
> On Tue, Jul 22, 2025 at 1:21 PM Benno Lossin <lossin@kernel.org> wrote:
>> On Wed May 21, 2025 at 1:17 AM CEST, Benno Lossin wrote:
>> > 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 <lossin@kernel.org>
>> > ---
>> >
>> > I don't think we need to backport this.
>> >
>> > ---
>> >  rust/kernel/sync.rs | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> > index 36a719015583..a10c812d8777 100644
>> > --- a/rust/kernel/sync.rs
>> > +++ b/rust/kernel/sync.rs
>> > @@ -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() };
>>
>> 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?
>
> 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

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Boqun Feng]

On Tue, Jul 22, 2025 at 02:03:25PM +0200, Benno Lossin wrote:
> On Tue Jul 22, 2025 at 1:34 PM CEST, Alice Ryhl wrote:
> > On Tue, Jul 22, 2025 at 1:21 PM Benno Lossin <lossin@kernel.org> wrote:
> >> On Wed May 21, 2025 at 1:17 AM CEST, Benno Lossin wrote:
> >> > 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 <lossin@kernel.org>
> >> > ---
> >> >
> >> > I don't think we need to backport this.
> >> >
> >> > ---
> >> >  rust/kernel/sync.rs | 7 +++++--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
> >> > index 36a719015583..a10c812d8777 100644
> >> > --- a/rust/kernel/sync.rs
> >> > +++ b/rust/kernel/sync.rs
> >> > @@ -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() };
> >>
> >> 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?
> >
> > 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/20240815074519.2684107-3-nmi@metaspace.dk/

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

Regards,
Boqun

> abused?
> 
> ---
> Cheers,
> Benno

Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

[Benno Lossin]

On Tue Jul 22, 2025 at 4:34 PM CEST, Boqun Feng wrote:
> On Tue, Jul 22, 2025 at 02:03:25PM +0200, Benno Lossin wrote:
>> On Tue Jul 22, 2025 at 1:34 PM CEST, Alice Ryhl wrote:
>> > On Tue, Jul 22, 2025 at 1:21 PM Benno Lossin <lossin@kernel.org> wrote:
>> >> On Wed May 21, 2025 at 1:17 AM CEST, Benno Lossin wrote:
>> >> > 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 <lossin@kernel.org>
>> >> > ---
>> >> >
>> >> > I don't think we need to backport this.
>> >> >
>> >> > ---
>> >> >  rust/kernel/sync.rs | 7 +++++--
>> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs
>> >> > index 36a719015583..a10c812d8777 100644
>> >> > --- a/rust/kernel/sync.rs
>> >> > +++ b/rust/kernel/sync.rs
>> >> > @@ -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() };
>> >>
>> >> 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?
>> >
>> > 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/20240815074519.2684107-3-nmi@metaspace.dk/
>
> 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