[PATCH v2 1/3] rust: add UnsafePinned type

[PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

`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
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.

Link: https://lore.kernel.org/rust-for-linux/CAH5fLgiOASgjoYKFz6kWwzLaH07DqP2ph+3YyCDh2+gYqGpABA@mail.gmail.com [0]
Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/types.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 2bbaab83b9d65da667a07e85b3c89c7fa881b53c..3c2f6ac62d161f1187b5e7ade86689eec667ff4d 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:
 ///
@@ -573,3 +576,57 @@ pub enum Either<L, R> {
 /// [`NotThreadSafe`]: type@NotThreadSafe
 #[allow(non_upper_case_globals)]
 pub const NotThreadSafe: NotThreadSafe = PhantomData;
+
+/// Stores a value that may be used from multiple mutable pointers.
+///
+/// `UnsafePinned` gets rid of some of the usual assumptions that Rust has for a value:
+/// - The value is allowed to be mutated, when a `&UnsafePinned<T>` exists on the Rust side.
+/// - No uniqueness for mutable references: it is fine to have multiple `&mut UnsafePinned<T>`
+///   point to the same value.
+///
+/// To avoid the ability to use [`core::mem::swap`] this still needs to be used through a
+/// [`core::pin::Pin`] reference.
+///
+/// This is useful for cases where a value might be shared with C code
+/// but not interpreted by it or in cases where it can not always be guaranteed that the
+/// references are unique.
+///
+/// This is similar to [`Opaque<T>`] but is guaranteed to always contain valid data and will
+/// call the [`Drop`] implementation of `T` when dropped.
+#[repr(transparent)]
+pub struct UnsafePinned<T> {
+    value: UnsafeCell<T>,
+    _pin: PhantomPinned,
+}
+
+impl<T> UnsafePinned<T> {
+    /// Creates a new [`UnsafePinned`] value.
+    pub const fn new(value: T) -> Self {
+        Self {
+            value: UnsafeCell::new(value),
+            _pin: PhantomPinned,
+        }
+    }
+
+    /// Create an [`UnsafePinned`] pin-initializer from the given pin-initializer.
+    pub 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 { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }
+    }
+
+    /// Returns a raw pointer to the contained data.
+    pub const fn get(&self) -> *mut T {
+        UnsafeCell::get(&self.value).cast::<T>()
+    }
+
+    /// Gets the value behind `this`.
+    ///
+    /// This function is useful to get access to the value without creating intermediate
+    /// references.
+    pub const fn raw_get(this: *const Self) -> *mut T {
+        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
+    }
+}

-- 
2.48.1

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

On Fri Jan 31, 2025 at 4:08 PM CET, Christian Schrefl wrote:
> @@ -573,3 +576,57 @@ pub enum Either<L, R> {
>  /// [`NotThreadSafe`]: type@NotThreadSafe
>  #[allow(non_upper_case_globals)]
>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
> +
> +/// Stores a value that may be used from multiple mutable pointers.
> +///
> +/// `UnsafePinned` gets rid of some of the usual assumptions that Rust has for a value:
> +/// - The value is allowed to be mutated, when a `&UnsafePinned<T>` exists on the Rust side.
> +/// - No uniqueness for mutable references: it is fine to have multiple `&mut UnsafePinned<T>`
> +///   point to the same value.

We have another patch series [1] in transit that changes the wording on
the `Opaque<T>` type. I think we should mirror the same wording here.

[1]: https://lore.kernel.org/rust-for-linux/20250305053438.1532397-2-dirk.behme@de.bosch.com/

> +///
> +/// To avoid the ability to use [`core::mem::swap`] this still needs to be used through a

IIRC typing out the whole path makes it also appear in the docs. I think
we should just use `mem::swap` and omit `core` of course you need to
add this to make it work:
    
    /// [`mem::swap`]: core::mem::swap

> +/// [`core::pin::Pin`] reference.

Here, I would link to `Pin`, but say "[pinned](core::pin::Pin) pointer."
instead, since eg `Pin<Box<T>>` also is okay to use.

> +///
> +/// This is useful for cases where a value might be shared with C code
> +/// but not interpreted by it or in cases where it can not always be guaranteed that the
> +/// references are unique.
> +///
> +/// This is similar to [`Opaque<T>`] but is guaranteed to always contain valid data and will
> +/// call the [`Drop`] implementation of `T` when dropped.
> +#[repr(transparent)]
> +pub struct UnsafePinned<T> {
> +    value: UnsafeCell<T>,
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T> UnsafePinned<T> {
> +    /// Creates a new [`UnsafePinned`] value.
> +    pub const fn new(value: T) -> Self {
> +        Self {
> +            value: UnsafeCell::new(value),
> +            _pin: PhantomPinned,
> +        }
> +    }
> +
> +    /// Create an [`UnsafePinned`] pin-initializer from the given pin-initializer.
> +    pub 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 { init::pin_init_from_closure(move |slot| value.__pinned_init(Self::raw_get(slot))) }

Ah this is a bit suboptimal, but I guess there currently isn't a better
way to do this. I'll add it to my list of things to improve with
pin-init.

> +    }
> +
> +    /// Returns a raw pointer to the contained data.
> +    pub const fn get(&self) -> *mut T {
> +        UnsafeCell::get(&self.value).cast::<T>()
> +    }
> +
> +    /// Gets the value behind `this`.
> +    ///
> +    /// This function is useful to get access to the value without creating intermediate
> +    /// references.
> +    pub const fn raw_get(this: *const Self) -> *mut T {
> +        UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()

Why the cast to `MaybeUninit<T>`? I think this can just be:

    UnsafeCell::raw_get(&raw const this.value)

---
Cheers,
Benno

> +    }
> +}

[PATCH v2 0/3] rust: add `UnsafePinned` type

[Christian Schrefl]

This version now only has the kernel implementation without the
config flag for using the upstream version. Additionally now
commits for using `UnsafePinned` in `Opaque` were added.

Checkpatch warns about `rust/kernel/types/unsafe_pinned.rs`
missing a MAINTAINERS entry, I don't think that is necessary since it
will be part of the `RUST` entry anyways (from what I understand).

Once this has had some time to review I'll rebase my `miscdevice`
patches [0] on top of this.

This patchset depends on the `pin-init` sync for v6.16 [1].

Link: https://lore.kernel.org/rust-for-linux/20250131-b4-rust_miscdevice_registrationdata-v2-0-588f1e6cfabe@gmail.com/ [0]
Link: https://lore.kernel.org/rust-for-linux/20250421221728.528089-1-benno.lossin@proton.me [1]

Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
Changes in v2:
- Expanded `UnsafePinned` documentation to describe differences
    with upstream rust implementation.
- Removed config flag for using upstream `UnsafePinned` type.
- Add patch implementing `Wrapper` for `Opaque`
- Add patch for using `UnsafePinned` internally in `Opaque`
- Link to v1: https://lore.kernel.org/r/20250418-rust_unsafe_pinned-v1-1-c4c7558399f8@gmail.com

---
Christian Schrefl (3):
      rust: add UnsafePinned type
      rust: implement `Wrapper<T>` for `Opaque<T>`
      rust: use `UnsafePinned` in the implementation of `Opaque`

 init/Kconfig                       |   3 +
 rust/kernel/lib.rs                 |   1 +
 rust/kernel/revocable.rs           |   2 +
 rust/kernel/types.rs               |  42 +++++++-------
 rust/kernel/types/unsafe_pinned.rs | 115 +++++++++++++++++++++++++++++++++++++
 5 files changed, 143 insertions(+), 20 deletions(-)
---
base-commit: 39051adb070432b283e6c11b2b24937281b9f97f
change-id: 20250418-rust_unsafe_pinned-891dce27418d
prerequisite-message-id: <20250421221728.528089-1-benno.lossin@proton.me>
prerequisite-patch-id: dcf79c049766e66eda0377b225bb441edefcdfe4
prerequisite-patch-id: 0a078ba4989327e90317f882fa42a387bb7594a0
prerequisite-patch-id: d5182c6fc3e3b2f255001334b5da9d5c5b7b29ed
prerequisite-patch-id: 016806607094f0f403000ec455db9ea79e538bf6
prerequisite-patch-id: 368bea523f3b3e14f1dd599343475e0b166fad37
prerequisite-patch-id: 526054a8a8871d1dd5f376b7ab6aa5542b481b70
prerequisite-patch-id: a691b331bc3200287413e6423b8cc3e9cbb177e0
prerequisite-patch-id: 5afb38f41b18408fef4d3abc5750c09a67b2d34e

Best regards,
-- 
Christian Schrefl <chrisi.schrefl@gmail.com>

[PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

`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@google.com>
Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Co-developed-by: Sky <sky@sky9.dev>
Signed-off-by: Sky <sky@sky9.dev>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.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@NotThreadSafe
 #[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).
+// 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.
+    ///
+    /// [`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
+    }
+}
+
+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) }
+    }
+}

