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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aH-hcnZ3hiQIQj-5@tardis-2.local>
Date: Tue, 22 Jul 2025 07:34:26 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Benno Lossin <lossin@...nel.org>
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 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.

Regards,
Boqun

> abused?
> 
> ---
> Cheers,
> Benno