Message-ID: <d433986a-fdad-4859-b8bf-080a18158189@gmail.com>
Date: Wed, 30 Apr 2025 19:30:07 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Benno Lossin <lossin@...nel.org>, Sky <sky@...9.dev>,
 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>,
 Gerald Wisböck <gerald.wisboeck@...ther.ink>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 1/3] rust: add UnsafePinned type

On 30.04.25 11:45 AM, Benno Lossin wrote:
> On Wed Apr 30, 2025 at 10:36 AM 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] by Sky
>> as a unstable feature, therefore this patch implements the subset of the
>> upstream API for the `UnsafePinned` type required for additional data in
>> `MiscDeviceRegistration` and in the implementation of the `Opaque` type.
>>
>> 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 the
>> documentation and a comment on the `UnsafePinned` type.
>>
>> The documentation on is based on the upstream rust documentation with
>> minor modifications for the kernel implementation.
>>
>> 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>
>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@...ther.ink>
>> Co-developed-by: Sky <sky@...9.dev>
>> Signed-off-by: Sky <sky@...9.dev>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@...il.com>
>> ---
>>  init/Kconfig                       |   3 +
>>  rust/kernel/lib.rs                 |   1 +
>>  rust/kernel/types.rs               |   6 ++
>>  rust/kernel/types/unsafe_pinned.rs | 115 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 125 insertions(+)
>>
>> diff --git a/init/Kconfig b/init/Kconfig
>> index 63f5974b9fa6ea3f5c92203cedd1f2f82aa468a1..727d85d2b59f555f1c33103bb78698551a41e7ca 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..705f420fdfbc4a576de1c4546578f2f04cdf615e 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,6 @@ pub enum Either<L, R> {
>>  /// [`NotThreadSafe`]: type@...ThreadSafe
>>  #[allow(non_upper_case_globals)]
>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>> +
>> +mod unsafe_pinned;
>> +pub use unsafe_pinned::UnsafePinned;
>> diff --git a/rust/kernel/types/unsafe_pinned.rs b/rust/kernel/types/unsafe_pinned.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..5a200aac30792bf71098087aee0fd9d2d51c468f
>> --- /dev/null
>> +++ b/rust/kernel/types/unsafe_pinned.rs
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: Apache-2.0 OR MIT
>> +
>> +//! The contents of this file partially come from the Rust standard library, hosted in
>> +//! the <https://github.com/rust-lang/rust> repository, licensed under
>> +//! "Apache-2.0 OR MIT" and adapted for kernel use. For copyright details,
>> +//! see <https://github.com/rust-lang/rust/blob/master/COPYRIGHT>.
>> +//!
>> +//! This file provides a implementation / polyfill of a subset of the upstream
>> +//! rust `UnsafePinned` type.
>> +
>> +use core::{cell::UnsafeCell, marker::PhantomPinned};
>> +use pin_init::{cast_pin_init, PinInit, Wrapper};
>> +
>> +/// 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.
>> +///
>> +/// # Kernel implementation notes
>> +///
>> +/// This implementation works because of the "`!Unpin` hack" in rustc, which allows (some kinds of)
>> +/// mutual aliasing of `!Unpin` types. This hack might be removed at some point, after which only
>> +/// the `core::pin::UnsafePinned` type will allow this behavior. In order to simplify the migration
>> +/// to future rust versions only this polyfill of this type should be used when this behavior is
>> +/// required.
>> +///
>> +/// In order to disable niche optimizations this implementation uses [`UnsafeCell`] internally,
>> +/// the upstream version however will not. So the fact that [`UnsafePinned`] contains an
>> +/// [`UnsafeCell`] must not be relied on (Other than the niche blocking).
> 
> I would make this last paragraph a normal comment, I don't think we
> should expose it in the docs.

I added this as docs since I wanted it to be a bit more visible,
but I can replace the comment text (about `UnsafeCell`) with this paragraph
and drop it from the docs if you want.
 
>> +// 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>`
>> +//      Required to use the `!Unpin hack`.
>> +// - `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.
> 
> I agree with copy-pasting the docs from upstream, even though our
> implementation already wraps the value in `UnsafeCell`, but I think we
> should include a comment at the top of this doc that mentions this
> difference. So something along the lines "In order to make replacing
> this type with the upstream one, we want to have as little API
> divergence as possible. Thus we don't mention the implementation detail
> of `UnsafeCell` and people have to use `UnsafePinned<UnsafeCell<T>>`
> instead of just `UnsafePinned<T>`." feel free to modify.
> 

I already wrote about this in comments (and documentation in this version)
on the `UnsafePinned` type definition.

I'm not sure where exactly we want to have this, but I think having it
at the top of the file and on the type definition is a bit redundant.

>
> If we add that, maybe we don't need the comment about needing
> `UnsafeCell` in `Opaque` after all (from my other mail).

I think I should still add a comment there.

> 
>> +    ///
>> +    /// [`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.
> 
> You did not include the `get_mut_{pinned,unchecked}` methods, so
> mentioning them here in the docs might confuse people. Do we want to
> have those methods?

I only included the functions that we needed for `Opaque` and my
`miscdevice' patches. I think these functions should only be added
once they have a user. That's why I wrote the next sentence in the
documents.

Should I handle this differently?

It should be a really simple patch to add these functions and I can
do that if someone needs them or I can just include them in this
patch set.

Cheers
Christian

> 
> ---
> Cheers,
> Benno
> 
>> +    ///
>> +    /// 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
>> +    }
>> +}
>> +
>> +impl<T> Wrapper<T> for UnsafePinned<T> {
>> +    fn pin_init<E>(init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        // SAFETY: `UnsafePinned<T>` has a compatible layout to `T`.
>> +        unsafe { cast_pin_init(init) }
>> +    }
>> +}
> 

Powered by blists - more mailing lists