-- 
2.49.0

[PATCH v2 2/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Christian Schrefl]

Moves the implementation for `pin-init` from an associated function
to the trait function of the `Wrapper` trait and extends the
implementation to support pin-initializers with error types.

This allows to declare functions that are generic over `Wrapper`
types.

Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
`pin-init` function. This is currently the only usage in the kernel.

Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/revocable.rs |  2 ++
 rust/kernel/types.rs     | 25 +++++++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index 1e5a9d25c21b279b01f90b02997492aa4880d84f..4db68ea2207ebafcc09d082fdc1e281f31846a38 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -5,6 +5,8 @@
 //! The [`Revocable`] type wraps other types and allows access to them to be revoked. The existence
 //! of a [`RevocableGuard`] ensures that objects remain valid.
 
+use pin_init::Wrapper;
+
 use crate::{bindings, prelude::*, sync::rcu, types::Opaque};
 use core::{
     marker::PhantomData,
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 705f420fdfbc4a576de1c4546578f2f04cdf615e..f06e8720e012102e5c41e79fd97b0607e927d71c 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -9,7 +9,7 @@
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
-use pin_init::{PinInit, Zeroable};
+use pin_init::{PinInit, Wrapper, Zeroable};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -332,17 +332,6 @@ pub const fn uninit() -> Self {
         }
     }
 
-    /// Create an opaque pin-initializer from the given pin-initializer.
-    pub fn pin_init(slot: impl PinInit<T>) -> impl PinInit<Self> {
-        Self::ffi_init(|ptr: *mut T| {
-            // SAFETY:
-            //   - `ptr` is a valid pointer to uninitialized memory,
-            //   - `slot` is not accessed on error; the call is infallible,
-            //   - `slot` is pinned in memory.
-            let _ = unsafe { PinInit::<T>::__pinned_init(slot, ptr) };
-        })
-    }
-
     /// Creates a pin-initializer from the given initializer closure.
     ///
     /// The returned initializer calls the given closure with the pointer to the inner `T` of this
