On 15.08.24 23:42, Gary Guo wrote:
> On Thu, 15 Aug 2024 14:32:28 -0700
> Boqun Feng <boqun.feng@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <nmi@xxxxxxxxxxxx> wrote:
>>>>> From: Andreas Hindborg <a.hindborg@xxxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxxxx>
>>>>> 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@xxxxxxxxxxx>
>>>>
>>>> LGTM. This makes me wonder if there's some design mistake in how we
>>>> handle lock classes in Rust.
>>>>
>>>> Reviewed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
>>>
>>> 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
