Re: [PATCH v4 2/3] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl <chrisi.schrefl@gmail.com>]

On 05.06.25 6:05 PM, Benno Lossin wrote:
> 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.

I can do it, but I think it would make a good-first-issue so lets go
with that for now.

> 
>>>> /// - `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`).

Not really shared with the C side, but with the `open` implementation in
`MiscDevice` that is (indirectly) called by C. (`UnsafeCell` will probably not be
needed, as `UnsafePinned` will almost certainly have `UnsafeCell` semantics in upstream).

Thinking about this has made me realize that the current code already is a bit
iffy, since `MiscDevice::open` gets `&MiscDeviceRegistration<Self>` as an argument. (It
should be fine since `UnsafeCell` and `UnsafePinned` semantics also apply to "parrent" types
i.e. `&MiscDeviceRegistration` also has the semantics of `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`.

Alright.

> 
>>>> /// - 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 :)

That would require a `Send` bound in the `register` function,
since a `MiscDevice` with `!Sync` `Data` would be valid now
(meaning that `MiscDeviceRegistration` may also be `!Sync`).

If you want I can go with that. I'm not really sure if its
really better (tough I don't feel that strongly either
way).

> 
>>>>> 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`.
> 

Alright I'll go with `Data` then.

> ---
> Cheers,
> Benno