Re: [PATCH v18 7/8] rust: Add `OwnableRefCounted`

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `OwnableRefCounted` trait is a safe trait but its default `into_shared` method contains an `unsafe` block relying on unenforced invariants, violating Rust's soundness rules.
- [Low] Incorrect documentation link for `ARef<Self>` in `aref.rs`.
--

commit b7dc8070575155be9312b255a20879f2e905b00f
Author: Oliver Mangold <oliver.mangold@pm.me>

rust: Add `OwnableRefCounted`
    
Types implementing one of these traits can safely convert between an
`ARef<T>` and an `Owned<T>`.

> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index e79936c00002c..bb4223c0f725a 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs

[ ... ]

> +pub trait OwnableRefCounted: RefCounted + Ownable + Sized {
> +    /// Checks if the [`ARef`] is unique and converts it to an [`Owned`] if that is the case.
> +    /// Otherwise it returns again an [`ARef`] to the same underlying object.
> +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;
> +
> +    /// Converts the [`Owned`] into an [`ARef`].
> +    #[inline]
> +    fn into_shared(this: Owned<Self>) -> ARef<Self> {
> +        // SAFETY: `Owned::into_raw` returns a pointer to a valid `Self`, and the `Owned` owned the
> +        // reference count that we now transfer to the new `ARef`.
> +        unsafe { ARef::from_raw(Owned::into_raw(this)) }
> +    }
> +}

[Severity: High]
Does this trait need to be marked unsafe?

The OwnableRefCounted trait is currently safe to implement, but the default
into_shared() method contains an unsafe block that assumes the Owned<Self>
implicitly holds a reference count of 1.

If a developer implements this safe trait for a type whose Owned<Self>
does not account for one RefCounted increment, calling into_shared() will
pass a pointer to ARef::from_raw() without a corresponding reference
increment.

When the resulting ARef is dropped, it will blindly call dec_ref(), which
could lead to a reference count underflow, double-free, or use-after-free.

Should OwnableRefCounted be an unsafe trait so that implementers are
required to guarantee the reference count invariant?

> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index d0865aeb9371b..77eb390139079 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs

[ ... ]

> @@ -35,7 +39,10 @@
>  /// Note: Implementing this trait allows types to be wrapped in an [`ARef<Self>`]. It requires an
>  /// internal reference count and provides only shared references. If unique references are required
>  /// [`Ownable`](crate::types::Ownable) should be implemented which allows types to be wrapped in an
> -/// [`Owned<Self>`](crate::types::Owned).
> +/// [`Owned<Self>`](crate::types::Owned). Implementing the trait
> +/// [`OwnableRefCounted`] allows to convert between unique and
> +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and
> +/// [`ARef<Self>`](crate::types::Owned)).

[Severity: Low]
This isn't a bug, but there appears to be a copy-paste typo in the rustdoc
link.

The documentation for ARef<Self> points to crate::types::Owned instead
of ARef.

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