linux-kernel - Re: [PATCH] rust: sync: fix safety comment for `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: <DBIT5KD4CXDZ.1MZUPQMAWL4Y8@kernel.org>
Date: Tue, 22 Jul 2025 20:50:20 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Boqun Feng" <boqun.feng@...il.com>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>, "Miguel Ojeda" <ojeda@...nel.org>,
 "Alex Gaynor" <alex.gaynor@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <benno.lossin@...ton.me>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Lyude Paul" <lyude@...hat.com>, "Mitchell Levy" <levymitchell0@...il.com>,
 "Wedson Almeida Filho" <wedsonaf@...il.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] rust: sync: fix safety comment for `static_lock_class`

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@...nel.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@...nel.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