@@ -393,6 +382,18 @@ pub const fn raw_get(this: *const Self) -> *mut T {
         UnsafeCell::raw_get(this.cast::<UnsafeCell<MaybeUninit<T>>>()).cast::<T>()
     }
 }
+impl<T> Wrapper<T> for Opaque<T> {
+    /// Create an opaque pin-initializer from the given pin-initializer.
+    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        Self::try_ffi_init(|ptr: *mut T| {
+            // SAFETY:
+            //   - `ptr` is a valid pointer to uninitialized memory,
+            //   - `slot` is not accessed on error; the call is infallible,
+            //   - `slot` is pinned in memory.
+            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
+        })
+    }
+}
 
 /// Types that are _always_ reference counted.
 ///

-- 
2.49.0

[PATCH v2 3/3] rust: use `UnsafePinned` in the implementation of `Opaque`

[Christian Schrefl]

This change makes the semantics of the `Opaque` implementation
clearer and prepares for the switch to the upstream rust UnsafePinned`
type in the future.

`Opaque` still uses `UnsafeCell` even though the kernel implementation
of `UnsafePinned` already includes it, since the current upstream
version does not.

Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
---
 rust/kernel/types.rs | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index f06e8720e012102e5c41e79fd97b0607e927d71c..44d96423a8a6c358bb7ebf12c24fad98e5c2cb61 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -4,12 +4,12 @@
 
 use core::{
     cell::UnsafeCell,
-    marker::{PhantomData, PhantomPinned},
+    marker::PhantomData,
     mem::{ManuallyDrop, MaybeUninit},
     ops::{Deref, DerefMut},
     ptr::NonNull,
 };
-use pin_init::{PinInit, Wrapper, Zeroable};
+use pin_init::{cast_pin_init, PinInit, Wrapper, Zeroable};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -308,8 +308,7 @@ fn drop(&mut self) {
 /// ```
 #[repr(transparent)]
 pub struct Opaque<T> {
-    value: UnsafeCell<MaybeUninit<T>>,
-    _pin: PhantomPinned,
+    value: UnsafePinned<UnsafeCell<MaybeUninit<T>>>,
 }
 
 // SAFETY: `Opaque<T>` allows the inner value to be any bit pattern, including all zeros.
@@ -319,16 +318,14 @@ impl<T> Opaque<T> {
     /// Creates a new opaque value.
     pub const fn new(value: T) -> Self {
         Self {
-            value: UnsafeCell::new(MaybeUninit::new(value)),
-            _pin: PhantomPinned,
+            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::new(value))),
         }
     }
 
     /// Creates an uninitialised value.
     pub const fn uninit() -> Self {
         Self {
-            value: UnsafeCell::new(MaybeUninit::uninit()),
-            _pin: PhantomPinned,
+            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::uninit())),
         }
     }
 
@@ -371,7 +368,7 @@ pub fn try_ffi_init<E>(
 
     /// Returns a raw pointer to the opaque data.
     pub const fn get(&self) -> *mut T {
-        UnsafeCell::get(&self.value).cast::<T>()
+        UnsafeCell::raw_get(self.value.get()).cast::<T>()
     }
 
     /// Gets the value behind `this`.
@@ -384,14 +381,12 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 }
 impl<T> Wrapper<T> for Opaque<T> {
     /// Create an opaque pin-initializer from the given pin-initializer.
-    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
-        Self::try_ffi_init(|ptr: *mut T| {
-            // SAFETY:
-            //   - `ptr` is a valid pointer to uninitialized memory,
-            //   - `slot` is not accessed on error; the call is infallible,
-            //   - `slot` is pinned in memory.
-            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
-        })
+    fn pin_init<E>(value_init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        let value_init =
+            UnsafePinned::pin_init(UnsafeCell::pin_init(MaybeUninit::pin_init(value_init)));
+        // SAFETY: `Opaque<T>` is a `repr(transparent)` wrapper around
+        // `UnsafePinned<UnsafeCell<MabeUninit<T>>>` so the memory representation is compatible.
+        unsafe { cast_pin_init(value_init) }
     }
 }
 

-- 
2.49.0

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Alice Ryhl]

On Wed, Apr 30, 2025 at 10:36:11AM +0200, 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@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 3/3] rust: use `UnsafePinned` in the implementation of `Opaque`

[Alice Ryhl]

