Message-ID: <D99SR99KSGLV.3TUC5AEKPHJHL@proton.me>
Date: Fri, 18 Apr 2025 13:25:47 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Christian Schrefl <chrisi.schrefl@...il.com>, 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>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC] rust: add UnsafePinned type

On Fri Apr 18, 2025 at 2:21 PM CEST, Christian Schrefl wrote:
> `UnsafePinned<T>` is useful for cases where a value might be shared with C
> code but not directly used by it. In particular this is added for
> storing additional data in the `MiscDeviceRegistration` which will be
> shared between `fops->open` and the containing struct.
>
> Similar to `Opaque` but guarantees that the value is always initialized
> and that the inner value is dropped when `UnsafePinned` is dropped.
>
> This was originally proposed for the IRQ abstractions [0] and is also
> useful for other where the inner data may be aliased, but is always valid
> and automatic `Drop` is desired.
>
> Since then the `UnsafePinned` type was added to upstream Rust [1] as a
> unstable feature, therefore this patch implements the subset required
> for additional data in `MiscDeviceRegistration` on older rust versions
> and using the upstream type on new rust versions which include this
> feature.
>
> Some differences to the upstream type definition are required in the
> kernel implementation, because upstream type uses some compiler changes
> to opt out of certain optimizations, this is documented in a comment on
> the `UnsafePinned` type.
>
> The documentation on is based on the upstream rust documentation with
> minor modifications.
>
> Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
> Link: https://github.com/rust-lang/rust/pull/137043 [1]
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@...il.com>

This probably is another patch by itself, but we should use
`UnsafePinned` in `Opaque` instead of re-implementing it there. Feel
free to open an issue, send a patch or let me open an issue.

> ---
> This patch is mostly to show how the upstream `UnsafePinned`
> Rust type can be used once it is stable.
>
> It is probalby not desired to use the unstable feature before
> that time.

Yeah, we don't want to introduce new unstable features where possible.
If this one is stabilized quickly, then we should add it, but if not,
then we just keep the manual implementation.

>
> To test using the upsteam implementation a fairly new nightly
> rust version is required.
>
> Tested with rustc 1.88.0-nightly (78f2104e3 2025-04-16) and
> rustc 1.78.0 (9b00956e5 2024-04-29).
> ---
>  init/Kconfig                       |  3 ++
>  rust/kernel/lib.rs                 |  1 +
>  rust/kernel/types.rs               | 34 ++++++++++++++
>  rust/kernel/types/unsafe_pinned.rs | 90 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 128 insertions(+)

> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 9d0471afc9648f2973235488b441eb109069adb1..c4e234d5c07168295499c2a8fccc70e00e83e7ca 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -253,6 +253,9 @@ fn drop(&mut self) {
>  ///
>  /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>  ///
> +/// In cases where the contained data is only used by Rust, is not allowed to be
> +/// uninitialized and automatic [`Drop`] is desired [`UnsafePinned`] should be used instead.
> +///
>  /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
>  /// It gets rid of all the usual assumptions that Rust has for a value:
>  ///
> @@ -578,3 +581,34 @@ pub enum Either<L, R> {
>  /// [`NotThreadSafe`]: type@...ThreadSafe
>  #[allow(non_upper_case_globals)]
>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
> +
> +// When available use the upstream `UnsafePinned` type
> +#[cfg(CONFIG_RUSTC_HAS_UNSAFE_PINNED)]
> +pub use core::pin::UnsafePinned;
> +
> +// Otherwise us the kernel implementation of `UnsafePinned`
> +#[cfg(not(CONFIG_RUSTC_HAS_UNSAFE_PINNED))]
> +mod unsafe_pinned;
> +#[cfg(not(CONFIG_RUSTC_HAS_UNSAFE_PINNED))]
> +pub use unsafe_pinned::UnsafePinned;
> +
> +/// Trait for creating a [`PinInit`]ialized wrapper containing `T`.
> +// Needs to be defined in kernel crate to get around the Orphan Rule when upstream `UnsafePinned`
> +// is used.
> +pub trait TryPinInitWrapper<T: ?Sized> {
> +    /// Create an [`Self`] pin-initializer which contains `T`
> +    fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E>;
> +}
> +impl<T: ?Sized> TryPinInitWrapper<T> for UnsafePinned<T> {
> +    fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        // SAFETY:
> +        //   - In case of an error in `value` the error is returned, otherwise `slot` is fully
> +        //     initialized, since `self.value` is initialized and `_pin` is a zero sized type.
> +        //   - The `Pin` invariants of `self.value` are upheld, since no moving occurs.
> +        unsafe {
> +            pin_init::pin_init_from_closure(move |slot| {
> +                value.__pinned_init(Self::raw_get_mut(slot))
> +            })
> +        }
> +    }
> +}

I'd prefer the impl for `core::pin::UnsafePinned` to exist in
`pin-init`. Do you mind creating a PR on github [1]? If not, just send a
patch and I will upstream it.

[1]: https://github.com/Rust-for-Linux/pin-init

---
Cheers,
Benno

Powered by blists - more mailing lists