Message-Id: <DAEQ7VRHEP4W.4O0KV31IPJFG@kernel.org>
Date: Thu, 05 Jun 2025 18:05:40 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Christian Schrefl" <chrisi.schrefl@...il.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Danilo Krummrich" <dakr@...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>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Arnd Bergmann" <arnd@...db.de>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Lee Jones" <lee@...nel.org>, "Daniel
 Almeida" <daniel.almeida@...labora.com>
Cc: Gerald Wisböck <gerald.wisboeck@...ther.ink>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] rust: miscdevice: add additional data to
 MiscDeviceRegistration

On Thu Jun 5, 2025 at 4:57 PM CEST, Christian Schrefl wrote:
> On 04.06.25 1:29 AM, Benno Lossin wrote:
>> On Mon Jun 2, 2025 at 11:16 PM CEST, Christian Schrefl wrote:
>>> On 31.05.25 2:23 PM, Benno Lossin wrote:
>>>> On Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
>>>>> +// SAFETY:
>>>>> +// - All `&self` methods on this type are written to ensure that it is safe to call them in
>>>>> +//   parallel.
>>>>> +// - `MiscDevice::RegistrationData` is always `Sync`.
>>>>> +unsafe impl<T: MiscDevice> Sync for MiscDeviceRegistration<T> {}
>>>>
>>>> I would feel better if we still add the `T::RegistrationData: Sync`
>>>> bound here even if it is vacuous today.
>>>
>>> Since a reference the `MiscDeviceRegistration` struct is an
>>> argument to the open function this struct must always be Sync,
>>> so adding bounds here doesn't make much sense.
>> 
>> Well yes, but this statement makes `MiscDeviceRegistration` be `Sync`
>> even if `T::RegistrationData` is not `Sync` if that bound got removed
>> at some point. And this "instability" is what I'm worried about.
>> 
>>> I'll add this a safety comment in `MiscdeviceVTable::open`
>>> about this.
>>>
>>> Is there a good way to assert this at build to avoid regessions?
>> 
>>     const _: () = {
>>         fn assert_sync<T: ?Sized + Sync>() {}
>>         fn ctx<T: MiscDevice>() {
>>             assert_sync::<T::RegistrationData>();
>>         }
>>     };
>> 
>
> I'll add the bound and a TODO about `assert_sync`, in `open`
> where `Send` is required.
>
> I intend to write a patch for `assert_sync` later.

Great :)