On Wed, Apr 30, 2025 at 10:36:13AM +0200, Christian Schrefl wrote:
> This change makes the semantics of the `Opaque` implementation
> clearer and prepares for the switch to the upstream rust UnsafePinned`
> type in the future.
> 
> `Opaque` still uses `UnsafeCell` even though the kernel implementation
> of `UnsafePinned` already includes it, since the current upstream
> version does not.
> 
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Alice Ryhl]

On Wed, Apr 30, 2025 at 10:36:11AM +0200, 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@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

> +config RUSTC_HAS_UNSAFE_PINNED
> +	def_bool RUSTC_VERSION >= 108800

> +#![cfg_attr(CONFIG_RUSTC_HAS_UNSAFE_PINNED, feature(unsafe_pinned))]

Actually, I missed this on the first look. Where is this feature used?
You only have a re-implementation right now.

Alice

Re: [PATCH v2 2/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Alice Ryhl]

On Wed, Apr 30, 2025 at 10:36:12AM +0200, Christian Schrefl wrote:
> Moves the implementation for `pin-init` from an associated function
> to the trait function of the `Wrapper` trait and extends the
> implementation to support pin-initializers with error types.
> 
> This allows to declare functions that are generic over `Wrapper`
> types.
> 
> Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
> `pin-init` function. This is currently the only usage in the kernel.
> 
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 2/3] rust: implement `Wrapper<T>` for `Opaque<T>`

[Benno Lossin]

On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
> Moves the implementation for `pin-init` from an associated function
> to the trait function of the `Wrapper` trait and extends the
> implementation to support pin-initializers with error types.
>
> This allows to declare functions that are generic over `Wrapper`
> types.
>
> Adds a use for the `Wrapper` trait in `revocable.rs`, to use the new
> `pin-init` function. This is currently the only usage in the kernel.
>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/revocable.rs |  2 ++
>  rust/kernel/types.rs     | 25 +++++++++++++------------
>  2 files changed, 15 insertions(+), 12 deletions(-)

Re: [PATCH v2 3/3] rust: use `UnsafePinned` in the implementation of `Opaque`

[Benno Lossin]

On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
> This change makes the semantics of the `Opaque` implementation
> clearer and prepares for the switch to the upstream rust UnsafePinned`
> type in the future.

s/This change makes/Make/
s/prepares/prepare/
s/UnsafePinned`/`UnsafePinned`/

> `Opaque` still uses `UnsafeCell` even though the kernel implementation
> of `UnsafePinned` already includes it, since the current upstream
> version does not.

This would be very important to add as a comment on the `value` field.

> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>

The change itself looks good. With the comment added:

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/types.rs | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

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@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.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@NotThreadSafe
>  #[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.

> +// 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.

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

> +    ///
> +    /// [`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?

---
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) }
> +    }
> +}

Re: [PATCH v2 0/3] rust: add `UnsafePinned` type

[Benno Lossin]

On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
> This version now only has the kernel implementation without the
> config flag for using the upstream version. Additionally now
> commits for using `UnsafePinned` in `Opaque` were added.
>
> Checkpatch warns about `rust/kernel/types/unsafe_pinned.rs`
> missing a MAINTAINERS entry, I don't think that is necessary since it
> will be part of the `RUST` entry anyways (from what I understand).

That is correct, it will be part of RUST. AFAIK checkpatch always warns
when a new file is added, but I could be wrong.

---
Cheers,
Benno

> Once this has had some time to review I'll rebase my `miscdevice`
> patches [0] on top of this.
>
> This patchset depends on the `pin-init` sync for v6.16 [1].
>
> Link: https://lore.kernel.org/rust-for-linux/20250131-b4-rust_miscdevice_registrationdata-v2-0-588f1e6cfabe@gmail.com/ [0]
> Link: https://lore.kernel.org/rust-for-linux/20250421221728.528089-1-benno.lossin@proton.me [1]
>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
> Changes in v2:
> - Expanded `UnsafePinned` documentation to describe differences
>     with upstream rust implementation.
> - Removed config flag for using upstream `UnsafePinned` type.
> - Add patch implementing `Wrapper` for `Opaque`
> - Add patch for using `UnsafePinned` internally in `Opaque`
> - Link to v1: https://lore.kernel.org/r/20250418-rust_unsafe_pinned-v1-1-c4c7558399f8@gmail.com
>
> ---
> Christian Schrefl (3):
>       rust: add UnsafePinned type
>       rust: implement `Wrapper<T>` for `Opaque<T>`
>       rust: use `UnsafePinned` in the implementation of `Opaque`
>
>  init/Kconfig                       |   3 +
>  rust/kernel/lib.rs                 |   1 +
>  rust/kernel/revocable.rs           |   2 +
>  rust/kernel/types.rs               |  42 +++++++-------
>  rust/kernel/types/unsafe_pinned.rs | 115 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 143 insertions(+), 20 deletions(-)
> ---
> base-commit: 39051adb070432b283e6c11b2b24937281b9f97f
> change-id: 20250418-rust_unsafe_pinned-891dce27418d
> prerequisite-message-id: <20250421221728.528089-1-benno.lossin@proton.me>
> prerequisite-patch-id: dcf79c049766e66eda0377b225bb441edefcdfe4
> prerequisite-patch-id: 0a078ba4989327e90317f882fa42a387bb7594a0
> prerequisite-patch-id: d5182c6fc3e3b2f255001334b5da9d5c5b7b29ed
> prerequisite-patch-id: 016806607094f0f403000ec455db9ea79e538bf6
> prerequisite-patch-id: 368bea523f3b3e14f1dd599343475e0b166fad37
> prerequisite-patch-id: 526054a8a8871d1dd5f376b7ab6aa5542b481b70
> prerequisite-patch-id: a691b331bc3200287413e6423b8cc3e9cbb177e0
> prerequisite-patch-id: 5afb38f41b18408fef4d3abc5750c09a67b2d34e
>
> Best regards,

Re: [PATCH v2 3/3] rust: use `UnsafePinned` in the implementation of `Opaque`

[Boqun Feng]

[Cc Ralf]

