1. Patchew
  2. linux
  3. View series

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

Benno Lossin posted 1 patch 7 months ago 
rust/kernel/sync.rs | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
[PATCH] rust: sync: fix safety comment for `static_lock_class`
Posted by Benno Lossin 7 months ago 
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`
Posted by Miguel Ojeda 4 months, 4 weeks ago 
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`
Posted by Benno Lossin 4 months, 4 weeks ago 
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`
Posted by Miguel Ojeda 4 months, 4 weeks ago 
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`
Posted by Alice Ryhl 4 months, 4 weeks ago 
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`
Posted by Benno Lossin 4 months, 4 weeks ago 
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`
Posted by Boqun Feng 4 months, 4 weeks ago 
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`
Posted by Benno Lossin 4 months, 4 weeks ago 
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
Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`
Posted by Alice Ryhl 4 months, 4 weeks ago 
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>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.