Message-Id: <DB1IPFNLFDWV.2V5O73DOB2RV6@kernel.org>
Date: Wed, 02 Jul 2025 13:03:45 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Oliver Mangold" <oliver.mangold@...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>, "Asahi Lina"
 <lina+kernel@...hilina.net>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v11 1/4] rust: types: Add Ownable/Owned types

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> From: Asahi Lina <lina+kernel@...hilina.net>
>
> By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
> (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
> `AlwaysRefCounted`, this mechanism expects the reference to be unique
> within Rust, and does not allow cloning.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> Signed-off-by: Asahi Lina <lina@...hilina.net>
> [ om:
>   - split code into separate file and `pub use` it from types.rs
>   - make from_raw() and into_raw() public
>   - fixes to documentation and commit message
> ]
> Signed-off-by: Oliver Mangold <oliver.mangold@...me>
> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> ---
>  rust/kernel/types.rs         |   7 +++
>  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++

I think we should name this file `owned.rs` instead. It's also what
we'll have for `ARef` when that is moved to `sync/`.

Also, I do wonder does this really belong into the `types` module? I
feel like it's becoming our `utils` module and while it does fit, I
think we should just make this a top level module. So
`rust/kernel/owned.rs`. Thoughts?

>  2 files changed, 141 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,6 +11,9 @@
>  };
>  use pin_init::{PinInit, Zeroable};
>  
> +pub mod ownable;
> +pub use ownable::{Ownable, OwnableMut, Owned};
> +
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>  ///
>  /// Ownership is transferred from Rust to a foreign language by calling [`Self::into_foreign`] and
> @@ -425,6 +428,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>  /// instances of a type.
>  ///
> +/// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
> +/// internal reference count and provides only shared references. If unique references are required
> +/// [`Ownable`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
> +///
>  /// # Safety
>  ///
>  /// Implementers must ensure that increments to the reference count keep the object alive in memory
> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..f4065a0d627a62d3ecb15edabf306e9b812556e1
> --- /dev/null
> +++ b/rust/kernel/types/ownable.rs
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Owned reference types.

I think it's a good idea to expand the docs here a bit.

> +
> +use core::{
> +    marker::PhantomData,
> +    mem::ManuallyDrop,
> +    ops::{Deref, DerefMut},
> +    ptr::NonNull,
> +};
> +
> +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.

This seems wrong, `var: Owned<T>` should life until `min(var, T)`, so
whatever is earlier: until the user drops the `var` or `T`'s lifetime
ends.

How about we just say:

    Type allocated and destroyed on the C side, but owned by Rust.

> +///
> +/// It allows such types to define their own custom destructor function to be called when a
> +/// Rust-owned reference is dropped.

We shouldn't call this a reference. Also we should start the first
paragraph with how this trait enables the usage of `Owned<Self>`.

> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> +///
> +/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> +/// provide reference counting but represents a unique, owned reference. If reference counting is
> +/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
> +/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).

I think this is more confusing than helpful. We should mention
`AlwaysRefCounted`, but the phrasing needs to be changed. Something
along the lines: if you need reference counting, implement
`AlwaysRefCounted` instead.

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> +///   kernel is case it frees the object, etc.).

This invariant sounds weird to me. It's vague "a state which the kernel
expects" and difficult to use (what needs this invariant?).

> +pub unsafe trait Ownable {
> +    /// Releases the object (frees it or returns it to foreign ownership).

Let's remove the part in parenthesis.

> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - `this` points to a valid `Self`.
> +    /// - The object is no longer referenced after this call.

s/The object/`*this`/

s/referenced/used/

> +    unsafe fn release(this: NonNull<Self>);
> +}
> +
> +/// Type where [`Owned<Self>`] derefs to `&mut Self`.

How about:

    Type allowing mutable access via [`Owned<Self>`].

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that access to a `&mut T` is safe, implying that:

s/T/Self/

> +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> +///   (i.e. most kernel types).

Can't we implicitly pin `Owned`?

> +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> +///   the usual rules) while Rust owns it.
> +pub unsafe trait OwnableMut: Ownable {}
> +
> +/// An owned reference to an ownable kernel object.

How about
    
    An owned `T`.

> +///
> +/// The object is automatically freed or released when an instance of [`Owned`] is
> +/// dropped.

I don't think we need to say this, I always assume this for all Rust
types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
and thus also `Opaque`.)

How about we provide some examples here?

> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.

What exactly is "owned" supposed to mean? It depends on the concrete `T`
and that isn't well-defined (since it's a generic)...

Maybe we should give `Ownable` the task to document the exact ownership
semantics of `T`?

> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +    _p: PhantomData<T>,
> +}
> +
> +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).

How does this amount to sending a `&mut T`?

I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
list grows...

I'll try to come up with something to simplify this design a bit wrt the
safety docs.

> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).

Same here.

> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new instance of [`Owned`].
> +    ///
> +    /// It takes over ownership of the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - Ownership of the underlying object can be transferred to the `Owned<T>` (i.e. operations
> +    ///   which require ownership will be safe).
> +    /// - No other Rust references to the underlying object exist. This implies that the underlying
> +    ///   object is not accessed through `ptr` anymore after the function call (at least until the
> +    ///   the `Owned<T>` is dropped).
> +    /// - The C code follows the usual shared reference requirements. That is, the kernel will never
> +    ///   mutate or free the underlying object (excluding interior mutability that follows the usual
> +    ///   rules) while Rust owns it.
> +    /// - In case `T` implements [`OwnableMut`] the previous requirement is extended from shared to
> +    ///   mutable reference requirements. That is, the kernel will not mutate or free the underlying
> +    ///   object and is okay with it being modified by Rust code.
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> +        // reference.

This doesn't follow for me. Well the first issue is that the safety
invariant of `Self` isn't well-defined, so let's revisit this when that
is fixed.

---
Cheers,
Benno

> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    /// Consumes the [`Owned`], returning a raw pointer.
> +    ///
> +    /// This function does not actually relinquish ownership of the object. After calling this
> +    /// function, the caller is responsible for ownership previously managed
> +    /// by the [`Owned`].
> +    pub fn into_raw(me: Self) -> NonNull<T> {
> +        ManuallyDrop::new(me).ptr
> +    }
> +}
> +
> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: OwnableMut> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> +        // return a mutable reference to it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: The type invariants guarantee that the `Owned` owns the object we're about to
> +        // release.
> +        unsafe { T::release(self.ptr) };
> +    }
> +}

Powered by blists - more mailing lists