On Wed, Apr 30, 2025 at 10:36:13AM +0200, Christian Schrefl wrote:
> This change makes the semantics of the `Opaque` implementation
> clearer and prepares for the switch to the upstream rust UnsafePinned`
> type in the future.
> 
> `Opaque` still uses `UnsafeCell` even though the kernel implementation
> of `UnsafePinned` already includes it, since the current upstream
> version does not.
> 
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> ---
>  rust/kernel/types.rs | 29 ++++++++++++-----------------
>  1 file changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index f06e8720e012102e5c41e79fd97b0607e927d71c..44d96423a8a6c358bb7ebf12c24fad98e5c2cb61 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -4,12 +4,12 @@
>  
>  use core::{
>      cell::UnsafeCell,
> -    marker::{PhantomData, PhantomPinned},
> +    marker::PhantomData,
>      mem::{ManuallyDrop, MaybeUninit},
>      ops::{Deref, DerefMut},
>      ptr::NonNull,
>  };
> -use pin_init::{PinInit, Wrapper, Zeroable};
> +use pin_init::{cast_pin_init, PinInit, Wrapper, Zeroable};
>  
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>  ///
> @@ -308,8 +308,7 @@ fn drop(&mut self) {
>  /// ```
>  #[repr(transparent)]
>  pub struct Opaque<T> {
> -    value: UnsafeCell<MaybeUninit<T>>,
> -    _pin: PhantomPinned,
> +    value: UnsafePinned<UnsafeCell<MaybeUninit<T>>>,

What's the Rust upstream opinion on `&UnsafePinned` vs `&UnsafeCell`?
Does `&UnsafePinned` provide the same noalias behavior as `&UnsafeCell`?

I'm wondering whether we should just do:

    pub struct Opaque<T> {
        value: UnsafePinned<MaybeUninit<T>>,
        _not_sync: PhantomData<UnsafeCell<()>>,
    }

, instead.

Regards,
Boqun

>  }
>  
>  // SAFETY: `Opaque<T>` allows the inner value to be any bit pattern, including all zeros.
> @@ -319,16 +318,14 @@ impl<T> Opaque<T> {
>      /// Creates a new opaque value.
>      pub const fn new(value: T) -> Self {
>          Self {
> -            value: UnsafeCell::new(MaybeUninit::new(value)),
> -            _pin: PhantomPinned,
> +            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::new(value))),
>          }
>      }
>  
>      /// Creates an uninitialised value.
>      pub const fn uninit() -> Self {
>          Self {
> -            value: UnsafeCell::new(MaybeUninit::uninit()),
> -            _pin: PhantomPinned,
> +            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::uninit())),
>          }
>      }
>  
> @@ -371,7 +368,7 @@ pub fn try_ffi_init<E>(
>  
>      /// Returns a raw pointer to the opaque data.
>      pub const fn get(&self) -> *mut T {
> -        UnsafeCell::get(&self.value).cast::<T>()
> +        UnsafeCell::raw_get(self.value.get()).cast::<T>()
>      }
>  
>      /// Gets the value behind `this`.
> @@ -384,14 +381,12 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  }
>  impl<T> Wrapper<T> for Opaque<T> {
>      /// Create an opaque pin-initializer from the given pin-initializer.
> -    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> -        Self::try_ffi_init(|ptr: *mut T| {
> -            // SAFETY:
> -            //   - `ptr` is a valid pointer to uninitialized memory,
> -            //   - `slot` is not accessed on error; the call is infallible,
> -            //   - `slot` is pinned in memory.
> -            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
> -        })
> +    fn pin_init<E>(value_init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
> +        let value_init =
> +            UnsafePinned::pin_init(UnsafeCell::pin_init(MaybeUninit::pin_init(value_init)));
> +        // SAFETY: `Opaque<T>` is a `repr(transparent)` wrapper around
> +        // `UnsafePinned<UnsafeCell<MabeUninit<T>>>` so the memory representation is compatible.
> +        unsafe { cast_pin_init(value_init) }
>      }
>  }
>  
> 
> -- 
> 2.49.0
> 
> 

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

On 30.04.25 11:19 AM, Alice Ryhl wrote:
> On Wed, Apr 30, 2025 at 10:36:11AM +0200, 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@google.com>
>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
>> Co-developed-by: Sky <sky@sky9.dev>
>> Signed-off-by: Sky <sky@sky9.dev>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
> 
>> +config RUSTC_HAS_UNSAFE_PINNED
>> +	def_bool RUSTC_VERSION >= 108800
> 
>> +#![cfg_attr(CONFIG_RUSTC_HAS_UNSAFE_PINNED, feature(unsafe_pinned))]
> 
> Actually, I missed this on the first look. Where is this feature used?
> You only have a re-implementation right now.
Was just a leftover from the previous version, removing the feature and
config flag (to be reintroduced once we want to use the upstream version).

Cheers
Christian

Re: [PATCH v2 3/3] rust: use `UnsafePinned` in the implementation of `Opaque`

[Christian Schrefl]

