Re: [PATCH 2/2] rust: block: fix wrong usage of lockdep API

[Benno Lossin <benno.lossin@proton.me>]

On 15.08.24 23:42, Gary Guo wrote:
> On Thu, 15 Aug 2024 14:32:28 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
>> On Thu, Aug 15, 2024 at 08:07:38PM +0100, Gary Guo wrote:
>>> On Thu, 15 Aug 2024 10:04:56 +0200
>>> Alice Ryhl <aliceryhl@google.com> wrote:
>>>> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <nmi@metaspace.dk> wrote:
>>>>> From: Andreas Hindborg <a.hindborg@samsung.com>
>>>>>
>>>>> When allocating `struct gendisk`, `GenDiskBuilder` is using a dynamic lock
>>>>> class key without registering the key. This is incorrect use of the API,
>>>>> which causes a `WARN` trace. This patch fixes the issue by using a static
>>>>> lock class key, which is more appropriate for the situation anyway.
>>>>>
>>>>> Fixes: 3253aba3408a ("rust: block: introduce `kernel::block::mq` module")
>>>>> Reported-by: "Behme Dirk (XC-CP/ESB5)" <Dirk.Behme@de.bosch.com>
>>>>> Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/6.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning
>>>>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>>>>
>>>> LGTM. This makes me wonder if there's some design mistake in how we
>>>> handle lock classes in Rust.
>>>>
>>>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>>>
>>> I agree. The API that we current have is designed without much
>>> consideration into dynamically allocated keys, and we use `&'static
>>> LockClassKey` in a lot of kernel crate APIs.
>>>
>>> This arguably is wrong, because presence of `&'static LockClassKey`
>>> doesn't mean the key is static. If we do a
>>> `Box::leak(Box::new(LockClassKey::new()))`, then this is a `&'static
>>> LockClassKey`, but lockdep wouldn't consider this as a static object.
>>>
>>> Maybe we should make the `new` function unsafe.
>>>
>>
>> I think a more proper fix is to make LockClassKey pin-init, for
>> dynamically allocated LockClassKey, we just use lockdep_register_key()
>> as the initializer and lockdep_unregister_key() as the desconstructor.
>> And instead of a `&'static LockClassKey`, we should use `Pin<&'static
>> LockClassKey>` to pass a lock class key. Of course we will need some
>> special treatment on static allocated keys (e.g. assume they are
>> initialized since lockdep doesn't require initialization for them).
>>
>>
>> Pin initializer:
>>
>> 	impl LockClassKey {
>> 	    pub fn new() -> impl PinInit<Self> {
>> 		pin_init!(Self {
>> 		    inner <- Opaque::ffi_init(|slot| { lockdep_register_key(slot) })
>> 		})
>> 	    }
>> 	}
>>
>> LockClassKey::new_uninit() for `static_lock_class!`:
>>
>>
>> 	impl LockClassKey {
>> 	    pub const fn new_uninit() -> MaybeUninit<Self> {

We don't need to wrap it in `MaybeUninit`, since it already is
containing an `Opaque`. But I think we don't need to expose this
function at all, see below.

>> 	        ....
>> 	    }
>> 	}
>>
>> and the new `static_lock_class!`:
>>
>> 	macro_rules! static_lock_class {
>> 	    () => {{
>> 		static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_uninit();

    () => {{
        // SAFETY: `LockClassKey` contains a single field of type `Opaque` and thus an uninitialized
        // value is valid.
        static CLASS: $crate::sync::LockClassKey = unsafe {
            ::core::mem::MaybeUninit::uninit().assume_init()
        };
        Pin::from_static(&CLASS)
    }};

That way users can either create a static class, or a dynamic one via
`new_dynmaic` (I think we should rename it while we're at it), which is
always registered.

> nit: this could just be `MaybeUninit::uninit()`
> 
>>
>> 	        // SAFETY: `CLASS` is pinned because it's static
>> 		// allocated. And it's OK to assume it's initialized
>> 		// because lockdep support uninitialized static
>> 		// allocated key.
>> 		unsafe { Pin::new_unchecked(CLASS.assume_init_ref()) }
> 
> nit: this could be `Pin::from_static(unsafe { CLASS.assume_init_ref() })`
> 
>> 	    }};
>> 	}
>>
>> Thoughts?
> 
> I think this design looks good. I suggested adding unsafe as a quick
> way to address the pontential misuse, when we have no user for
> dynamically allocated keys.

I think we should do it properly, since the solution seems easy.

---
Cheers,
Benno