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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLghaE6fC0riAch7Wj94ZqNx86mi4ytBhCzcR7KCcw6ZqVQ@mail.gmail.com>
Date: Mon, 27 Jan 2025 14:33:38 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: Miguel Ojeda <ojeda@...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>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Arnd Bergmann <arnd@...db.de>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Lee Jones <lee@...nel.org>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

On Mon, Jan 27, 2025 at 2:27 PM Christian Schrefl
<chrisi.schrefl@...il.com> wrote:
>
> Hi Alice
>
> On 27.01.25 11:27 AM, Alice Ryhl wrote:
> ><snip>
> >>
> >> 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.
> >
>
> So if I understand correctly, the following should be correct:
>
> pub fn register(
>     opts: MiscDeviceOptions,
>     data: impl PinInit<T::RegistrationData, Error>,
> ) -> impl PinInit<Self, Error> {
>     try_pin_init!(Self {
>         data <- UnsafePinned::try_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.
>             // `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) })
>         }),
>         _t: PhantomData,
>     })
> }
>
> Sorry I don't know the details of `(try_)pin_init` and the docs only say:
> > The fields are initialized in the order that they appear in the initializer. So it is possible
> > to read already initialized fields using raw pointers.
> >
> > IMPORTANT: You are not allowed to create references to fields of the struct inside of the
> > initializer.
>
> This says its invalid to create references, but as soon as `misc_register` its theoretically
> possible that a `open()` call happens and creates a reference to the Registration. I assume
> that is fine, because the Registration would be fully initialized at that point, but that's
> technically against the Docs.

Creating a reference immediately after misc_register finishes should
be fine. I think that warning is more strict than necessary.

> Also does `try_pin_init!()` automatically drop `data` when `bindings::misc_register` fails?

Yep. On failure, previous fields are cleaned up.

> I couldn't find anything in the Docs about when and what is dropped.
>
> Is there a equivalent to `cargo expand` for the kernel?
> It would be nice to be able to look at the code generated by the macros.

You can expand Rust macros by invoking make on the file with an .i
extension, in exactly the same way as you expand C macros.

Alice