Re: [PATCH v17 02/10] rust: types: Add Ownable/Owned types

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] `Ownable::release` takes `&mut self` instead of a raw pointer, causing Undefined Behavior due to invalidating a live reference during deallocation and narrowing pointer provenance.
- [High] `Owned<T>` inherently pins its contents, but the safety contracts for `Ownable` and `from_raw` fail to enforce or document this Pin guarantee.
--

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

    rust: types: Add Ownable/Owned types
    
    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.

    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..456e239e906ef
> --- /dev/null
> +++ b/rust/kernel/owned.rs
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +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 the caller has exclusive ownership of `T`, and this ownership can
> +    /// be transferred to the `release` method.
> +    unsafe fn release(&mut self);

[Severity: High]
Does taking a mutable reference here instead of a raw pointer risk undefined
behavior?

If Owned::drop() creates a temporary reference and passes it to release(),
deallocating the backing memory of this live mutable reference violates
Rust's aliasing rules because the reference remains in scope during the free:

Owned::drop()
    T::release(self.ptr.as_mut())

Additionally, if T is an opaque C-allocated type (often represented as a
Zero-Sized Type), creating a mutable reference narrows the pointer's
provenance to exactly 0 bytes. Converting this back to a raw pointer and
passing it to a C deallocator would be invalid.

Would it be safer for release() to take a raw pointer (like NonNull<Self> or
*mut Self), similar to AlwaysRefCounted::dec_ref()?

[Severity: High]
Since Owned<T> guarantees that its underlying T is pinned and exposes
Pin<&mut T> via as_pin_mut(), does Ownable need to be an unsafe trait
to enforce pinning invariants?

Currently, Ownable is safe to implement, and release() provides an unpinned
mutable reference. A safe implementation of release() could potentially move
the value. When Owned::drop() is called, this would move a value that was
previously pinned, violating the pinning guarantees.

> +}

[ ... ]

> +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 the safety documentation for from_raw() need to require that the
allocation is permanently pinned?

Without this requirement, a caller could pass an unpinned allocation,
extract it later via into_raw(), and move it. This would violate the
internal pinning assumptions made by Owned<T> when calling as_pin_mut().

> +        // INVARIANT: By function safety requirement we satisfy the first invariant of `Self`.
> +        // We treat `T` as pinned from now on.
> +        Self { ptr }
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-unique-ref-v17-0-7b4c3d2930b9@kernel.org?part=2