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

[Boqun Feng <boqun.feng@gmail.com>]

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