Re: [PATCH v3 13/16] rust: block: add `GenDisk` private data support

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

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jul 11, 2025 at 01:43:14PM +0200, Andreas Hindborg wrote:

..

>> @@ -97,7 +98,14 @@ pub fn build<T: Operations>(
>>          self,
>>          name: fmt::Arguments<'_>,
>>          tagset: Arc<TagSet<T>>,
>> +        queue_data: T::QueueData,
>>      ) -> Result<GenDisk<T>> {
>> +        let data = queue_data.into_foreign();
>> +        let recover_data = ScopeGuard::new(|| {
>> +            // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> +            unsafe { T::QueueData::from_foreign(data) };
>
> I think this is clearer to read as
>
> drop(unsafe { T::QueueData::from_foreign(data) });

OK.

>
>> +        });
>> +
>>          // SAFETY: `bindings::queue_limits` contain only fields that are valid when zeroed.
>>          let mut lim: bindings::queue_limits = unsafe { core::mem::zeroed() };
>>
>> @@ -112,7 +120,7 @@ pub fn build<T: Operations>(
>>              bindings::__blk_mq_alloc_disk(
>>                  tagset.raw_tag_set(),
>>                  &mut lim,
>> -                core::ptr::null_mut(),
>> +                data.cast(),
>>                  static_lock_class!().as_ptr(),
>>              )
>>          })?;
>> @@ -166,8 +174,12 @@ pub fn build<T: Operations>(
>>              },
>>          )?;
>>
>> +        recover_data.dismiss();
>> +
>>          // INVARIANT: `gendisk` was initialized above.
>>          // INVARIANT: `gendisk` was added to the VFS via `device_add_disk` above.
>> +        // INVARIANT: `gendisk.queue.queue_data` is set to `data` in the call to
>> +        // `__blk_mq_alloc_disk` above.
>>          Ok(GenDisk {
>>              _tagset: tagset,
>>              gendisk,
>> @@ -179,9 +191,10 @@ pub fn build<T: Operations>(
>>  ///
>>  /// # Invariants
>>  ///
>> -/// - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> -/// - `gendisk` was added to the VFS through a call to
>> -///   `bindings::device_add_disk`.
>> +///  - `gendisk` must always point to an initialized and valid `struct gendisk`.
>> +///  - `gendisk` was added to the VFS through a call to
>> +///    `bindings::device_add_disk`.
>> +///  - `self.gendisk.queue.queuedata` is initialized by a call to `ForeignOwnable::into_foreign`.
>>  pub struct GenDisk<T: Operations> {
>>      _tagset: Arc<TagSet<T>>,
>>      gendisk: *mut bindings::gendisk,
>> @@ -193,9 +206,20 @@ unsafe impl<T: Operations + Send> Send for GenDisk<T> {}
>>
>>  impl<T: Operations> Drop for GenDisk<T> {
>>      fn drop(&mut self) {
>> +        // SAFETY: By type invariant of `Self`, `self.gendisk` points to a valid
>> +        // and initialized instance of `struct gendisk`, and, `queuedata` was
>> +        // initialized with the result of a call to
>> +        // `ForeignOwnable::into_foreign`.
>> +        let queue_data = unsafe { (*(*self.gendisk).queue).queuedata };
>> +
>>          // SAFETY: By type invariant, `self.gendisk` points to a valid and
>>          // initialized instance of `struct gendisk`, and it was previously added
>>          // to the VFS.
>>          unsafe { bindings::del_gendisk(self.gendisk) };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDiskBuilder::build` with
>> +        // a call to `ForeignOwnable::into_foreign` to create `queuedata`.
>> +        // `ForeignOwnable::from_foreign` is only called here.
>> +        let _queue_data = unsafe { T::QueueData::from_foreign(queue_data.cast()) };
>
> Ditto here.
>
> drop(unsafe { T::QueueData::from_foreign(queue_data.cast()) });

Thanks, I agree.

>
> Also, is this cast necessary as of
> https://lore.kernel.org/all/20250711-rnull-up-v6-16-v3-13-3a262b4e2921@kernel.org/
> ?

I think you have the wrong link, you probably refer to the recent
`ForeignOwnable` change. And yes, the cast is redundant.


Best regards,
Andreas Hindborg