Message-Id: <DADAEIT9E1R8.1J69W5DKYAQGY@kernel.org>
Date: Wed, 04 Jun 2025 01:29:22 +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 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>();
        }
    };

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?

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

> /// - `data` must be usable until `misc_deregister` (called when dropped) has returned.

What does "usable" mean?

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

>> We might want to give this a shorter name?
>
> I think its fine, but I am open to Ideas.

`Data`?

---
Cheers,
Benno

Powered by blists - more mailing lists