On 30.04.25 6:44 PM, Boqun Feng wrote:
> [Cc Ralf]
> 
> On Wed, Apr 30, 2025 at 10:36:13AM +0200, Christian Schrefl wrote:
>> This change makes the semantics of the `Opaque` implementation
>> clearer and prepares for the switch to the upstream rust UnsafePinned`
>> type in the future.
>>
>> `Opaque` still uses `UnsafeCell` even though the kernel implementation
>> of `UnsafePinned` already includes it, since the current upstream
>> version does not.
>>
>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.com>
>> ---
>>  rust/kernel/types.rs | 29 ++++++++++++-----------------
>>  1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index f06e8720e012102e5c41e79fd97b0607e927d71c..44d96423a8a6c358bb7ebf12c24fad98e5c2cb61 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -4,12 +4,12 @@
>>  
>>  use core::{
>>      cell::UnsafeCell,
>> -    marker::{PhantomData, PhantomPinned},
>> +    marker::PhantomData,
>>      mem::{ManuallyDrop, MaybeUninit},
>>      ops::{Deref, DerefMut},
>>      ptr::NonNull,
>>  };
>> -use pin_init::{PinInit, Wrapper, Zeroable};
>> +use pin_init::{cast_pin_init, PinInit, Wrapper, Zeroable};
>>  
>>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>>  ///
>> @@ -308,8 +308,7 @@ fn drop(&mut self) {
>>  /// ```
>>  #[repr(transparent)]
>>  pub struct Opaque<T> {
>> -    value: UnsafeCell<MaybeUninit<T>>,
>> -    _pin: PhantomPinned,
>> +    value: UnsafePinned<UnsafeCell<MaybeUninit<T>>>,
> 
> What's the Rust upstream opinion on `&UnsafePinned` vs `&UnsafeCell`?
> Does `&UnsafePinned` provide the same noalias behavior as `&UnsafeCell`?

From the upsteam rust docs [0]:
> Further note that this does not lift the requirement that shared
references must be read-only! Use `UnsafeCell` for that.

I at some point there was discussion about possibly needing to use
`UnsafeCell` internally (because of `pin::deref`). But since the
current upstream documentation explicitly mentions still needing
`UnsafeCell` I assume that it will stay this was. If the upstream
implementation changes I can add a patch that removes the
unnecessary `UnsafeCell` and changes the documentation.

I asked about this on the `UnsafePinned` rust tracking issue [1].

Link: https://doc.rust-lang.org/nightly/std/pin/struct.UnsafePinned.html [0]
Link: https://github.com/rust-lang/rust/issues/125735#issuecomment-2842668816 [1]

Cheers
Christian

> I'm wondering whether we should just do:
> 
>     pub struct Opaque<T> {
>         value: UnsafePinned<MaybeUninit<T>>,
>         _not_sync: PhantomData<UnsafeCell<()>>,
>     }
> 
> , instead.
> 
> Regards,
> Boqun
> 
>>  }
>>  
>>  // SAFETY: `Opaque<T>` allows the inner value to be any bit pattern, including all zeros.
>> @@ -319,16 +318,14 @@ impl<T> Opaque<T> {
>>      /// Creates a new opaque value.
>>      pub const fn new(value: T) -> Self {
>>          Self {
>> -            value: UnsafeCell::new(MaybeUninit::new(value)),
>> -            _pin: PhantomPinned,
>> +            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::new(value))),
>>          }
>>      }
>>  
>>      /// Creates an uninitialised value.
>>      pub const fn uninit() -> Self {
>>          Self {
>> -            value: UnsafeCell::new(MaybeUninit::uninit()),
>> -            _pin: PhantomPinned,
>> +            value: UnsafePinned::new(UnsafeCell::new(MaybeUninit::uninit())),
>>          }
>>      }
>>  
>> @@ -371,7 +368,7 @@ pub fn try_ffi_init<E>(
>>  
>>      /// Returns a raw pointer to the opaque data.
>>      pub const fn get(&self) -> *mut T {
>> -        UnsafeCell::get(&self.value).cast::<T>()
>> +        UnsafeCell::raw_get(self.value.get()).cast::<T>()
>>      }
>>  
>>      /// Gets the value behind `this`.
>> @@ -384,14 +381,12 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>>  }
>>  impl<T> Wrapper<T> for Opaque<T> {
>>      /// Create an opaque pin-initializer from the given pin-initializer.
>> -    fn pin_init<E>(slot: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> -        Self::try_ffi_init(|ptr: *mut T| {
>> -            // SAFETY:
>> -            //   - `ptr` is a valid pointer to uninitialized memory,
>> -            //   - `slot` is not accessed on error; the call is infallible,
>> -            //   - `slot` is pinned in memory.
>> -            unsafe { PinInit::<T, E>::__pinned_init(slot, ptr) }
>> -        })
>> +    fn pin_init<E>(value_init: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        let value_init =
>> +            UnsafePinned::pin_init(UnsafeCell::pin_init(MaybeUninit::pin_init(value_init)));
>> +        // SAFETY: `Opaque<T>` is a `repr(transparent)` wrapper around
>> +        // `UnsafePinned<UnsafeCell<MabeUninit<T>>>` so the memory representation is compatible.
>> +        unsafe { cast_pin_init(value_init) }
>>      }
>>  }
>>  
>>
>> -- 
>> 2.49.0
>>
>>

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

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@google.com>
>> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
>> Co-developed-by: Sky <sky@sky9.dev>
>> Signed-off-by: Sky <sky@sky9.dev>
>> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.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@NotThreadSafe
>>  #[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) }
>> +    }
>> +}
> 

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

On 30.04.25 10:36 AM, 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@google.com>
> Reviewed-by: Gerald Wisböck <gerald.wisboeck@feather.ink>
> Co-developed-by: Sky <sky@sky9.dev>
> Signed-off-by: Sky <sky@sky9.dev>
> Signed-off-by: Christian Schrefl <chrisi.schrefl@gmail.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@NotThreadSafe
>  #[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.

[CC Ralf]

Ralf has replied to me on Github that this will most likely change [0]. How should this be handled?

