Message-Id: <A5A7C4C9-1504-439C-B4FF-C28482AF7444@collabora.com>
Date: Fri, 28 Nov 2025 15:06:42 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Oliver Mangold <oliver.mangold@...me>
Cc: Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Benno Lossin <lossin@...nel.org>,
 Danilo Krummrich <dakr@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Dave Ertman <david.m.ertman@...el.com>,
 Ira Weiny <ira.weiny@...el.com>,
 Leon Romanovsky <leon@...nel.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>,
 Jan Kara <jack@...e.cz>,
 Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 Viresh Kumar <vireshk@...nel.org>,
 Nishanth Menon <nm@...com>,
 Stephen Boyd <sboyd@...nel.org>,
 Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 Paul Moore <paul@...l-moore.com>,
 Serge Hallyn <sergeh@...nel.org>,
 Asahi Lina <lina+kernel@...hilina.net>,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 linux-block@...r.kernel.org,
 dri-devel@...ts.freedesktop.org,
 linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org,
 linux-pm@...r.kernel.org,
 linux-pci@...r.kernel.org,
 linux-security-module@...r.kernel.org
Subject: Re: [PATCH v13 4/4] rust: Add `OwnableRefCounted`

Hi Oliver,

> On 17 Nov 2025, at 07:08, Oliver Mangold <oliver.mangold@...me> wrote:
> 
> Types implementing one of these traits can safely convert between an
> `ARef<T>` and an `Owned<T>`.
> 
> This is useful for types which generally are accessed through an `ARef`
> but have methods which can only safely be called when the reference is
> unique, like e.g. `block::mq::Request::end_ok()`.
> 
> Signed-off-by: Oliver Mangold <oliver.mangold@...me>
> Co-developed-by: Andreas Hindborg <a.hindborg@...nel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
> Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
> ---
> rust/kernel/owned.rs     | 138 ++++++++++++++++++++++++++++++++++++++++++++---
> rust/kernel/sync/aref.rs |  11 +++-
> rust/kernel/types.rs     |   2 +-
> 3 files changed, 141 insertions(+), 10 deletions(-)
> 
> diff --git a/rust/kernel/owned.rs b/rust/kernel/owned.rs
> index a26747cbc13b..26ab2b00ada0 100644
> --- a/rust/kernel/owned.rs
> +++ b/rust/kernel/owned.rs
> @@ -5,6 +5,7 @@
> //! These pointer types are useful for C-allocated objects which by API-contract
> //! are owned by Rust, but need to be freed through the C API.
> 
> +use crate::sync::aref::{ARef, RefCounted};
> use core::{
>     mem::ManuallyDrop,
>     ops::{Deref, DerefMut},
> @@ -14,14 +15,16 @@
> 
> /// Type allocated and destroyed on the C side, but owned by Rust.
> ///
> -/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type. This
> -/// is useful when it is desirable to tie the lifetime of the reference to an owned object, rather
> -/// than pass around a bare reference. [`Ownable`] types can define custom drop logic that is
> -/// executed when the owned reference [`Owned<Self>`] pointing to the object is dropped.
> +/// Implementing this trait allows types to be referenced via the [`Owned<Self>`] pointer type.
> +///  - This is useful when it is desirable to tie the lifetime of an object reference to an owned
> +///    object, rather than pass around a bare reference.
> +///  - [`Ownable`] types can define custom drop logic that is executed when the owned reference
> +///    of type [`Owned<_>`] pointing to the object is dropped.
> ///
> /// Note: The underlying object is not required to provide internal reference counting, because it
> /// represents a unique, owned reference. If reference counting (on the Rust side) is required,
> -/// [`RefCounted`](crate::types::RefCounted) should be implemented.
> +/// [`RefCounted`] should be implemented. [`OwnableRefCounted`] should be implemented if conversion
> +/// between unique and shared (reference counted) ownership is needed.
> ///
> /// # Safety
> ///
> @@ -143,9 +146,7 @@ impl<T: Ownable> Owned<T> {
>     ///   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.
>     pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> -        Self {
> -            ptr,
> -        }
> +        Self { ptr }
>     }

Unrelated change?

> 
>     /// Consumes the [`Owned`], returning a raw pointer.
> @@ -193,3 +194,124 @@ fn drop(&mut self) {
>         unsafe { T::release(self.ptr) };
>     }
> }
> +
> +/// A trait for objects that can be wrapped in either one of the reference types [`Owned`] and
> +/// [`ARef`].
> +///
> +/// # Examples
> +///
> +/// A minimal example implementation of [`OwnableRefCounted`], [`Ownable`] and its usage with
> +/// [`ARef`] and [`Owned`] looks like this:
> +///
> +/// ```
> +/// # #![expect(clippy::disallowed_names)]
> +/// # use core::cell::Cell;
> +/// # use core::ptr::NonNull;
> +/// # use kernel::alloc::{flags, kbox::KBox, AllocError};
> +/// # use kernel::sync::aref::{ARef, RefCounted};
> +/// # use kernel::types::{Owned, Ownable, OwnableRefCounted};
> +///
> +/// // Example internally refcounted struct.

