Re: [PATCH v10 1/5] rust: types: Add Ownable/Owned types

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

On 250514 1132, Benno Lossin wrote:
> On Fri May 2, 2025 at 11:02 AM CEST, Oliver Mangold wrote:
> > +/// 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.
> 
> The docs should mention `AlwaysRefCounted` and when to use it instead of
> this trait. We should probably also backlink from `AlwaysRefCounted` to
> `Ownable`.

Will do.

> > +///
> > +/// # 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).
> 
> I don't immediately understand what this means. How about "Any value of
> type `Self` needs to be stored as [`Owned<Self>`]."?

Let me think. The safety requirements here talk about safety of
implementing the trait.  But if you have a `Self` which is not wrapped, you
still cannot create an `Owned<Self>` in safe code. It's different from an
`AlwaysRefCounted`, where an `ARef<Self>` can be created from a `&Self`.

> And then ask in
> `Owned::from_raw` for a pointer that is valid indefinitely (or at least
> until `release` is called).

So, hmm, I think one could even move this safety requirement to `Owned::from_raw()`.

> > +/// - 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.
> 
> I feel like this requirement is better put on the `Owned::from_raw`
> function.

Together with the above, this would leave to safety requirements for `Ownable.
Make `Ownable` a safe trait, then? Instead of safety requirements just add an invariant:

    # Invariant
    
    An `Owned<Self>` represents a unique reference to a `Self`, thus holding
    an `Owned<Self>` or `&mut Owned<Self>` allows one to assume that the object
    is not accessed concurrently from elsewhere.

Not sure what is best. Would that make sense?

> > +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`.
> 
> The "A subtrait of Ownable that asserts" sounds a bit clumsy to me, how
> about "Type where [`Owned<Self>`] derefs to `&mut Self`."?

That's okay with me.

> > +///
> > +/// # 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).
> 
> I don't like that we put this requirement here, since it's actually
> something that should be asserted by `Owned::from_raw`.
> The reason for that is that anyone can call `Owned::from_raw` with a
> pointer pointing to `Self` and there is no safety requirement on that
> function that ensures the correctness of the `DerefMut` impl.
> 
> > +pub unsafe trait OwnableMut: Ownable {}
> 
> I don't like the name, but at the same time I also have no good
> suggestion :( I'll think some more about it.
> 
> ---
> Cheers,
> Benno

Best regards,

Oliver