Message-ID: <3eef5777-9190-4782-8433-7b6ad4b9acd3@gmail.com>
Date: Mon, 2 Jun 2025 23:16:33 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Benno Lossin <lossin@...nel.org>, 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 31.05.25 2:23 PM, Benno Lossin wrote:
> 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.

That was a leftover from a previous version, fixed.

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

Fixed.

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

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.

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?
> 
>>  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.
/// - `data` contains a valid `T::RegistrationData` for the whole lifetime of [`MiscDeviceRegistration`]
/// - `data` must be usable until `misc_deregister` (called when dropped) has returned.
/// - 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.

> 
> We might want to give this a shorter name?

I think its fine, but I am open to Ideas.

Cheers
Christian

Powered by blists - more mailing lists