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

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

On 05.06.25 7:27 PM, Benno Lossin wrote:
> On Thu Jun 5, 2025 at 6:52 PM CEST, Christian Schrefl wrote:
>> 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:
>>>>>>>>  #[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.
> 
> Feel free to open the issue :)

I've opened [0]. I don't have the permissions to add tags for that.
[0]: https://github.com/Rust-for-Linux/linux/issues/1168

> 
>>>>>> /// - `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).
> 
> Ah yes, I meant "shared with other Rust code through the C side" ie the
> pointer round-trips through C (that isn't actually relevant, but that's
> why I mentioned C).
> 
>> 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`).
> 
> It's fine, since all non-ZST fields are `Opaque`. Otherwise we'd need to
> wrap all fields with that.

Yeah I understand that its not UB, but to me it seems a bit fragile and opaque why it is allowed.
That's what I meant by "a bit iffy".

> 
>>>>>> /// - 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 don't lose anything by doing this, so I think we should do it.
> If in the future someone invents a way `MiscDevice` that's only in the
> current thread and it can be registered (so like a "thread-local"
> `MiscDevice` :), then this will be less painful to change.

Alright but I doubt that realistic, since the `Data` would always at
least be shared between the owner of `MiscDeviceRegistration` and the
`fops` implementation. Meaning its always shared with syscall context
and I don't think it makes sense to have a registration owed in 
that context.

Cheers
Christian