Message-ID: <7f80f64a-e0f6-469f-ab2e-ff40344929b9@gmail.com>
Date: Fri, 18 Apr 2025 14:35:52 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: 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>, 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 18.04.25 2:21 PM, 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 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.
> 
> 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/init/Kconfig b/init/Kconfig
> index dd2ea3b9a799205daa4c1f0c694a9027e344c690..f34e96cd3fb8a058a83e38c2ea9cb17737f5e0b6 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -140,6 +140,9 @@ config LD_CAN_USE_KEEP_IN_OVERLAY
>  config RUSTC_HAS_COERCE_POINTEE
>  	def_bool RUSTC_VERSION >= 108400
>  
> +config RUSTC_HAS_UNSAFE_PINNED
> +	def_bool RUSTC_VERSION >= 108800
> +
>  config PAHOLE_VERSION
>  	int
>  	default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE))
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index de07aadd1ff5fe46fd89517e234b97a6590c8e93..c08f0a50f1d8db95799478caa8e85558a1fcae8d 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -17,6 +17,7 @@
>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(coerce_unsized))]
>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(dispatch_from_dyn))]
>  #![cfg_attr(not(CONFIG_RUSTC_HAS_COERCE_POINTEE), feature(unsize))]
> +#![cfg_attr(CONFIG_RUSTC_HAS_UNSAFE_PINNED, feature(unsafe_pinned))]
>  #![feature(inline_const)]
>  #![feature(lint_reasons)]
>  // Stable in Rust 1.82
> 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))
> +            })
> +        }
> +    }
> +}
> diff --git a/rust/kernel/types/unsafe_pinned.rs b/rust/kernel/types/unsafe_pinned.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..e4e8986a044e9fe9215712dc9837ecfdbd6d5176
> --- /dev/null
> +++ b/rust/kernel/types/unsafe_pinned.rs
> @@ -0,0 +1,90 @@
> +// SPDX-License-Identifier: GPL-2.0
This probably needs to be Apache-2.0 OR MIT

> +
> +//! This file provides a implementation of a subset of the upstream rust `UnsafePinned` type
> +//! for rust versions that don't include this type.
> +
> +use core::{cell::UnsafeCell, marker::PhantomPinned};
> +
> +/// This type provides a way to opt-out of typical aliasing rules;
> +/// specifically, `&mut UnsafePinned<T>` is not guaranteed to be a unique pointer.
> +///
> +/// However, even if you define your type like `pub struct Wrapper(UnsafePinned<...>)`, it is still
> +/// very risky to have an `&mut Wrapper` that aliases anything else. Many functions that work
> +/// generically on `&mut T` assume that the memory that stores `T` is uniquely owned (such as
> +/// `mem::swap`). In other words, while having aliasing with `&mut Wrapper` is not immediate
> +/// Undefined Behavior, it is still unsound to expose such a mutable reference to code you do not
> +/// control! Techniques such as pinning via [`Pin`](core::pin::Pin) are needed to ensure soundness.
> +///
> +/// Similar to [`UnsafeCell`], [`UnsafePinned`] will not usually show up in
> +/// the public API of a library. It is an internal implementation detail of libraries that need to
> +/// support aliasing mutable references.
> +///
> +/// Further note that this does *not* lift the requirement that shared references must be read-only!
> +/// Use [`UnsafeCell`] for that.
> +///
> +/// This type blocks niches the same way [`UnsafeCell`] does.
> +//
> +// As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
> +// - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
> +// - `UnsafeCell<T>` instead of T to disallow niche optimizations,
> +//     which is handled in the compiler in upstream Rust
> +#[repr(transparent)]
> +pub struct UnsafePinned<T: ?Sized> {
> +    _ph: PhantomPinned,
> +    value: UnsafeCell<T>,
> +}
> +
> +impl<T> UnsafePinned<T> {
> +    /// Constructs a new instance of [`UnsafePinned`] which will wrap the specified value.
> +    ///
> +    /// All access to the inner value through `&UnsafePinned<T>` or `&mut UnsafePinned<T>` or
> +    /// `Pin<&mut UnsafePinned<T>>` requires `unsafe` code.
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn new(value: T) -> Self {
> +        UnsafePinned {
> +            value: UnsafeCell::new(value),
> +            _ph: PhantomPinned,
> +        }
> +    }
> +}
> +impl<T: ?Sized> UnsafePinned<T> {
> +    /// Get read-only access to the contents of a shared `UnsafePinned`.
> +    ///
> +    /// Note that `&UnsafePinned<T>` is read-only if `&T` is read-only. This means that if there is
> +    /// mutation of the `T`, future reads from the `*const T` returned here are UB! Use
> +    /// [`UnsafeCell`] if you also need interior mutability.
> +    ///
> +    /// [`UnsafeCell`]: core::cell::UnsafeCell
> +    ///
> +    /// ```rust,no_build
> +    /// use kernel::types::UnsafePinned;
> +    ///
> +    /// unsafe {
> +    ///     let mut x = UnsafePinned::new(0);
> +    ///     let ptr = x.get(); // read-only pointer, assumes immutability
> +    ///     x.get_mut_unchecked().write(1);
> +    ///     ptr.read(); // UB!
> +    /// }
> +    /// ```
> +    ///
> +    /// Note that the `get_mut_unchecked` function used by this example is
> +    /// currently not implemented in the kernel implementation.
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn get(&self) -> *const T {
> +        self.value.get()
> +    }
> +
> +    /// Gets a mutable pointer to the wrapped value.
> +    ///
> +    /// The difference from `get_mut_pinned` and `get_mut_unchecked` is that this function
> +    /// accepts a raw pointer, which is useful to avoid the creation of temporary references.
> +    ///
> +    /// These functions mentioned here are currently not implemented in the kernel.
> +    #[inline(always)]
> +    #[must_use]
> +    pub const fn raw_get_mut(this: *mut Self) -> *mut T {
> +        this as *mut T
> +    }
> +}
> 
> ---
> base-commit: 0af2f6be1b4281385b618cb86ad946eded089ac8
> change-id: 20250418-rust_unsafe_pinned-891dce27418d
> 
> Best regards,

Powered by blists - more mailing lists