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

[Andreas Hindborg <a.hindborg@kernel.org>]

"Gary Guo" <gary@garyguo.net> writes:

> On Mon Feb 2, 2026 at 9:37 AM GMT, Andreas Hindborg wrote:
>> Gary Guo <gary@garyguo.net> writes:
>>
>>> On Mon, 17 Nov 2025 10:07:40 +0000
>>> Oliver Mangold <oliver.mangold@pm.me> wrote:
>>>
>>>> From: Asahi Lina <lina+kernel@asahilina.net>

<cut>

>>>> +impl<T: Ownable> Owned<T> {
>>>> +    /// Creates a new instance of [`Owned`].
>>>> +    ///
>>>> +    /// It takes over ownership of the underlying object.
>>>> +    ///
>>>> +    /// # Safety
>>>> +    ///
>>>> +    /// Callers must ensure that:
>>>> +    /// - `ptr` points to a valid instance of `T`.
>>>> +    /// - Ownership of the underlying `T` can be transferred to the `Self<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 `Self<T>` is dropped.
>>>
>>> Is this correct? If `Self<T>` is dropped then `T::release` is called so
>>> the pointer should also not be accessed further?
>>
>> I can't follow you point here. Are you saying that the requirement is
>> wrong because `T::release` will access the object by reference? If so,
>> that is part of `Owned<_>::drop`, which is explicitly mentioned in the
>> comment (until .. dropped).
>
> I meant that the `Self<T>` is dropped, the object is destroyed so it should also
> not be accessed further. Perhaps just remove the "(at least ...)" part from
> comment.

Right, got it. The "until.." is in place to allow reuse of the
allocation. There is no requirement here to drop `T` via the `release`
method when an `Owned<T>` is dropped. Implementers are free to implement
schemes that reuse the object without drop and re-init. This can be used
in object caches such as for the block request cache.

>
>>
>>>
>>>> +    /// - 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.
>>>
>>> The concept "interior mutability" doesn't really exist on the C side.
>>> Also, use of interior mutability (by UnsafeCell) would be incorrect if
>>> the type is implemented in the rust side (as this requires a
>>> UnsafePinned).
>>>
>>> Interior mutability means things can be mutated behind a shared
>>> reference -- however in this case, we have a mutable reference (either
>>> `Pin<&mut Self>` or `&mut Self`)!
>>>
>>> Perhaps together with the next line, they could be just phrased like
>>> this?
>>>
>>> - The underlying object must not be accessed (read or mutated) through
>>>   any pointer other than the created `Owned<T>`.
>>>   Opt-out is still possbile similar to a mutable reference (e.g. by
>>>   using p`Opaque`]).
>>>
>>> I think we should just tell the user "this is just a unique reference
>>> similar to &mut". They should be able to deduce that all the `!Unpin`
>>> that opts out from uniqueness of mutable reference applies here too.
>>
>> I agree. I would suggest updating the struct documentation:
>>
>>     @@ -108,7 +108,7 @@ pub unsafe trait Ownable {
>>         unsafe fn release(this: NonNull<Self>);
>>     }
>>
>>     -/// An owned reference to an owned `T`.
>>     +/// An mutable reference to an owned `T`.
>>     ///
>>     /// The [`Ownable`] is automatically freed or released when an instance of [`Owned`] is
>>     /// dropped.
>>
>> And then the safety requirement as
>>
>>  An `Owned<T>` is a mutable reference to the underlying object. As such,
>>  the object must not be accessed (read or mutated) through any pointer
>>  other than the created `Owned<T>`. Opt-out is still possbile similar to
>>  a mutable reference (e.g. by using [`Opaque`]).
>
> Sounds good to me.

OK.

>
>>
>>
>>>> +    /// - In case `T` implements [`Unpin`] 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.
>>>
>>> - If `T` implements [`Unpin`], the structure must not be mutated for
>>>   the entire lifetime of `Owned<T>`.
>>
>> Would it be OK to just write "If `T: Unpin`, the ..."?
>>
>> Again, opt out is possible, right?
>>
>
> When the "mutable reference" framing above I think you can just drop this part.

Agreed.

>
>>>
>>>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>>>
>>> This needs a (rather trivial) INVARIANT comment.
>>
>> OK.
>>
>>>
>>>> +        Self {
>>>> +            ptr,
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /// Consumes the [`Owned`], returning a raw pointer.
>>>> +    ///
>>>> +    /// This function does not actually relinquish ownership of the object. After calling this
>>>
>>> Perhaps "relinquish" isn't the best word here? In my mental model
>>> this function is pretty much relinquishing ownership as `Owned<T>` no
>>> longer exists. It just doesn't release the object.
>>
>> How about this:
>>
>>
>>     /// Consumes the [`Owned`], returning a raw pointer.
>>     ///
>>     /// This function does not drop the underlying `T`. When this function returns, ownership of the
>>     /// underlying `T` is with the caller.
>
> SGTM.

OK.


Best regards,
Andreas Hindborg