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

Commit Message

Andreas Hindborg Aug. 15, 2024, 7:49 a.m. UTC

From: Andreas Hindborg <[e-mail]>

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)" <[e-mail]>
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 <[e-mail]>
---
 rust/kernel/block/mq/gen_disk.rs | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Alice Ryhl Aug. 15, 2024, 8:04 a.m. UTC | #1

On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
>
> From: Andreas Hindborg <[e-mail]>
>
> 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)" <[e-mail]>
> 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 <[e-mail]>

LGTM. This makes me wonder if there's some design mistake in how we
handle lock classes in Rust.

Reviewed-by: Alice Ryhl <[e-mail]>

Dirk Behme Aug. 15, 2024, 10:02 a.m. UTC | #2

On 15.08.2024 09:49, Andreas Hindborg wrote:
> From: Andreas Hindborg <[e-mail]>
> 
> 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)" <[e-mail]>
> 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 <[e-mail]>

Tested-by: Dirk Behme <[e-mail]>

Many thanks! :)

Dirk

> ---
>   rust/kernel/block/mq/gen_disk.rs | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
> index f548a6199847..dbe560b09953 100644
> --- a/rust/kernel/block/mq/gen_disk.rs
> +++ b/rust/kernel/block/mq/gen_disk.rs
> @@ -6,7 +6,7 @@
>   //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
>   
>   use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
> -use crate::error;
> +use crate::{error, static_lock_class};
>   use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
>   use core::fmt::{self, Write};
>   
> @@ -93,8 +93,6 @@ pub fn build<T: Operations>(
>           name: fmt::Arguments<'_>,
>           tagset: Arc<TagSet<T>>,
>       ) -> Result<GenDisk<T>> {
> -        let lock_class_key = crate::sync::LockClassKey::new();
> -
>           // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>           let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>   
> @@ -110,7 +108,7 @@ pub fn build<T: Operations>(
>                   tagset.raw_tag_set(),
>                   &mut lim,
>                   core::ptr::null_mut(),
> -                lock_class_key.as_ptr(),
> +                static_lock_class!().as_ptr(),
>               )
>           })?;
>

Benno Lossin Aug. 15, 2024, 7:05 p.m. UTC | #3

On 15.08.24 10:04, Alice Ryhl wrote:
> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
>>
>> From: Andreas Hindborg <[e-mail]>
>>
>> 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)" <[e-mail]>
>> 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 <[e-mail]>
> 
> LGTM. This makes me wonder if there's some design mistake in how we
> handle lock classes in Rust.

So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
also movable. I think in this case we either just overlooked it or
thought that the C side would initialize it.

For those people that know about this, are there APIs that initialize
`lock_class_key` themselves? (ie not a function to initialize a lock
class key, but rather an API like `__blk_mq_alloc_disk`)
Because if it is usually expected that the class key is already
initialized, then I think we should change our abstraction.

Additionally, I think that it needs to be pinned, since it contains an
`struct hlist_node` (I might be wrong on this, but that looks and sounds
like an intrusive linked list).

Also the `new` function is probably prone for misuse, since it will
create a new lock class key every time it is run. But as I learned in
[1], the more common use-case is a single lock class key for several
locks. Therefore it might be a good idea to at least rename it to
`new_dynamic` or similar and add appropriate documentation pointing to
`static_lock_class!`.

[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755

---
Cheers,
Benno

Gary Guo Aug. 15, 2024, 7:07 p.m. UTC | #4

On Thu, 15 Aug 2024 10:04:56 +0200
Alice Ryhl <[e-mail]> wrote:

> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
> >
> > From: Andreas Hindborg <[e-mail]>
> >
> > 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)" <[e-mail]>
> > 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 <[e-mail]>  
> 
> LGTM. This makes me wonder if there's some design mistake in how we
> handle lock classes in Rust.
> 
> Reviewed-by: Alice Ryhl <[e-mail]>

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.

For the patch itself:

