Message-ID: <aBr7nbI2AScXSjTg@google.com>
Date: Wed, 7 May 2025 06:20:13 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: 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>, Benno Lossin <benno.lossin@...ton.me>, 
	Trevor Gross <tmgross@...ch.edu>, Asahi Lina <lina@...hilina.net>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 1/5] rust: types: Add Ownable/Owned types

On Tue, May 06, 2025 at 01:20:04PM +0200, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@...gle.com> writes:
> 
> > On Fri, May 02, 2025 at 09:02:29AM +0000, Oliver Mangold wrote:
> >> From: Asahi Lina <lina@...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
> >> ]
> >> Signed-off-by: Oliver Mangold <oliver.mangold@...me>
> >> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> >> ---
> >>  rust/kernel/types.rs         |   3 ++
> >>  rust/kernel/types/ownable.rs | 117 +++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 120 insertions(+)
> >>
> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >> index 9d0471afc9648f2973235488b441eb109069adb1..5d8a99dcba4bf733107635bf3f0c15840ec33e4c 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
> >> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..52e7a69019f1e2bbbe3cf715651b67a5a5c7c13d
> >> --- /dev/null
> >> +++ b/rust/kernel/types/ownable.rs
> >> @@ -0,0 +1,117 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +//! Owned reference types.
> >> +
> >> +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.
> >> +///
> >> +/// It allows such types to define their own custom destructor function to be called when
> >> +/// a Rust-owned reference is dropped.
> >> +///
> >> +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// Implementers must ensure that:
> >> +/// - Any objects owned by Rust as [`Owned<T>`] stay alive while that owned reference exists (i.e.
> >> +///   until the [`release()`](Ownable::release) trait method is called).
> >> +/// - That the C code follows the usual mutable reference requirements. That is, the kernel will
> >> +///   never mutate the [`Ownable`] (excluding internal mutability that follows the usual rules)
> >> +///   while Rust owns it.
> >
> > This seems too strong? Or does the exception mean to say that this does
> > not apply to anything containing `Opaque`? By far most structs using
> > this will use Opaque, so maybe directly mention Opaque instead?
> 
> `Opaque` is covered by "(excluding internal mutability that follows the usual rules)".

I guess. But I think it would still be good to rephrase to mention
Opaque for ease of understanding.

> > That C code follows the usual aliasing rules. That is, unless the value
> > is wrapped in `Opaque` (or `UnsafeCell`), then the value must not be
> > modified in any way while Rust owns it, unless that modification happens
> > inside a `&mut T` method on the value.
> >
> >> +pub unsafe trait Ownable {
> >> +    /// Releases the object (frees it or returns it to foreign ownership).
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// Callers must ensure that the object is no longer referenced after this call.
> >> +    unsafe fn release(this: NonNull<Self>);
> >> +}
> >> +
> >> +/// A subtrait of Ownable that asserts that an [`Owned<T>`] or `&mut Owned<T>` Rust reference
> >> +/// may be dereferenced into a `&mut T`.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// Implementers must ensure that access to a `&mut T` is safe, implying that it is okay to call
> >> +/// [`core::mem::swap`] on the `Ownable`. This excludes pinned types (meaning: most kernel types).
> >> +pub unsafe trait OwnableMut: Ownable {}
> >> +
> >> +/// An owned reference to an ownable kernel object.
> >> +///
> >> +/// The object is automatically freed or released when an instance of [`Owned`] is
> >> +/// dropped.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// The pointer stored in `ptr` is valid for the lifetime of the [`Owned`] instance.
> >
> > This should probably talk about ownership.
> 
> How about
> 
>   The pointee of `ptr` can be considered owned by the [`Owned`] instance.

Sure.

Alice

Powered by blists - more mailing lists