Message-Id: <DAA6VHUTDC13.3FLLGNXYINO9I@kernel.org>
Date: Sat, 31 May 2025 10:05:28 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: <gregkh@...uxfoundation.org>, <rafael@...nel.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <benno.lossin@...ton.me>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <chrisi.schrefl@...il.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
>> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
>> > @@ -40,44 +41,43 @@ pub const fn into_raw<T: MiscDevice>(self) -> bindings::miscdevice {
>> >      }
>> >  }
>> >  
>> > -/// A registration of a miscdevice.
>> > -///
>> >  /// # Invariants
>> >  ///
>> > -/// `inner` is a registered misc device.
>> > +/// - `inner` is a registered misc device,
>> > +/// - `data` is valid for the entire lifetime of `Self`.
>> >  #[repr(C)]
>> >  #[pin_data(PinnedDrop)]
>> > -pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > +struct RawDeviceRegistration<T: MiscDevice> {
>> >      #[pin]
>> >      inner: Opaque<bindings::miscdevice>,
>> > -    #[pin]
>> > -    data: Opaque<T::RegistrationData>,
>> > +    data: NonNull<T::RegistrationData>,
>> >      _t: PhantomData<T>,
>> 
>> You shouldn't need the `PhantomData` here.
>> 
>> Also, do we need to ask for `T: MiscDevice` here? Could we instead have
>> just `T` and then below you write
>> `RawDeviceRegistration<T::RegistrationData>` instead? (`new` of course
>> needs to have a new generic: `U: MiscDevice<RegistrationData = T>`)
>
> Sure, is there any advantage? Your proposal seems more complicated at a first
> glance.

It would make `RawDeviceRegistration` simpler, but maybe it's not worth
it.

>> >  }
>> >  
>> > -// 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> {}
>> > -
>> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
>> > -    /// Register a misc device.
>> > -    pub fn register(
>> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
>> > +    fn new<'a>(
>> >          opts: MiscDeviceOptions,
>> > -        data: impl PinInit<T::RegistrationData, Error>,
>> > -    ) -> impl PinInit<Self, Error> {
>> > +        parent: Option<&'a Device<Bound>>,
>> > +        data: &'a T::RegistrationData,
>> > +    ) -> impl PinInit<Self, Error> + 'a
>> > +    where
>> > +        T: 'a,
>> > +    {
>> >          try_pin_init!(Self {
>> > -            data <- Opaque::pin_init(data),
>> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
>> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
>> > +            data: NonNull::from(data),
>> 
>> Both the argument in the INVARIANT comment and way this works are a bit
>> flawed.
>
> Why is the argument flawed? Let's say we go with your proposal below, what would
> the safety requirement for RawDeviceRegistration::new and the invariant of
> RawDeviceRegistration look like? Wouldn't it be the exact same argument?

So what I write below it not really true. But the argument relies on the
fact that `data` is pointing to a value that will stay alive for the
duration of the existence of `self`. That should be a safety requirement
of `new` (even if we take a reference as an argument).

>> Instead, I'd recommend directly taking the `NonNull` as a
>> parameter. Yes the function will need to be `unsafe`, but the lifetime
>> that you're creating below only lives for `'a`, but the object might
>> live much longer. You might still be fine, but I'd just recommend
>> staying in raw pointer land (or in this case `NonNull`).

>> > @@ -108,6 +108,108 @@ pub fn device(&self) -> &Device {
>> >          unsafe { Device::as_ref((*self.as_raw()).this_device) }
>> >      }
>> >  
>> > +    fn data(&self) -> &T::RegistrationData {
>> > +        // SAFETY: The type invariant guarantees that `data` is valid for the entire lifetime of
>> > +        // `Self`.
>> > +        unsafe { self.data.as_ref() }
>> > +    }
>> > +}
>> > +
>> > +#[pinned_drop]
>> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<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()) };
>> > +    }
>> > +}
>> > +
>> > +#[expect(dead_code)]
>> > +enum DeviceRegistrationInner<T: MiscDevice> {
>> > +    Raw(Pin<KBox<RawDeviceRegistration<T>>>),
>> > +    Managed(Devres<RawDeviceRegistration<T>>),
>> 
>> These two names could be shortened (`DeviceRegistrationInner` and
>> `RawDeviceRegistration`) as they are only implementation details of this
>> file. How about `InnerRegistration` and `RawRegistration`? Or maybe
>> something even shorter.
>
> There's a reason why I keep them something with "DeviceRegistration" everywhere,
> which is to make it clear that it's both a device instance *and* a registration,
> which is actually rather uncommon and caused by the fact that device creation
> and registration needs to be done under the misc_mtx in misc_register().
>
> This is also the reason for those data structures to be a bit complicated; it
> would be much simpler if device creation and registration would be independent
> things.

Then keep the longer names.

>> > +}
>> > +
>> > +/// A registration of a miscdevice.
>> > +#[pin_data(PinnedDrop)]
>> > +pub struct MiscDeviceRegistration<T: MiscDevice> {
>> > +    inner: DeviceRegistrationInner<T>,
>> > +    #[pin]
>> > +    data: Opaque<T::RegistrationData>,
>> 
>> Why is it necessary to store `data` inside of `Opaque`?
>
> It was UnsafePinned before, but Alice proposed to go with Opaque for the
> meantime. Anyways, this is not introduced by this patch, it comes from
> Christians patch adding T::RegistrationData.

Ah then I'll re-read that discussion.

>> > +    this_device: ARef<Device>,
>> > +    _t: PhantomData<T>,
>> > +}

>> > +            inner: {
>> > +                // SAFETY:
>> > +                //   - `this` is a valid pointer to `Self`,
>> > +                //   - `data` was properly initialized above.
>> > +                let data = unsafe { &*(*this.as_ptr()).data.get() };
>> 
>> As mentioned above, this creates a reference that is valid for this
>> *block*. So its lifetime will end after the `},` and before
>> `this_device` is initialized.
>> 
>> It *might* be ok to turn it back into a raw pointer in
>> `RawDeviceRegistration::new`, but I wouldn't bet on it.
>
> Why? The reference is still valid in RawDeviceRegistration::new, no?

Yes the reference is still valid in the `new` function call. I was under
the impression that the pointer created in `new` would get invalidated,
because the struct would get reborrowed in the meantime, but that's not
the case, since this pointer is obtained by `get`. So you should be
good.

>> > +
>> > +                let raw = RawDeviceRegistration::new(opts, parent, data);
>> > +
>> > +                // FIXME: Work around a bug in rustc, to prevent the following warning:
>> > +                //
>> > +                //   "warning: value captured by `dev` is never read."
>> > +                //
>> > +                // Link: https://github.com/rust-lang/rust/issues/141615
>> 
>> Note that the bug is that the compiler complains about the wrong span.
>> The original value of `dev` is `None` and that value is never used, so
>> the warning is justified. So this `let _ = dev;` still needs to stay
>> until `pin-init` supports accessing previously initialized fields (now
>> I'm pretty certain that I will implement that soon).
>
> Do you want to propose an alternative comment about this?

I think we don't need a comment here.

>> > +                let _ = dev;
>> > +
>> > +                if let Some(parent) = parent {
>> > +                    let devres = Devres::new(parent, raw, GFP_KERNEL)?;
>> > +
>> > +                    dev = Some(devres.access(parent)?.device().into());
>> > +                    DeviceRegistrationInner::Managed(devres)
>> > +                } else {
>> > +                    let boxed = KBox::pin_init(raw, GFP_KERNEL)?;
>> > +
>> > +                    dev = Some(boxed.device().into());
>> > +                    DeviceRegistrationInner::Raw(boxed)
>> > +                }
>> > +            },
>> > +            // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>> > +            // case.
>> > +            this_device: {
>> > +                // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>> > +                unsafe { dev.unwrap_unchecked() }
>> > +            },
>> 
>> No need for the extra block, just do:
>> 
>>     // Cache `this_device` within `Self` to avoid having to access `Devres` in the managed
>>     // case.
>>     // SAFETY: `dev` is guaranteed to be set in the initializer of `inner` above.
>>     this_device: unsafe { dev.unwrap_unchecked() },
>
> Yes, I know, but I found the above a bit cleaner -- I don't mind changing it
> though.
>
>> I'm also pretty sure that the compiler would optimize `.take().unwrap()`
>> and also this is only executed once per `MiscDeviceRegistration`, so
>> even if it isn't it wouldn't really matter. So I'd prefer if we don't
>> use `unsafe` here even if it is painfully obvious (if I'm fast enough
>> with implementing, you can rebase on top before you merge and then this
>> will be gone anyways :)
>
> Sounds good! :)
>
> But I think that unsafe is better than unwrap() in such cases; unsafe requires
> us to explain why it's OK to do it, which makes it less likely to create bugs.

That's not exactly how I would think about it, but your argument makes
sense.

> (Just recently I wrote some code, hit the need for unsafe and, while writing up
> the safety comment, I had to explain to myself, why the way I was about to
> implement this was pretty broken.)

That's great :)

> unwrap() on the other hand, doesn't require any explanation, but panics the
> kernel in the worst case.

Yeah that is true. Ultimately for this case it won't matter, since I'll
just implement the access thing in pin-init.

It'll still take a while, because it will touch several parts that other
patches also touch. Thus I prefer to first pick the patches already on
the list, but for that I'll wait until the merge window closes.

---
Cheers,
Benno

Powered by blists - more mailing lists