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

["Benno Lossin" <lossin@kernel.org>]

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

> +///
> +/// # 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>`]."? And then ask in
`Owned::from_raw` for a pointer that is valid indefinitely (or at least
until `release` 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.

I feel like this requirement is better put on the `Owned::from_raw`
function.

> +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`."?

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