Message-ID: <75b94302-e6ca-431d-95de-044af3061839@gmail.com>
Date: Fri, 18 Apr 2025 15:33:41 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Benno Lossin <benno.lossin@...ton.me>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC] rust: add UnsafePinned type

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

I'll take a look at it once this patch has landed if you want
or I might include that as a second Patch for a future version.

>> ---
>> This patch is mostly to show how the upstream `UnsafePinned`
>> Rust type can be used once it is stable.
>>
>> It is probalby not desired to use the unstable feature before
>> that time.
> 
> Yeah, we don't want to introduce new unstable features where possible.
> If this one is stabilized quickly, then we should add it, but if not,
> then we just keep the manual implementation.

My plan was for this RFC to demonstrate the usage of vendored and upsteam
`UnsafePinned`, with a (not RFC) patch that just adds the vendored
implementation.  

> 
>>
>> To test using the upsteam implementation a fairly new nightly
>> rust version is required.
>>
>> Tested with rustc 1.88.0-nightly (78f2104e3 2025-04-16) and
>> rustc 1.78.0 (9b00956e5 2024-04-29).
>> ---
>>  init/Kconfig                       |  3 ++
>>  rust/kernel/lib.rs                 |  1 +
>>  rust/kernel/types.rs               | 34 ++++++++++++++
>>  rust/kernel/types/unsafe_pinned.rs | 90 ++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 128 insertions(+)
> 
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 9d0471afc9648f2973235488b441eb109069adb1..c4e234d5c07168295499c2a8fccc70e00e83e7ca 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -253,6 +253,9 @@ fn drop(&mut self) {
>>  ///
>>  /// [`Opaque<T>`] is meant to be used with FFI objects that are never interpreted by Rust code.
>>  ///
>> +/// In cases where the contained data is only used by Rust, is not allowed to be
>> +/// uninitialized and automatic [`Drop`] is desired [`UnsafePinned`] should be used instead.
>> +///
>>  /// It is used to wrap structs from the C side, like for example `Opaque<bindings::mutex>`.
>>  /// It gets rid of all the usual assumptions that Rust has for a value:
>>  ///
>> @@ -578,3 +581,34 @@ pub enum Either<L, R> {
>>  /// [`NotThreadSafe`]: type@...ThreadSafe
>>  #[allow(non_upper_case_globals)]
>>  pub const NotThreadSafe: NotThreadSafe = PhantomData;
>> +
>> +// When available use the upstream `UnsafePinned` type
>> +#[cfg(CONFIG_RUSTC_HAS_UNSAFE_PINNED)]
>> +pub use core::pin::UnsafePinned;
>> +
>> +// Otherwise us the kernel implementation of `UnsafePinned`
>> +#[cfg(not(CONFIG_RUSTC_HAS_UNSAFE_PINNED))]
>> +mod unsafe_pinned;
>> +#[cfg(not(CONFIG_RUSTC_HAS_UNSAFE_PINNED))]
>> +pub use unsafe_pinned::UnsafePinned;
>> +
>> +/// Trait for creating a [`PinInit`]ialized wrapper containing `T`.
>> +// Needs to be defined in kernel crate to get around the Orphan Rule when upstream `UnsafePinned`
>> +// is used.
>> +pub trait TryPinInitWrapper<T: ?Sized> {
>> +    /// Create an [`Self`] pin-initializer which contains `T`
>> +    fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E>;
>> +}
>> +impl<T: ?Sized> TryPinInitWrapper<T> for UnsafePinned<T> {
>> +    fn try_pin_init<E>(value: impl PinInit<T, E>) -> impl PinInit<Self, E> {
>> +        // SAFETY:
>> +        //   - In case of an error in `value` the error is returned, otherwise `slot` is fully
>> +        //     initialized, since `self.value` is initialized and `_pin` is a zero sized type.
>> +        //   - The `Pin` invariants of `self.value` are upheld, since no moving occurs.
>> +        unsafe {
>> +            pin_init::pin_init_from_closure(move |slot| {
>> +                value.__pinned_init(Self::raw_get_mut(slot))
>> +            })
>> +        }
>> +    }
>> +}
> 
> I'd prefer the impl for `core::pin::UnsafePinned` to exist in
> `pin-init`. Do you mind creating a PR on github [1]? If not, just send a
> patch and I will upstream it.

It would probably also make sense to add this impl to other 
Container/Wrapper types in `pin-init`.

I'll start writing up a PR.

Cheers,
Christian

Powered by blists - more mailing lists