Reviewed-by: Gary Guo <[e-mail]>

Benno Lossin Aug. 15, 2024, 7:15 p.m. UTC | #5

On 15.08.24 21:05, Benno Lossin wrote:
> On 15.08.24 10:04, Alice Ryhl wrote:
>> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
>>>
>>> From: Andreas Hindborg <[e-mail]>
>>>
>>> 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)" <[e-mail]>
>>> 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 <[e-mail]>
>>
>> LGTM. This makes me wonder if there's some design mistake in how we
>> handle lock classes in Rust.
> 
> So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
> also movable. I think in this case we either just overlooked it or
> thought that the C side would initialize it.
> 
> For those people that know about this, are there APIs that initialize
> `lock_class_key` themselves? (ie not a function to initialize a lock
> class key, but rather an API like `__blk_mq_alloc_disk`)
> Because if it is usually expected that the class key is already
> initialized, then I think we should change our abstraction.

Sorry, I got confused, this has nothing to do with initialization.

---
Cheers,
Benno

> Additionally, I think that it needs to be pinned, since it contains an
> `struct hlist_node` (I might be wrong on this, but that looks and sounds
> like an intrusive linked list).
> 
> Also the `new` function is probably prone for misuse, since it will
> create a new lock class key every time it is run. But as I learned in
> [1], the more common use-case is a single lock class key for several
> locks. Therefore it might be a good idea to at least rename it to
> `new_dynamic` or similar and add appropriate documentation pointing to
> `static_lock_class!`.
> 
> [1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755
> 
> ---
> Cheers,
> Benno
> 
>

Boqun Feng Aug. 15, 2024, 9:32 p.m. UTC | #6

On Thu, Aug 15, 2024 at 08:07:38PM +0100, Gary Guo wrote:
> On Thu, 15 Aug 2024 10:04:56 +0200
> Alice Ryhl <[e-mail]> wrote:
> 
> > On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
> > >
> > > From: Andreas Hindborg <[e-mail]>
> > >
> > > 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)" <[e-mail]>
> > > 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 <[e-mail]>  
> > 
> > LGTM. This makes me wonder if there's some design mistake in how we
> > handle lock classes in Rust.
> > 
> > Reviewed-by: Alice Ryhl <[e-mail]>
> 
> 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> {
	        ....
	    }
	}

and the new `static_lock_class!`:

	macro_rules! static_lock_class {
	    () => {{
		static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_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()) }
	    }};
	}

Thoughts?

Regards,
Boqun

> For the patch itself:
> 
> Reviewed-by: Gary Guo <[e-mail]>

Boqun Feng Aug. 15, 2024, 9:34 p.m. UTC | #7

On Thu, Aug 15, 2024 at 07:15:43PM +0000, Benno Lossin wrote:
> On 15.08.24 21:05, Benno Lossin wrote:
> > On 15.08.24 10:04, Alice Ryhl wrote:
> >> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
> >>>
> >>> From: Andreas Hindborg <[e-mail]>
> >>>
> >>> 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)" <[e-mail]>
> >>> 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 <[e-mail]>
> >>
> >> LGTM. This makes me wonder if there's some design mistake in how we
> >> handle lock classes in Rust.
> > 
> > So `LockClassKey::new` doesn't initialize the `lock_class_key` and is
> > also movable. I think in this case we either just overlooked it or
> > thought that the C side would initialize it.
> > 
> > For those people that know about this, are there APIs that initialize
> > `lock_class_key` themselves? (ie not a function to initialize a lock
> > class key, but rather an API like `__blk_mq_alloc_disk`)
> > Because if it is usually expected that the class key is already
> > initialized, then I think we should change our abstraction.
> 
> Sorry, I got confused, this has nothing to do with initialization.
> 

