linux-kernel - Re: [PATCH v4 12/15] rust: block: add `GenDisk` private data support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87a543fkh1.fsf@t14s.mail-host-address-is-not-set>
Date: Wed, 13 Aug 2025 14:56:58 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, Alex
 Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy
 Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, Trevor
 Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Jens Axboe
 <axboe@...nel.dk>, linux-block@...r.kernel.org,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 12/15] rust: block: add `GenDisk` private data support

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

> On Tue, Aug 12, 2025 at 10:44:30AM +0200, Andreas Hindborg wrote:
>> Allow users of the rust block device driver API to install private data in
>> the `GenDisk` structure.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
>
> Overall LGTM.
> Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
>
>>          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(|| {
>> +            drop(
>> +                // SAFETY: T::QueueData was created by the call to `into_foreign()` above
>> +                unsafe { T::QueueData::from_foreign(data) },
>> +            );
>
> This is usually formatted as:
>
> // SAFETY: T::QueueData was created by the call to `into_foreign()` above
> drop(unsafe { T::QueueData::from_foreign(data) });

I don't really have a preference, my optimization function was to
minimize distance to the unsafe block. Are there any rust guidelines on this?

>
>>  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) };
>> +
>> +        drop(
>> +            // 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.
>> +            unsafe { T::QueueData::from_foreign(queue_data) },
>> +        );
>
> Ditto here.
>
>>          //    reference counted by `ARef` until then.
>>          let rq = unsafe { Request::aref_from_raw((*bd).rq) };
>>
>> +        // SAFETY: `hctx` is valid as required by this function.
>> +        let queue_data = unsafe { (*(*hctx).queue).queuedata };
>> +
>> +        // SAFETY: `queue.queuedata` was created by `GenDisk::try_new()` with a
>> +        // call to `ForeignOwnable::into_pointer()` to create `queuedata`.
>> +        // `ForeignOwnable::from_foreign()` is only called when the tagset is
>> +        // dropped, which happens after we are dropped.
>> +        let queue_data = unsafe { T::QueueData::borrow(queue_data.cast()) };
>
> Is this cast necessary? Is it not a void pointer?

Leftover from old `ForeignOwnable` I think. I'll remove it.


Best regards,
Andreas Hindborg