Re: [PATCH v11 1/4] rust: types: Add Ownable/Owned types

[Oliver Mangold <oliver.mangold@pm.me>]

On 250702 1303, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > From: Asahi Lina <lina+kernel@asahilina.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@asahilina.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@pm.me>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.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?

I don't have much of an opinion on on that. But maybe refactoring types.rs
should be an independent task?

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

Yes, I guess that sounds sloppy.

> How about we just say:
> 
>     Type allocated and destroyed on the C side, but owned by Rust.

Would be okay with me.

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

I guess you have a point. The shortened version is probably good enough.

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

Okay with all of the above.

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

I have been thinking about that. But this would mean that the blanket
implementation for `Deref` would conflict with the one for `OwnableMut`.

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

    A wrapper around `T`.

maybe?

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

Hmm, it is an important feature of the wrapper that it turns the `*Ownable`
into an object that is automatically dropped. So shouldn't that be
mentioned here?

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

"owned" means that access to the `T` is exclusive through the `Owned<T>`,
so normal Rust semantics can be applied.

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

Good point. That documentation hasn't been updated since `&mut T` access
has been split out into `OwnableMut`. Not sure how to phrase it now.

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

I guess it follows from:

> > +    /// It takes over ownership of the underlying object.

Okay, sure, it not in the safety requirement.

Best,

Oliver