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

[Alice Ryhl]

On Fri, Jan 24, 2025 at 12:26 AM Christian Schrefl
<chrisi.schrefl@xxxxxxxxx> wrote:
>
> On 19.01.25 11:11 PM, Christian Schrefl wrote:
> > When using the Rust miscdevice bindings, you generally embed the
> > MiscDeviceRegistration within another struct:
> >
> > struct MyDriverData {
> >     data: SomeOtherData,
> >     misc: MiscDeviceRegistration<MyMiscFile>
> > }
> >
> > <snip>
> >
> >  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 {
> >              inner <- Opaque::try_ffi_init(move |slot: *mut bindings::miscdevice| {
> >                  // SAFETY: The initializer can write to the provided `slot`.
> > @@ -79,6 +85,7 @@ pub fn register(opts: MiscDeviceOptions) -> impl PinInit<Self, Error> {
> >                  // misc device.
> >                  to_result(unsafe { bindings::misc_register(slot) })
> >              }),
> > +            data <- Aliased::try_pin_init(data),
> >              _t: PhantomData,
> >          })
>
> After some thought I think `register` needs to be something like:
>
> pub fn register(
>     opts: MiscDeviceOptions,
>     data: impl PinInit<T::RegistrationData, Error>,
> ) -> impl PinInit<Self, Error> {
>     try_pin_init!(Self {
>         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>()) };
>             Ok::<(), Error>(())
>         }),
>         data <- UnsafePinned::try_pin_init(data),
>         _t: PhantomData,
>     })
>     .pin_chain(|slot| {
>         // 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.as_raw()) }).map_err(|err| {
>             // Drop the Data in case misc_register fails.
>             // SAFETY:
>             // - We just initialized `data`.
>             // - We are about to return `Err(err)`, so it is valid for us to drop `data`.
>             unsafe {
>                 ptr::drop_in_place(slot.data.get());
>             }
>             err
>         })
>     })
> }
>
> to make sure that `misc_register` is called after data is initialized and to that
> `data` will be dropped correctly in case `misc_register` fails.
>
> But I'm not very familiar with `(try_)pin_init!` so this might be unnecessary?

Using pin_chain is definitely incorrect because it will run the
destructor of MiscDeviceRegistration if the misc_register call fails,
but calling misc_deregister is incorrect in that case.

You should be able to just move the `data <-
UnsafePinned::try_pin_init(data)` line to the top so that the field is
initialized first.

Alice