Message-ID: <66976464-ef42-4020-9f13-2038fd5b22cc@gmail.com>
Date: Mon, 27 Jan 2025 14:35:51 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>,
 Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: miscdevice: Add additional data to
 MiscDeviceRegistration

On 27.01.25 2:33 PM, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
> <chrisi.schrefl@...il.com> wrote:
>>
>> Hi Alice
>>
>> On 27.01.25 11:27 AM, Alice Ryhl wrote:
>>> <snip>
>>>>
>>>> to make sure that `misc_register` is called after data is initialized and to that
>>>> `data` will be dropped correctly in case `misc_register` fails.
>>>>
>>>> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?
>>>
>>> Using pin_chain is definitely incorrect because it will run the
>>> destructor of MiscDeviceRegistration if the misc_register call fails,
>>> but calling misc_deregister is incorrect in that case.
>>>
>>> You should be able to just move the `data <-
>>> UnsafePinned::try_pin_init(data)` line to the top so that the field is
>>> initialized first.
>>>
>>
>> So if I understand correctly, the following should be correct:
>>
>> pub fn register(
>>     opts: MiscDeviceOptions,
>>     data: impl PinInit<T::RegistrationData, Error>,
>> ) -> impl PinInit<Self, Error> {
>>     try_pin_init!(Self {
>>         data <- UnsafePinned::try_pin_init(data),
>>         inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
>>             // SAFETY: The initializer can write to the provided `slot`.
>>             unsafe { slot.write(opts.into_raw::<T>()) };
>>             // SAFETY: We just wrote the misc device options to the slot. The miscdevice will
>>             // get unregistered before `slot` is deallocated because the memory is pinned and
>>             // the destructor of this type deallocates the memory.
>>             // `data` is Initialized before `misc_register` so no race with `fops->open()`
>>             // is possible.
>>             // INVARIANT: If this returns `Ok(())`, then the `slot` will contain a registered
>>             // misc device.
>>             to_result(unsafe { bindings::misc_register(slot) })
>>         }),
>>         _t: PhantomData,
>>     })
>> }
>>
>> Sorry I don't know the details of `(try_)pin_init` and the docs only say:
>>> The fields are initialized in the order that they appear in the initializer. So it is possible
>>> to read already initialized fields using raw pointers.
>>>
>>> IMPORTANT: You are not allowed to create references to fields of the struct inside of the
>>> initializer.
>>
>> This says its invalid to create references, but as soon as `misc_register` its theoretically
>> possible that a `open()` call happens and creates a reference to the Registration. I assume
>> that is fine, because the Registration would be fully initialized at that point, but that's
>> technically against the Docs.
> 
> Creating a reference immediately after misc_register finishes should
> be fine. I think that warning is more strict than necessary.
> 
>> Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?
> 
> Yep. On failure, previous fields are cleaned up.
> 
>> I couldn't find anything in the Docs about when and what is dropped.
>>
>> Is there a equivalent to `cargo expand` for the kernel?
>> It would be nice to be able to look at the code generated by the macros.
> 
> You can expand Rust macros by invoking make on the file with an .i
> extension, in exactly the same way as you expand C macros.

Thanks!

Powered by blists - more mailing lists