Re: [PATCH v18 2/8] rust: types: Add Ownable/Owned types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `Owned<T>` unconditionally structurally pins `T` by exposing `Pin<&mut T>`, but its API lacks the necessary documentation and safety bounds to enforce the pinning invariants. Specifically, `Owned::from_raw(ptr)` takes ownership without requiring the caller to promise the memory is or can be pinned; `Owned::into_raw(me)` safely extracts the pointer without documenting that the returned pointer's referent is still pinned; and `Ownable::release(this)` doesn't forbid the implementer from moving the pinned `Self` before dropping it. This omission allows an `Ownable` implementer or a user correctly following the explicit safety contracts to accidentally move a pinned `!Unpin` type, resulting in undefined behavior.
--

commit b7329ea2c0525ffa6a63171ede0921ad2e6a23e5
Author: Asahi Lina <lina+kernel@asahilina.net>

rust: types: Add Ownable/Owned types

This commit introduces Ownable and Owned types for unique owned pointers
with custom drop logic. These types allow C-allocated objects owned by Rust
to be freed through the C API. It also updates documentation and adds usage
examples.

Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/

> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> new file mode 100644
> index 0000000000000..7fe9ec3e55126
> --- /dev/null
> +++ b/rust/kernel/owned.rs
[ ... ]
> +pub trait Ownable {
> +    /// Tear down this `Ownable`.
> +    ///
> +    /// Implementers of `Ownable` can use this function to clean up the use of `Self`. This can
> +    /// include freeing the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that they have exclusive ownership of the `Self` pointed to by `this`,
> +    /// and that this ownership is transferred to the `release` method. `this` must not be used
> +    /// after calling this method, as the underlying object may have been freed.
> +    unsafe fn release(this: NonNull<Self>);

[Severity: High]
Since Owned<T> treats T as pinned (exposing Pin<&mut T> via as_pin_mut()),
should the safety contract here also require that implementers treat `this`
as pointing to pinned memory?

If an implementer moves the value during cleanup (for example, by reading it
onto the stack), it could violate the pinning guarantees for !Unpin types and
cause memory corruption due to self-reference invalidation.

> +}
[ ... ]
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new instance of [`Owned`].
> +    ///
> +    /// This function takes over ownership of the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - `ptr` points to a valid instance of `T`.
> +    /// - Until `T::release` is called, the returned `Owned<T>` exclusively owns the underlying `T`.
> +    #[inline]
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {

[Severity: High]
Does this safety contract need to require that the pointer is permanently
pinned?

Because Owned<T> can expose a pinned mutable reference, callers should be
aware that they cannot assume the memory remains unpinned after calling this
function.

> +        // INVARIANT: By function safety requirement we satisfy the first invariant of `Self`.
> +        // We treat `T` as pinned from now on.
> +        Self { ptr }
> +    }
> +
> +    /// 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.
> +    #[inline]
> +    pub fn into_raw(me: Self) -> NonNull<T> {

[Severity: High]
Since into_raw() safely strips the Owned wrapper, should there be
documentation stating that the caller remains bound by the pin invariant
established by Owned?

Without this, a caller might extract the pointer and safely move the
previously pinned object, resulting in undefined behavior.

> +        ManuallyDrop::new(me).ptr
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-unique-ref-v18-0-4e06b5896d47@kernel.org?part=2