Re: [PATCH v3] block: assign caller-specific lockdep class to disk->open_mutex

[Andreas Hindborg]

Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> writes:

> On 2026/06/05 16:54, Andreas Hindborg wrote:
>> Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> writes:
>> 
>>> The block core currently allocates a single monolithic lockdep key for
>>> disk->open_mutex across all callers. This single key conflates locking
>>> hierarchies between independent block streams. For example, if a stacked
>>> driver like loop flushes its internal workqueues inside lo_release() while
>>> holding its own open_mutex, lockdep views this as a potential ABBA deadlock
>>> against the underlying storage stack, leading to numerous circular
>>> dependency splats.
>>>
>>> To structurally reduce false positives, this patch splits the global
>>> monolithic lock class into distinct, per-caller instances during disk
>>> allocation. This is done by replacing "struct lock_class_key" with
>>> "struct gendisk_lkclass", which contains two instances of
>>> "struct lock_class_key" for the legacy "(bio completion)" map and
>>> disk->open_mutex respectively.
>>>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
>> 
>> For the Rust part, we have existing infrastructure for lock class keys
>> [1]. Please take a look how we generate lock class keys elsewhere [2].
>
> My understanding is that we don't have infrastructure for lock class keys
> that can be applied to
>
>   +struct gendisk_lkclass {
>   +	struct lock_class_key bio_lkclass;
>   +	struct lock_class_key open_mutex_lkclass;
>   +};
>
>   -	static struct lock_class_key __key;
>   +	static struct gendisk_lkclass __key;
>
> change. Alternative approach is welcomed if you have one.

Sorry, I did not pay enough attention. I would suggest this approach:

diff --git a/drivers/block/rnull/rnull.rs b/drivers/block/rnull/rnull.rs
index 21557a1ce866..13048cea8bb0 100644
--- a/drivers/block/rnull/rnull.rs
+++ b/drivers/block/rnull/rnull.rs
@@ -68,7 +68,7 @@ fn new(
             .logical_block_size(block_size)?
             .physical_block_size(block_size)?
             .rotational(rotational)
-            .build(fmt!("{}", name.to_str()?), tagset, queue_data, kernel::my_gendisk_lkclass!())
+            .build(fmt!("{}", name.to_str()?), tagset, queue_data)
     }
 }
 
diff --git a/rust/kernel/block/mq.rs b/rust/kernel/block/mq.rs
index 10f22b200567..1fd0d54dd549 100644
--- a/rust/kernel/block/mq.rs
+++ b/rust/kernel/block/mq.rs
@@ -88,7 +88,7 @@
 //!     Arc::pin_init(TagSet::new(1, 256, 1), flags::GFP_KERNEL)?;
 //! let mut disk = gen_disk::GenDiskBuilder::new()
 //!     .capacity_sectors(4096)
-//!     .build(fmt!("myblk"), tagset, (), kernel::my_gendisk_lkclass!())?;
+//!     .build(fmt!("myblk"), tagset, ())?;
 //!
 //! # Ok::<(), kernel::error::Error>(())
 //! ```
diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs
index 8c452e90fe9d..58f87a184407 100644
--- a/rust/kernel/block/mq/gen_disk.rs
+++ b/rust/kernel/block/mq/gen_disk.rs
@@ -24,6 +24,7 @@
     sync::Arc,
     types::{
         ForeignOwnable,
+        Opaque,
         ScopeGuard, //
     }, //
 };
@@ -49,43 +50,6 @@ fn default() -> Self {
     }
 }
 
-/// A wrapper type for safely passing "struct gendisk_lkclass" argument.
-///
-/// This type can only be instantiated via the [`my_gendisk_lkclass!`] macro.
-pub struct GenDiskLockClass(pub(crate) *mut bindings::gendisk_lkclass);
-
-impl GenDiskLockClass {
-    /// Retrieve the underlying raw pointer.
-    pub(crate) fn as_ptr(&self) -> *mut bindings::gendisk_lkclass {
-        self.0
-    }
-}
-
-#[doc(hidden)]
-pub mod __internal {
-    use super::*;
-
-    /// Internal constructor used ONLY by the `my_gendisk_lkclass!` macro.
-    ///
-    /// SAFETY: `ptr` must point to a valid static `gendisk_lkclass` instance.
-    pub const unsafe fn new_lock_class(ptr: *mut bindings::gendisk_lkclass) -> GenDiskLockClass {
-        GenDiskLockClass(ptr)
-    }
-}
-
-/// Helper macro to generate a unique caller-local static lock class struct
-#[macro_export]
-macro_rules! my_gendisk_lkclass {
-    () => {{
-        static mut LKCLASS: $crate::bindings::gendisk_lkclass = $crate::bindings::gendisk_lkclass {
-            bio_lkclass: const { unsafe { ::core::mem::zeroed() } },
-            open_mutex_lkclass: const { unsafe { ::core::mem::zeroed() } },
-        };
-
-        unsafe { $crate::block::mq::gen_disk::__internal::new_lock_class(&raw mut LKCLASS) }
-    }};
-}
-
 impl GenDiskBuilder {
     /// Create a new instance.
     pub fn new() -> Self {
@@ -148,7 +112,6 @@ pub fn build<T: Operations>(
         name: fmt::Arguments<'_>,
         tagset: Arc<TagSet<T>>,
         queue_data: T::QueueData,
-        lkclass: GenDiskLockClass,
     ) -> Result<GenDisk<T>> {
         let data = queue_data.into_foreign();
         let recover_data = ScopeGuard::new(|| {
@@ -164,14 +127,21 @@ pub fn build<T: Operations>(
             lim.features = bindings::BLK_FEAT_ROTATIONAL;
         }
 
-        // SAFETY: `tagset.raw_tag_set()` points to a valid and initialized tag set
+        let keys = KBox::pin_init(
+            Opaque::ffi_init(|ptr: *mut bindings::gendisk_lkclass| {
+                // SAFETY: `ptr` is valid for writes
+                unsafe { bindings::lockdep_register_key(&raw mut (*ptr).bio_lkclass) };
+                // SAFETY: `ptr` is valid for writes
+                unsafe { bindings::lockdep_register_key(&raw mut (*ptr).open_mutex_lkclass) };
+            }),
+            GFP_KERNEL,
+        )?;
+
+        // SAFETY:
+        // - `tagset.raw_tag_set()` points to a valid and initialized tag set.
+        // - We keep `keys` alive for the lifetime of the returned gendisk.
         let gendisk = from_err_ptr(unsafe {
-            bindings::__blk_mq_alloc_disk(
-                tagset.raw_tag_set(),
-                &mut lim,
-                data,
-                lkclass.as_ptr(),
-            )
+            bindings::__blk_mq_alloc_disk(tagset.raw_tag_set(), &mut lim, data, keys.get())
         })?;
 
         const TABLE: bindings::block_device_operations = bindings::block_device_operations {
@@ -230,6 +200,7 @@ pub fn build<T: Operations>(
         Ok(GenDisk {
             _tagset: tagset,
             gendisk,
+            _lock_class_keys: keys,
         })
     }
 }
@@ -245,6 +216,7 @@ pub fn build<T: Operations>(
 pub struct GenDisk<T: Operations> {
     _tagset: Arc<TagSet<T>>,
     gendisk: *mut bindings::gendisk,
+    _lock_class_keys: Pin<KBox<Opaque<bindings::gendisk_lkclass>>>,
 }
 
 // SAFETY: `GenDisk` is an owned pointer to a `struct gendisk` and an `Arc` to a
---


Best regards,
Andreas Hindborg