Message-Id: <DAACCYW3QRQE.1O75L2SHJYVPM@kernel.org>
Date: Sat, 31 May 2025 14:23:23 +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 Fri May 30, 2025 at 10:46 PM CEST, Christian Schrefl wrote:
> @@ -45,32 +46,46 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>  /// # Invariants
>  ///
>  /// `inner` is a registered misc device.
> -#[repr(transparent)]
> +#[repr(C)]

Why do we need linear layout? `container_of!` also works with the `Rust`
layout.

>  #[pin_data(PinnedDrop)]
> -pub struct MiscDeviceRegistration<T> {
> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>      #[pin]
>      inner: Opaque<bindings::miscdevice>,
> +    #[pin]
> +    data: Opaque<T::RegistrationData>,
>      _t: PhantomData<T>,

No need to keep the `PhantomData` field around, since you're using `T`
above.

>  }
>  
> -// SAFETY: It is allowed to call `misc_deregister` on a different thread from where you called
> -// `misc_register`.
> -unsafe impl<T> Send for MiscDeviceRegistration<T> {}
> -// SAFETY: All `&self` methods on this type are written to ensure that it is safe to call them in
> -// parallel.
> -unsafe impl<T> Sync for MiscDeviceRegistration<T> {}
> +// SAFETY:
> +// - It is allowed to call `misc_deregister` on a different thread from where you called
> +//   `misc_register`.
> +// - Only implements `Send` if `MiscDevice::RegistrationData` is also `Send`.
> +unsafe impl<T: MiscDevice> Send for MiscDeviceRegistration<T> where T::RegistrationData: Send {}
> +
> +// 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.

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

> +        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?

We might want to give this a shorter name?

---
Cheers,
Benno

> +
>      /// Called when the misc device is opened.
>      ///
>      /// The returned pointer will be stored as the private data for the file.

Powered by blists - more mailing lists