>> That would also be fine with me if you insist on not adding the bound.
>> 
>> (the `assert_sync` function should maybe be somewhere where everyone can
>> use it)
>> 
>>>>>  impl<T: MiscDevice> MiscDeviceRegistration<T> {
>>>>>      /// Register a misc device.
>>>>> -    pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
>>>>> +    pub fn register(
>>>>> +        opts: MiscDeviceOptions,
>>>>> +        data: impl PinInit<T::RegistrationData, Error>,
>>>>> +    ) -> impl PinInit<Self, Error> {
>>>>>          try_pin_init!(Self {
>>>>> +            data <- Opaque::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.
>>>>> +                // 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) })
>>>>> @@ -93,13 +108,24 @@ pub fn device(&self) -> &Device {
>>>>>          // before the underlying `struct miscdevice` is destroyed.
>>>>>          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>>>>>      }
>>>>> +
>>>>> +    /// Access the additional data stored in this registration.
>>>>> +    pub fn data(&self) -> &T::RegistrationData {
>>>>> +        // SAFETY:
>>>>> +        // * No mutable reference to the value contained by `self.data` can ever be created.
>>>>> +        // * The value contained by `self.data` is valid for the entire lifetime of `&self`.
>>>>
>>>> Please add type invariants for these two requirements.
>>>>
>>>>> +        unsafe { &*self.data.get() }
>>>>> +    }
>>>>>  }
>>>>>  
>>>>>  #[pinned_drop]
>>>>> -impl<T> PinnedDrop for MiscDeviceRegistration<T> {
>>>>> +impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
>>>>>      fn drop(self: Pin<&mut Self>) {
>>>>>          // SAFETY: We know that the device is registered by the type invariants.
>>>>>          unsafe { bindings::misc_deregister(self.inner.get()) };
>>>>> +
>>>>> +        // SAFETY: `self.data` is valid for dropping and nothing uses it anymore.
>>>>
>>>> Ditto.
>>>
>>> I'm not quite sure how to formulate these, what do you think of:
>>>
>>> /// - `inner` is a registered misc device.
>> 
>> This doesn't really mean something to me, maybe it's better to reference
>> the registering function?
>
> That is from previous code so this should probably not be changed
> in this series.

I personally wouldn't mind a commit that fixes this up, but if you don't
want to do it, let me know then we can make this a good-first-issue.

>>> /// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
>> 
>> This sounds good. But help me understand, why do we need `Opaque` /
>> `UnsafePinned` again? If we're only using shared references, then we
>> could also just store the object by value?
>
> Since the Module owns the `MiscDeviceRegistration` it may create `&mut MiscDeviceRegistration`,
> so from what I understand having a `& RegistrationData` reference into that is UB without
> `UnsafePinned` (or `Opaque` since that includes `UnsafePinned` semantics).

And the stored `T::RegistrationData` is shared as read-only with the C
side? Yes in that case we want `UnsafePinned<UnsafeCell<>>` (or for the
moment `Opaque`).

>>> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
>> 
>> What does "usable" mean?
>
> I guess valid / alive might be better wording?
>
> I meant to say that the `fops` functions might use the `RegistrationData` until 
> `misc_deregister` has returned so we must ensure that these accesses are allowed.  

Then use `valid`.

>>> /// - no mutable references to `data` may be created.
>> 
>>>>> +        unsafe { core::ptr::drop_in_place(self.data.get()) };
>>>>>      }
>>>>>  }
>>>>>  
>>>>> @@ -109,6 +135,13 @@ pub trait MiscDevice: Sized {
>>>>>      /// What kind of pointer should `Self` be wrapped in.
>>>>>      type Ptr: ForeignOwnable + Send + Sync;
>>>>>  
>>>>> +    /// The additional data carried by the [`MiscDeviceRegistration`] for this [`MiscDevice`].
>>>>> +    /// If no additional data is required than the unit type `()` should be used.
>>>>> +    ///
>>>>> +    /// This data can be accessed in [`MiscDevice::open()`] using
>>>>> +    /// [`MiscDeviceRegistration::data()`].
>>>>> +    type RegistrationData: Sync;
>>>>
>>>> Why do we require `Sync` here?
>>>
>>> Needed for `MiscDeviceRegistration` to be `Send`, see response above.
>> 
>> You could also just ask the type there to be `Sync`, then users will get
>> an error when they try to use `MiscDevice` in a way where
>> `RegistrationData` is required to be `Sync`.
>
> I don't think there is any point to allow defining a `MiscDevice` implementation
> that cant actually be used/registered.

Sure, but the bound asserting that it is `Sync` doesn't need to be here,
having it just on the `impl Sync for MiscDeviceRegistration` is good
enough. (though one could argue that people would get an earlier error
if it is already asserted here. I think we should have some general
guidelines here :)

>>>> We might want to give this a shorter name?
>>>
>>> I think its fine, but I am open to Ideas.
>> 
>> `Data`?
>
> I feel that `Data` is just very ambiguous, especially since it is associated with 
> `MiscDevice` not the `MiscDeviceRegistration` in which its used.

But it is the data of the MiscDevice, no?

> One Idea I've had was `AssociatedData` but that's less clear and not much shorter
> than `RegistrationData`.

Of the two, I'd prefer `RegistrationData`.

> But I'd be alright to just with `Data` if that is wanted.

If you think that `RegistrationData` is more clear then go with that.
But I honestly don't derive much meaning from that over just `Data`. You
can still of course mention in the docs that this data is stored in the
registration.

But since there is no other way to associate data to a `MiscDevice`, I
think it makes sense to call it `Data`.

---
Cheers,
Benno

Powered by blists - more mailing lists