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

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

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.

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

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

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

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

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

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

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

Cheers
Christian