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

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

On Mon Jul 7, 2025 at 8:58 AM CEST, Oliver Mangold wrote:
> 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?

But you're adding the new file there? Just add it under
`rust/kernel/owned.rs` instead.

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

Yeah we could not implement `DerefMut` in that case (or only for `T:
Unpin`).

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

"wrapper" seems wrong, since a wrapper in my mind is a newtype. So it
would be `struct Owned(T)` which is wrong. The `T` is stored elsewhere.

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

I would expect that that happens, but sure we can leave it 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.

Okay, in that case just say that `ptr` has exclusive access.

>> 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'm also unsure, also for the correct trait bounds on this impl.

---
Cheers,
Benno

>> 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> {}