nit: IMHO the wording could improve ^

> +/// //
> +/// // # Invariants
> +/// //
> +/// // - `refcount` is always non-zero for a valid object.
> +/// // - `refcount` is >1 if there are more than 1 Rust reference to it.
> +/// //
> +/// struct Foo {
> +///     refcount: Cell<usize>,
> +/// }
> +///
> +/// impl Foo {
> +///     fn new() -> Result<Owned<Self>, AllocError> {
> +///         // We are just using a `KBox` here to handle the actual allocation, as our `Foo` is
> +///         // not actually a C-allocated object.
> +///         let result = KBox::new(
> +///             Foo {
> +///                 refcount: Cell::new(1),
> +///             },
> +///             flags::GFP_KERNEL,
> +///         )?;
> +///         let result = NonNull::new(KBox::into_raw(result))
> +///             .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
> +///         // SAFETY: We just allocated the `Self`, thus it is valid and there cannot be any other
> +///         // Rust references. Calling `into_raw()` makes us responsible for ownership and
> +///         // we won't use the raw pointer anymore, thus we can transfer ownership to the `Owned`.
> +///         Ok(unsafe { Owned::from_raw(result) })
> +///     }
> +/// }
> +///
> +/// // SAFETY: We increment and decrement each time the respective function is called and only free
> +/// // the `Foo` when the refcount reaches zero.
> +/// unsafe impl RefCounted for Foo {
> +///     fn inc_ref(&self) {
> +///         self.refcount.replace(self.refcount.get() + 1);
> +///     }
> +///
> +///     unsafe fn dec_ref(this: NonNull<Self>) {
> +///         // SAFETY: By requirement on calling this function, the refcount is non-zero,
> +///         // implying the underlying object is valid.
> +///         let refcount = unsafe { &this.as_ref().refcount };
> +///         let new_refcount = refcount.get() - 1;
> +///         if new_refcount == 0 {
> +///             // The `Foo` will be dropped when `KBox` goes out of scope.
> +///             // SAFETY: The [`KBox<Foo>`] is still alive as the old refcount is 1. We can pass
> +///             // ownership to the [`KBox`] as by requirement on calling this function,
> +///             // the `Self` will no longer be used by the caller.
> +///             unsafe { KBox::from_raw(this.as_ptr()) };
> +///         } else {
> +///             refcount.replace(new_refcount);
> +///         }
> +///     }
> +/// }
> +///
> +/// impl OwnableRefCounted for Foo {
> +///     fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>> {
> +///         if this.refcount.get() == 1 {
> +///             // SAFETY: The `Foo` is still alive and has no other Rust references as the refcount
> +///             // is 1.
> +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +///         } else {
> +///             Err(this)
> +///         }
> +///     }
> +/// }
> +///