I would fine with submitting a patch once it changes on the rust side (possibly waiting until the
feature is close to stabilization). I think it is better to only add this guarantee later as it
will be easier to remove unnecessary `UnsafeCell`s than it would be to later add them back in ever
case where they would be needed (in case rust doesn't change `UnsafePinned` to act like `UnsafeCell`).

Also see to the tracking issue [1] for the reason why `UnsafeCell` behavior is most likely required.

[0]: https://github.com/rust-lang/rust/issues/125735#issuecomment-2842926832
[1]: https://github.com/rust-lang/rust/issues/137750

Cheers
Christian

> +///
> +/// 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).
> +// 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.
> +    ///
> +    /// [`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
> +    }
> +}
> +
> +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) }
> +    }
> +}
> 

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
> On 30.04.25 11:45 AM, Benno Lossin wrote:
>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>> +/// 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.

I think we shouldn't talk about these implementation details in the
docs.

>>> +// 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.

Sure.

>>> +    /// 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.

Then I'd remove the sentence referencing the functions you don't add.

---
Cheers,
Benno

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

On Thu May 1, 2025 at 7:12 PM CEST, Christian Schrefl wrote:
> On 30.04.25 10:36 AM, Christian Schrefl wrote:
>> +/// 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.
>
> [CC Ralf]
>
> Ralf has replied to me on Github that this will most likely change [0]. How should this be handled?
>
> I would fine with submitting a patch once it changes on the rust side (possibly waiting until the
> feature is close to stabilization). I think it is better to only add this guarantee later as it
> will be easier to remove unnecessary `UnsafeCell`s than it would be to later add them back in ever
> case where they would be needed (in case rust doesn't change `UnsafePinned` to act like `UnsafeCell`).

Agreed, unless Ralf suggests a different way, we should do it like this.

---
Cheers,
Benno

> Also see to the tracking issue [1] for the reason why `UnsafeCell` behavior is most likely required.
>
> [0]: https://github.com/rust-lang/rust/issues/125735#issuecomment-2842926832
> [1]: https://github.com/rust-lang/rust/issues/137750
>
> Cheers
> Christian

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

On 01.05.25 8:51 PM, Benno Lossin wrote:
> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
>> On 30.04.25 11:45 AM, Benno Lossin wrote:
>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>>> +/// 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.
> 
> I think we shouldn't talk about these implementation details in the
> docs.

Alright, what do you think of:

// 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`.
// - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
//   the upstream version however currently does not. This will most likely change in the future
//   but for now we don't expose this in the documentation, since adding the guarantee is simpler
//   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
//   must not be relied on (Other than the niche blocking).

> 
>>>> +// 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.
> 
> Sure.

I'll add the following sentence to the end of the module documentation:

For details on the difference to the upstream implementation see the
comment on the [`UnsafePinned`] struct definition.

> 
>>>> +    /// 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.
> 
> Then I'd remove the sentence referencing the functions you don't add.

Alright

Cheers
Christian

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Benno Lossin]

On Thu May 1, 2025 at 9:11 PM CEST, Christian Schrefl wrote:
> On 01.05.25 8:51 PM, Benno Lossin wrote:
>> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
>>> On 30.04.25 11:45 AM, Benno Lossin wrote:
>>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>>>> +/// 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.
>> 
>> I think we shouldn't talk about these implementation details in the
>> docs.
>
> Alright, what do you think of:
>
> // As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`

There are two '`' after PhantomPinned.

> // - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`

s/ a / an /

I find the phrasing 'avoid needing <negative impl>' a bit weird, I'd
just say "`PhantomPinned` to ensure the struct always is `!Unpin` and
thus enables the `!Unpin` hack".

If you have a link to somewhere that explains that hack, then I'd also
put it there. I forgot if it's written down somewhere.

> //   Required to use the `!Unpin hack`.
> // - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
> //   the upstream version however currently does not. This will most likely change in the future
> //   but for now we don't expose this in the documentation, since adding the guarantee is simpler
> //   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
> //   must not be relied on (Other than the niche blocking).

---
Cheers,
Benno

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Christian Schrefl]

[cc Ralf]

On 02.05.25 12:51 AM, Benno Lossin wrote:
> On Thu May 1, 2025 at 9:11 PM CEST, Christian Schrefl wrote:
>> On 01.05.25 8:51 PM, Benno Lossin wrote:
>>> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
>>>> On 30.04.25 11:45 AM, Benno Lossin wrote:
>>>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
>>>>>> +/// 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.
>>>
>>> I think we shouldn't talk about these implementation details in the
>>> docs.
>>
>> Alright, what do you think of:
>>
>> // As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
> 
> There are two '`' after PhantomPinned.
> 
>> // - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
> 
> s/ a / an /
> 
> I find the phrasing 'avoid needing <negative impl>' a bit weird, I'd
> just say "`PhantomPinned` to ensure the struct always is `!Unpin` and
> thus enables the `!Unpin` hack".

Thanks I'll use that.

> 
> If you have a link to somewhere that explains that hack, then I'd also
> put it there. I forgot if it's written down somewhere.

I haven't found anything that describes the hack in detail.
From what I understand its a combination of disabling `noalias`
[0] (this PR enables it for `Unpin` types) and disabling 
`dereferencable` [1] on `&mut !Unpin` types.
Related rust issue about this [2].

Maybe Alice, Ralf or someone else form the rust side can provide
a better reference?

[0]: https://github.com/rust-lang/rust/pull/82834
[1]: https://github.com/rust-lang/rust/pull/106180
[2]: https://github.com/rust-lang/rust/issues/63818

Cheers
Christian

