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

["Benno Lossin" <lossin@kernel.org>]

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