We wouldn’t need this implementation if we added a “refcount()”
member to this trait. This lets you abstract away this logic for all
implementors, which has the massive upside of making sure we hardcode (and thus
enforce) the refcount == 1 check.


> +/// // SAFETY: This implementation of `release()` is safe for any valid `Self`.
> +/// unsafe impl Ownable for Foo {
> +///     unsafe fn release(this: NonNull<Self>) {
> +///         // SAFETY: Using `dec_ref()` from [`RefCounted`] to release is okay, as the refcount is
> +///         // always 1 for an [`Owned<Foo>`].
> +///         unsafe{ Foo::dec_ref(this) };
> +///     }
> +/// }
> +///
> +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");

All these “expects()” and custom error strings would go away if you
place this behind a fictional function that returns Result.

> +/// let mut foo = ARef::from(foo);
> +/// {
> +///     let bar = foo.clone();
> +///     assert!(Owned::try_from(bar).is_err());
> +/// }
> +/// assert!(Owned::try_from(foo).is_ok());
> +/// ```
> +pub trait OwnableRefCounted: RefCounted + Ownable + Sized {
> +    /// Checks if the [`ARef`] is unique and convert it to an [`Owned`] it that is that case.
> +    /// Otherwise it returns again an [`ARef`] to the same underlying object.
> +    fn try_from_shared(this: ARef<Self>) -> Result<Owned<Self>, ARef<Self>>;

Again, this method can go way if we add a method to expose the refcount.

> +
> +    /// Converts the [`Owned`] into an [`ARef`].
> +    fn into_shared(this: Owned<Self>) -> ARef<Self> {
> +        // SAFETY: Safe by the requirements on implementing the trait.
> +        unsafe { ARef::from_raw(Owned::into_raw(this)) }
> +    }
> +}
> +
> +impl<T: OwnableRefCounted> TryFrom<ARef<T>> for Owned<T> {
> +    type Error = ARef<T>;
> +    /// Tries to convert the [`ARef`] to an [`Owned`] by calling
> +    /// [`try_from_shared()`](OwnableRefCounted::try_from_shared). In case the [`ARef`] is not
> +    /// unique, it returns again an [`ARef`] to the same underlying object.
> +    fn try_from(b: ARef<T>) -> Result<Owned<T>, Self::Error> {
> +        T::try_from_shared(b)
> +    }
> +}
> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
> index 937dcf6ed5de..2dbffe2ed1b8 100644
> --- a/rust/kernel/sync/aref.rs
> +++ b/rust/kernel/sync/aref.rs
> @@ -30,7 +30,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`](crate::types::OwnableRefCounted) allows to convert between unique and
> +/// shared references (i.e. [`Owned<Self>`](crate::types::Owned) and
> +/// [`ARef<Self>`](crate::types::Owned)).
> ///
> /// # Safety
> ///
> @@ -180,6 +183,12 @@ fn from(b: &T) -> Self {
>     }
> }
> 
> +impl<T: crate::types::OwnableRefCounted> From<crate::types::Owned<T>> for ARef<T> {
> +    fn from(b: crate::types::Owned<T>) -> Self {
> +        T::into_shared(b)
> +    }
> +}
> +

Not sure why we’re using fully-qualified names here if we can import them, but your call.

> impl<T: RefCounted> Drop for ARef<T> {
>     fn drop(&mut self) {
>         // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8ef01393352b..a9b72709d0d3 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,7 +11,7 @@
> };
> use pin_init::{PinInit, Wrapper, Zeroable};
> 
> -pub use crate::owned::{Ownable, Owned};
> +pub use crate::owned::{Ownable, OwnableRefCounted, Owned};
> 
> pub use crate::sync::aref::{ARef, AlwaysRefCounted, RefCounted};
> 
> 
> -- 
> 2.51.2
> 
> 
> 

— Daniel

Powered by blists - more mailing lists