[PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Andreas Hindborg]

From: Andreas Hindborg <a.hindborg@samsung.com>

The rust block device bindings include a wrong use of lock class key. This
causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.

This series fixes the issue by using a static lock class key. To do this, the
series first fixes the rust build system to correctly export rust statics from
the bss segment.

Andreas Hindborg (2):
  rust: fix export of bss symbols
  rust: block: fix wrong usage of lockdep API

 rust/Makefile                    | 2 +-
 rust/kernel/block/mq/gen_disk.rs | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)


base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
-- 
2.46.0

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

[Andreas Hindborg]

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>
---
 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(),
             )
         })?;
 
-- 
2.46.0

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

[Alice Ryhl]

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>

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

[Dirk Behme]

On 15.08.2024 09:49, Andreas Hindborg 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>

Tested-by: Dirk Behme <dirk.behme@de.bosch.com>

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(),
>               )
>           })?;
>   

-- 
======================================================================
Dirk Behme                      Robert Bosch Car Multimedia GmbH
                                 CM/ESO2
Phone: +49 5121 49-3274         Dirk Behme
Fax:   +49 711 811 5053274      PO Box 77 77 77
mailto:dirk.behme@de.bosch.com  D-31132 Hildesheim - Germany

Bosch Group, Car Multimedia (CM)
              Engineering SW Operating Systems 2 (ESO2)

Robert Bosch Car Multimedia GmbH - Ein Unternehmen der Bosch Gruppe
Sitz: Hildesheim
Registergericht: Amtsgericht Hildesheim HRB 201334
Aufsichtsratsvorsitzender: Dr. Dirk Hoheisel
Geschäftsführung: Dr. Steffen Berns;
                   Dr. Sven Ost, Jörg Pollak, Dr. Walter Schirm
======================================================================

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Jens Axboe]

On Thu, 15 Aug 2024 07:49:37 +0000, Andreas Hindborg wrote:
> The rust block device bindings include a wrong use of lock class key. This
> causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.
> 
> This series fixes the issue by using a static lock class key. To do this, the
> series first fixes the rust build system to correctly export rust statics from
> the bss segment.
> 
> [...]

Applied, thanks!

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

Best regards,
-- 
Jens Axboe

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Miguel Ojeda]

Hi Jens,

On Thu, Aug 15, 2024 at 4:22 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Applied, thanks!
>
> [2/2] rust: block: fix wrong usage of lockdep API
>       commit: d28b514ea3ae15274a4d70422ecc873bf6258e77

If you picked 2/2 only, it requires the former as far as I understand.

If you want to pick both:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Otherwise, I am happy to take them too.

Thanks!

Cheers,
Miguel

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Jens Axboe]

On 8/15/24 9:31 AM, Miguel Ojeda wrote:
> Hi Jens,
> 
> On Thu, Aug 15, 2024 at 4:22?PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> Applied, thanks!
>>
>> [2/2] rust: block: fix wrong usage of lockdep API
>>       commit: d28b514ea3ae15274a4d70422ecc873bf6258e77
> 
> If you picked 2/2 only, it requires the former as far as I understand.

Sorry I missed that, mostly because they were split into two separate
postings. Hence it only pulled one.

> If you want to pick both:
> 
> Acked-by: Miguel Ojeda <ojeda@kernel.org>
> 
> Otherwise, I am happy to take them too.

Go ahead and take them, I'll just kill the one I have. Thanks!

-- 
Jens Axboe

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

[Benno Lossin]

On 15.08.24 10:04, Alice Ryhl 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.

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

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

[Gary Guo]

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.

For the patch itself:

Reviewed-by: Gary Guo <gary@garyguo.net>

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

[Benno Lossin]

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

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

[Boqun Feng]

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> {
	        ....
	    }
	}

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 <gary@garyguo.net>

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

[Boqun Feng]

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

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

[Gary Guo]

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> {
> 	        ....
> 	    }
> 	}
> 
> 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

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Miguel Ojeda]

On Thu, Aug 15, 2024 at 6:00 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> Go ahead and take them, I'll just kill the one I have. Thanks!

Thanks, will do!

Cheers,
Miguel

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

[Benno Lossin]

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

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

[Benno Lossin]

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

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Miguel Ojeda]

On Thu, Aug 15, 2024 at 9:49 AM Andreas Hindborg <nmi@metaspace.dk> wrote:
>
> From: Andreas Hindborg <a.hindborg@samsung.com>
>
> The rust block device bindings include a wrong use of lock class key. This
> causes a WARN trace when lockdep is enabled and a `GenDisk` is constructed.
>
> This series fixes the issue by using a static lock class key. To do this, the
> series first fixes the rust build system to correctly export rust statics from
> the bss segment.

Applied to `rust-fixes` -- thanks everyone!

(I am sending the notice twice for this series, since somehow the
email threads got split into two in Lore, which also broke `b4`)

    [ Applied `rustfmt` and reworded slightly. - Miguel ]

Cheers,
Miguel

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Miguel Ojeda]

On Wed, Aug 21, 2024 at 12:50 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
>     [ Applied `rustfmt` and reworded slightly. - Miguel ]

Dirk noticed the Zulip link was broken due to a rename of the topic.
Normally I try to remember to convert them into message links (which
are permalinks) and to shorten them when they are too long too -- done
now.

For context, Zulip is gaining the ability to get topic permalinks (see
https://github.com/zulip/zulip/issues/21505 -- we gave them feedback a
year ago that it would be useful for kernel commits), though I think
it is not exposed through the UI yet.

Thanks Dirk!

Cheers,
Miguel

Re: [PATCH 0/2] rust: fix erranous use of lock class key in rust block device bindings

[Miguel Ojeda]

On Wed, Aug 21, 2024 at 1:36 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Dirk noticed the Zulip link was broken due to a rename of the topic.
> Normally I try to remember to convert them into message links (which
> are permalinks) and to shorten them when they are too long too -- done
> now.
>
> For context, Zulip is gaining the ability to get topic permalinks (see
> https://github.com/zulip/zulip/issues/21505 -- we gave them feedback a
> year ago that it would be useful for kernel commits), though I think
> it is not exposed through the UI yet.

It could be a good idea to warn about non-permalinks to Zulip in
`checkpatch.pl` -- Cc'ing Andy and Joe and created an issue:

    https://github.com/Rust-for-Linux/linux/issues/1104

Cheers,
Miguel