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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Tetsuo Handa" <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> On 2026/06/19 18:49, Andreas Hindborg wrote:
>>> 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:
>
> I'm OK with your module-specific lockdep class approach
>
>> @@ -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 {
>
> if we can assume that there is (up to) only one
>
>   let mut disk = gen_disk::GenDiskBuilder::new().capacity_sectors(4096).build(fmt!("myblk"), tagset, ())?;
>
> call for each rust module.

With the approach I suggest here, we get a new set of keys for each
invocation of `GenDiskBuilder::build`. No statics involved, keys are
allocated dynamically.

The current code makes one key for to be shared by _all_ callers, which
is probably a bad idea.

>
> By the way, are you aware that rust-enabled linux-next build is recently failing
> ( https://syzkaller.appspot.com/bug?extid=1f14a35d0c73d31555e4 ) ?

I was not aware, thanks for pointing that out. I actually did build
linux-next for both June 16 and June 18 and I did not see any issue. It
looks like a tooling problem to me. I'll see if I can figure out what it
is.


Best regards,
Andreas Hindborg