For static allocated key, no initialization is needed, for dynamic
allocated key, lockdep_register_key() will need to be called before
using the key.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > Additionally, I think that it needs to be pinned, since it contains an
> > `struct hlist_node` (I might be wrong on this, but that looks and sounds
> > like an intrusive linked list).
> > 
> > Also the `new` function is probably prone for misuse, since it will
> > create a new lock class key every time it is run. But as I learned in
> > [1], the more common use-case is a single lock class key for several
> > locks. Therefore it might be a good idea to at least rename it to
> > `new_dynamic` or similar and add appropriate documentation pointing to
> > `static_lock_class!`.
> > 
> > [1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/.E2.9C.94.206.2E11.2E0-rc1.3A.20rust.2Fkernel.2Fblock.2Fmq.2Ers.3A.20doctest.20lock.20warning/near/460074755
> > 
> > ---
> > Cheers,
> > Benno
> > 
> > 
>

Gary Guo Aug. 15, 2024, 9:42 p.m. UTC | #8

On Thu, 15 Aug 2024 14:32:28 -0700
Boqun Feng <[e-mail]> 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 <[e-mail]> wrote:
> >   
> > > On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:  
> > > >
> > > > From: Andreas Hindborg <[e-mail]>
> > > >
> > > > 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)" <[e-mail]>
> > > > 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 <[e-mail]>    
> > > 
> > > LGTM. This makes me wonder if there's some design mistake in how we
> > > handle lock classes in Rust.
> > > 
> > > Reviewed-by: Alice Ryhl <[e-mail]>  
> > 
> > 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> {
> 	        ....
> 	    }
> 	}
> 
> and the new `static_lock_class!`:
> 
> 	macro_rules! static_lock_class {
> 	    () => {{
> 		static CLASS: MaybeUninit<$crate::sync::LockClassKey> = $crate::sync::LockClassKey::new_uninit();

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.

Best,
Gary

Benno Lossin Aug. 16, 2024, 1:08 p.m. UTC | #9

On 15.08.24 23:42, Gary Guo wrote:
> On Thu, 15 Aug 2024 14:32:28 -0700
> Boqun Feng <[e-mail]> 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 <[e-mail]> wrote:
>>>> On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <[e-mail]> wrote:
>>>>> From: Andreas Hindborg <[e-mail]>
>>>>>
>>>>> 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)" <[e-mail]>
>>>>> 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 <[e-mail]>
>>>>
>>>> LGTM. This makes me wonder if there's some design mistake in how we
>>>> handle lock classes in Rust.
>>>>
>>>> Reviewed-by: Alice Ryhl <[e-mail]>
>>>
>>> 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

Benno Lossin Aug. 16, 2024, 3:59 p.m. UTC | #10

On 15.08.24 09:49, Andreas Hindborg wrote:
> From: Andreas Hindborg <[e-mail]>
> 
> 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)" <[e-mail]>
> 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 <[e-mail]>
> ---
>  rust/kernel/block/mq/gen_disk.rs | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Benno Lossin <[e-mail]>

---
Cheers,
Benno

Patch

diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index f548a6199847..dbe560b09953 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -6,7 +6,7 @@ 
 //! C header: [`include/linux/blk_mq.h`](srctree/include/linux/blk_mq.h)
 
 use crate::block::mq::{raw_writer::RawWriter, Operations, TagSet};
-use crate::error;
+use crate::{error, static_lock_class};
 use crate::{bindings, error::from_err_ptr, error::Result, sync::Arc};
 use core::fmt::{self, Write};
 
@@ -93,8 +93,6 @@  pub fn build<T: Operations>(
         name: fmt::Arguments<'_>,
         tagset: Arc<TagSet<T>>,
     ) -> Result<GenDisk<T>> {
-        let lock_class_key = crate::sync::LockClassKey::new();
-
         // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
         let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
 
@@ -110,7 +108,7 @@  pub fn build<T: Operations>(
                 tagset.raw_tag_set(),
                 &mut lim,
                 core::ptr::null_mut(),
-                lock_class_key.as_ptr(),
+                static_lock_class!().as_ptr(),
             )
         })?;