>> //   Required to use the `!Unpin hack`.
>> // - In order to disable niche optimizations this implementation uses `UnsafeCell` internally,
>> //   the upstream version however currently does not. This will most likely change in the future
>> //   but for now we don't expose this in the documentation, since adding the guarantee is simpler
>> //   than removing it. Meaning that for now the fact that `UnsafePinned` contains an `UnsafeCell`
>> //   must not be relied on (Other than the niche blocking).

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Alice Ryhl]

On Fri, May 02, 2025 at 02:08:13AM +0200, Christian Schrefl wrote:
> [cc Ralf]
> 
> On 02.05.25 12:51 AM, Benno Lossin wrote:
> > On Thu May 1, 2025 at 9:11 PM CEST, Christian Schrefl wrote:
> >> On 01.05.25 8:51 PM, Benno Lossin wrote:
> >>> On Wed Apr 30, 2025 at 7:30 PM CEST, Christian Schrefl wrote:
> >>>> On 30.04.25 11:45 AM, Benno Lossin wrote:
> >>>>> On Wed Apr 30, 2025 at 10:36 AM CEST, Christian Schrefl wrote:
> >>>>>> +/// 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.
> >>>
> >>> I think we shouldn't talk about these implementation details in the
> >>> docs.
> >>
> >> Alright, what do you think of:
> >>
> >> // As opposed to the upstream Rust type this contains a `PhantomPinned`` and `UnsafeCell<T>`
> > 
> > There are two '`' after PhantomPinned.
> > 
> >> // - `PhantomPinned` to avoid needing a `impl<T> !Unpin for UnsafePinned<T>`
> > 
> > s/ a / an /
> > 
> > I find the phrasing 'avoid needing <negative impl>' a bit weird, I'd
> > just say "`PhantomPinned` to ensure the struct always is `!Unpin` and
> > thus enables the `!Unpin` hack".
> 
> Thanks I'll use that.
> 
> > 
> > If you have a link to somewhere that explains that hack, then I'd also
> > put it there. I forgot if it's written down somewhere.
> 
> I haven't found anything that describes the hack in detail.
> From what I understand its a combination of disabling `noalias`
> [0] (this PR enables it for `Unpin` types) and disabling 
> `dereferencable` [1] on `&mut !Unpin` types.
> Related rust issue about this [2].
> 
> Maybe Alice, Ralf or someone else form the rust side can provide
> a better reference?
> 
> [0]: https://github.com/rust-lang/rust/pull/82834
> [1]: https://github.com/rust-lang/rust/pull/106180
> [2]: https://github.com/rust-lang/rust/issues/63818

I wrote this a long time ago:
https://gist.github.com/Darksonn/1567538f56af1a8038ecc3c664a42462

But it doesn't really take the angle of explaining the !Unpin hack. It's
more of an early doc arguing for having an UnsafePinned type.

Alice

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Ralf Jung]

Hi all,

On 01.05.25 20:55, Benno Lossin wrote:
> On Thu May 1, 2025 at 7:12 PM CEST, Christian Schrefl wrote:
>> On 30.04.25 10:36 AM, Christian Schrefl wrote:
>>> +/// 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.
>>
>> [CC Ralf]
>>
>> Ralf has replied to me on Github that this will most likely change [0]. How should this be handled?
>>
>> I would fine with submitting a patch once it changes on the rust side (possibly waiting until the
>> feature is close to stabilization). I think it is better to only add this guarantee later as it
>> will be easier to remove unnecessary `UnsafeCell`s than it would be to later add them back in ever
>> case where they would be needed (in case rust doesn't change `UnsafePinned` to act like `UnsafeCell`).
> 
> Agreed, unless Ralf suggests a different way, we should do it like this.

Sorry, I replied only on github since that was easier to do more quickly. ;)

Yes I would recommend for now to keep the `UnsafeCell` in the kernel sources, 
until the compiler actually implements the change that removes `noalias` from 
`&UnsafePinned`. And even then you should probably keep the `UnsafeCell` when 
building for older compiler versions from before that patch (or keep it until 
you drop support for those older compiler versions).

This is not a spec-only change, codegen really has to be adjusted to make 
`&UnsafePinned` properly compatible with mutable aliasing, and I see no reason 
to risk potential problems here by prematurely removing that `UnsafeCell`.

Kind regards,
Ralf

> 
> ---
> Cheers,
> Benno
> 
>> Also see to the tracking issue [1] for the reason why `UnsafeCell` behavior is most likely required.
>>
>> [0]: https://github.com/rust-lang/rust/issues/125735#issuecomment-2842926832
>> [1]: https://github.com/rust-lang/rust/issues/137750
>>
>> Cheers
>> Christian

Re: [PATCH v2 1/3] rust: add UnsafePinned type

[Ralf Jung]

Hi all,

>> If you have a link to somewhere that explains that hack, then I'd also
>> put it there. I forgot if it's written down somewhere.
> 
> I haven't found anything that describes the hack in detail.
>  From what I understand its a combination of disabling `noalias`
> [0] (this PR enables it for `Unpin` types) and disabling
> `dereferencable` [1] on `&mut !Unpin` types.
> Related rust issue about this [2].
> 
> Maybe Alice, Ralf or someone else form the rust side can provide
> a better reference?

The `!Unpick` hack simply removes `noalias` and `dereferenceable` from `&mut 
!Unpin` and `Box<!Unpin>` types (similar to how we already remove  `noalias` and 
`dereferenceable` from `&!Freeze` types). This is not a stable guarantee and 
will probably change in future version of the compiler.

Kind regards,
Ralf