linux-kernel - Re: [PATCH v2 1/2] rust: sync: refactor static_lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <DBNM0XTCESZY.18G1A6U91QNE6@kernel.org>
Date: Mon, 28 Jul 2025 12:18:29 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Miguel Ojeda" <ojeda@...nel.org>
Cc: "Gary Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Daniel Almeida" <daniel.almeida@...labora.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] rust: sync: refactor static_lock_class!() macro

On Mon Jul 28, 2025 at 11:42 AM CEST, Alice Ryhl wrote:
> By introducing a new_static() constructor, the macro does not need to go
> through MaybeUninit::uninit().assume_init(), which is a pattern that is
> best avoided when possible.
>
> The safety comment requires not only requires that the value is leaked,

"requires" appears twice.

> but also that it is stored in the right portion of memory. This is so
> that the lockdep static_obj() check will succeed when using this
> constructor. One could argue that lockdep detects this scenario, so the
> safety comment isn't needed. However, it simplifies matters to require
> that static_obj() will succeed and it's not a burdensome requirement on
> the caller.

I'd argue that's implementation detail and the safety requirement of
using a lockclass key is that it either is uninit in static memory or it
was registered. (otherwise we wouldn't be "allowed" to add this as a
safety requirement)

(just adding this for info, feel free to keep the paragraph above as-is)

> Suggested-by: Benno Lossin <lossin@...nel.org>
> Reviewed-by: Daniel Almeida <daniel.almeida@...labora.com>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>

Reviewed-by: Benno Lossin <lossin@...nel.org>

> @@ -95,13 +110,11 @@ fn drop(self: Pin<&mut Self>) {
>  #[macro_export]
>  macro_rules! static_lock_class {
>      () => {{
> -        static CLASS: $crate::sync::LockClassKey =
> -            // 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() };
> +        // SAFETY: The returned `LockClassKey` is stored in static memory. Drop never runs on a

You're not mentioning the "pinned in a static memory location" part
(only the static memory, so missing the pinning). A read-only static
is implicitly pinned, so we should mention that.

---
Cheers,
Benno

> +        // static global.
> +        static CLASS: $crate::sync::LockClassKey = unsafe {
> +            $crate::sync::LockClassKey::new_static()
> +        };
>          $crate::prelude::Pin::static_ref(&CLASS)
>      }};
>  }