[PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[Oliver Mangold]

This allows to convert between ARef<T> and Owned<T> by
implementing the new trait OwnedRefCounted.

This way we will have a shared/unique reference counting scheme
for types with built-in refcounts in analogy to Arc/UniqueArc.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
---
Changes in v11:
- Rework of documentation. I tried to honor all requests for changes "in
  spirit" plus some clearifications and corrections of my own.
- Dropping `SimpleOwnedRefCounted` by request from Alice, as it creates a
  potentially problematic blanket implementation (which a derive macro that
  could be created later would not have).
- Dropping Miguel's "kbuild: provide `RUSTC_HAS_DO_NOT_RECOMMEND` symbol"
  patch, as it is not needed anymore after dropping `SimpleOwnedRefCounted`.
  (I can add it again, if it is considered useful anyway).
- Link to v10: https://lore.kernel.org/r/20250502-unique-ref-v10-0-25de64c0307f@pm.me

Changes in v10:
- Moved kernel/ownable.rs to kernel/types/ownable.rs
- Fixes in documentation / comments as suggested by Andreas Hindborg
- Added Reviewed-by comment for Andreas Hindborg
- Fix rustfmt of pid_namespace.rs
- Link to v9: https://lore.kernel.org/r/20250325-unique-ref-v9-0-e91618c1de26@pm.me

Changes in v9:
- Rebase onto v6.14-rc7
- Move Ownable/OwnedRefCounted/Ownable, etc., into separate module
- Documentation fixes to Ownable/OwnableMut/OwnableRefCounted
- Add missing SAFETY documentation to ARef example
- Link to v8: https://lore.kernel.org/r/20250313-unique-ref-v8-0-3082ffc67a31@pm.me

Changes in v8:
- Fix Co-developed-by and Suggested-by tags as suggested by Miguel and Boqun
- Some small documentation fixes in Owned/Ownable patch
- removing redundant trait constraint on DerefMut for Owned as suggested by Boqun Feng
- make SimpleOwnedRefCounted no longer implement RefCounted as suggested by Boqun Feng
- documentation for RefCounted as suggested by Boqun Feng
- Link to v7: https://lore.kernel.org/r/20250310-unique-ref-v7-0-4caddb78aa05@pm.me

Changes in v7:
- Squash patch to make Owned::from_raw/into_raw public into parent
- Added Signed-off-by to other people's commits
- Link to v6: https://lore.kernel.org/r/20250310-unique-ref-v6-0-1ff53558617e@pm.me

Changes in v6:
- Changed comments/formatting as suggested by Miguel Ojeda
- Included and used new config flag RUSTC_HAS_DO_NOT_RECOMMEND,
  thus no changes to types.rs will be needed when the attribute
  becomes available.
- Fixed commit message for Owned patch.
- Link to v5: https://lore.kernel.org/r/20250307-unique-ref-v5-0-bffeb633277e@pm.me

Changes in v5:
- Rebase the whole thing on top of the Ownable/Owned traits by Asahi Lina.
- Rename AlwaysRefCounted to RefCounted and make AlwaysRefCounted a
  marker trait instead to allow to obtain an ARef<T> from an &T,
  which (as Alice pointed out) is unsound when combined with UniqueRef/Owned.
- Change the Trait design and naming to implement this feature,
  UniqueRef/UniqueRefCounted is dropped in favor of Ownable/Owned and
  OwnableRefCounted is used to provide the functions to convert
  between Owned and ARef.
- Link to v4: https://lore.kernel.org/r/20250305-unique-ref-v4-1-a8fdef7b1c2c@pm.me

Changes in v4:
- Just a minor change in naming by request from Andreas Hindborg,
  try_shared_to_unique() -> try_from_shared(),
  unique_to_shared() -> into_shared(),
  which is more in line with standard Rust naming conventions.
- Link to v3: https://lore.kernel.org/r/Z8Wuud2UQX6Yukyr@mango

---
Asahi Lina (1):
      rust: types: Add Ownable/Owned types

Oliver Mangold (3):
      rust: Split `AlwaysRefCounted` into two traits
      rust: Add missing SAFETY documentation for `ARef` example
      rust: Add `OwnableRefCounted`

 rust/kernel/block/mq/request.rs |  15 ++-
 rust/kernel/cred.rs             |   8 +-
 rust/kernel/device.rs           |   8 +-
 rust/kernel/fs/file.rs          |  10 +-
 rust/kernel/mm.rs               |  13 +-
 rust/kernel/mm/mmput_async.rs   |   9 +-
 rust/kernel/opp.rs              |   8 +-
 rust/kernel/pci.rs              |   6 +-
 rust/kernel/pid_namespace.rs    |   8 +-
 rust/kernel/platform.rs         |   6 +-
 rust/kernel/task.rs             |   6 +-
 rust/kernel/types.rs            |  62 +++++++---
 rust/kernel/types/ownable.rs    | 257 ++++++++++++++++++++++++++++++++++++++++
 13 files changed, 372 insertions(+), 44 deletions(-)
---
base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
change-id: 20250305-unique-ref-29fcd675f9e9

Best regards,
-- 
Oliver Mangold <oliver.mangold@pm.me>

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

[Oliver Mangold]

From: Asahi Lina <lina+kernel@asahilina.net>

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.

Conceptually, this is similar to a `KBox<T>`, except that it delegates
resource management to the `T` instead of using a generic allocator.

Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
Signed-off-by: Asahi Lina <lina@asahilina.net>
[ om:
  - split code into separate file and `pub use` it from types.rs
  - make from_raw() and into_raw() public
  - fixes to documentation and commit message
]
Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
---
 rust/kernel/types.rs         |   7 +++
 rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -11,6 +11,9 @@
 };
 use pin_init::{PinInit, Zeroable};
 
+pub mod ownable;
+pub use ownable::{Ownable, OwnableMut, Owned};
+
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
 /// Ownership is transferred from Rust to a foreign language by calling [`Self::into_foreign`] and
@@ -425,6 +428,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
 /// instances of a type.
 ///
+/// 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`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
+///
 /// # Safety
 ///
 /// Implementers must ensure that increments to the reference count keep the object alive in memory
diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
new file mode 100644
index 0000000000000000000000000000000000000000..f4065a0d627a62d3ecb15edabf306e9b812556e1
--- /dev/null
+++ b/rust/kernel/types/ownable.rs
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Owned reference types.
+
+use core::{
+    marker::PhantomData,
+    mem::ManuallyDrop,
+    ops::{Deref, DerefMut},
+    ptr::NonNull,
+};
+
+/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.
+///
+/// It allows such types to define their own custom destructor function to be called when a
+/// Rust-owned reference is dropped.
+///
+/// This is usually implemented by wrappers to existing structures on the C side of the code.
+///
+/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
+/// provide reference counting but represents a unique, owned reference. If reference counting is
+/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
+/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
+///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
+///   kernel is case it frees the object, etc.).
+pub unsafe trait Ownable {
+    /// Releases the object (frees it or returns it to foreign ownership).
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that:
+    /// - `this` points to a valid `Self`.
+    /// - The object is no longer referenced after this call.
+    unsafe fn release(this: NonNull<Self>);
+}
+
+/// Type where [`Owned<Self>`] derefs to `&mut Self`.
+///
+/// # Safety
+///
+/// Implementers must ensure that access to a `&mut T` is safe, implying that:
+/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
+///   (i.e. most kernel types).
+/// - The kernel will never access the underlying object (excluding internal mutability that follows
+///   the usual rules) while Rust owns it.
+pub unsafe trait OwnableMut: Ownable {}
+
+/// An owned reference to an ownable kernel object.
+///
+/// The object is automatically freed or released when an instance of [`Owned`] is
+/// dropped.
+///
+/// # Invariants
+///
+/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
+pub struct Owned<T: Ownable> {
+    ptr: NonNull<T>,
+    _p: PhantomData<T>,
+}
+
+// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
+// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
+unsafe impl<T: Ownable + Send> Send for Owned<T> {}
+
+// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
+// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
+unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
+
+impl<T: Ownable> Owned<T> {
+    /// Creates a new instance of [`Owned`].
+    ///
+    /// It takes over ownership of the underlying object.
+    ///
+    /// # Safety
+    ///
+    /// Callers must ensure that:
+    /// - Ownership of the underlying object can be transferred to the `Owned<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 `Owned<T>` is dropped).
+    /// - 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.
+    /// - In case `T` implements [`OwnableMut`] 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.
+    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
+        // INVARIANT: The safety requirements guarantee that the new instance now owns the
+        // reference.
+        Self {
+            ptr,
+            _p: PhantomData,
+        }
+    }
+
+    /// Consumes the [`Owned`], returning a raw pointer.
+    ///
+    /// This function does not actually relinquish ownership of the object. After calling this
+    /// function, the caller is responsible for ownership previously managed
+    /// by the [`Owned`].
+    pub fn into_raw(me: Self) -> NonNull<T> {
+        ManuallyDrop::new(me).ptr
+    }
+}
+
+impl<T: Ownable> Deref for Owned<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY: The type invariants guarantee that the object is valid.
+        unsafe { self.ptr.as_ref() }
+    }
+}
+
+impl<T: OwnableMut> DerefMut for Owned<T> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
+        // return a mutable reference to it.
+        unsafe { self.ptr.as_mut() }
+    }
+}
+
+impl<T: Ownable> Drop for Owned<T> {
+    fn drop(&mut self) {
+        // SAFETY: The type invariants guarantee that the `Owned` owns the object we're about to
+        // release.
+        unsafe { T::release(self.ptr) };
+    }
+}

-- 
2.49.0

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

[Benno Lossin]

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> From: Asahi Lina <lina+kernel@asahilina.net>
>
> 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.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> [ om:
>   - split code into separate file and `pub use` it from types.rs
>   - make from_raw() and into_raw() public
>   - fixes to documentation and commit message
> ]
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/types.rs         |   7 +++
>  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++

I think we should name this file `owned.rs` instead. It's also what
we'll have for `ARef` when that is moved to `sync/`.

Also, I do wonder does this really belong into the `types` module? I
feel like it's becoming our `utils` module and while it does fit, I
think we should just make this a top level module. So
`rust/kernel/owned.rs`. Thoughts?

>  2 files changed, 141 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,6 +11,9 @@
>  };
>  use pin_init::{PinInit, Zeroable};
>  
> +pub mod ownable;
> +pub use ownable::{Ownable, OwnableMut, Owned};
> +
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>  ///
>  /// Ownership is transferred from Rust to a foreign language by calling [`Self::into_foreign`] and
> @@ -425,6 +428,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>  /// instances of a type.
>  ///
> +/// 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`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
> +///
>  /// # Safety
>  ///
>  /// Implementers must ensure that increments to the reference count keep the object alive in memory
> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..f4065a0d627a62d3ecb15edabf306e9b812556e1
> --- /dev/null
> +++ b/rust/kernel/types/ownable.rs
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Owned reference types.

I think it's a good idea to expand the docs here a bit.

> +
> +use core::{
> +    marker::PhantomData,
> +    mem::ManuallyDrop,
> +    ops::{Deref, DerefMut},
> +    ptr::NonNull,
> +};
> +
> +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.

This seems wrong, `var: Owned<T>` should life until `min(var, T)`, so
whatever is earlier: until the user drops the `var` or `T`'s lifetime
ends.

How about we just say:

    Type allocated and destroyed on the C side, but owned by Rust.

> +///
> +/// It allows such types to define their own custom destructor function to be called when a
> +/// Rust-owned reference is dropped.

We shouldn't call this a reference. Also we should start the first
paragraph with how this trait enables the usage of `Owned<Self>`.

> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> +///
> +/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> +/// provide reference counting but represents a unique, owned reference. If reference counting is
> +/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
> +/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).

I think this is more confusing than helpful. We should mention
`AlwaysRefCounted`, but the phrasing needs to be changed. Something
along the lines: if you need reference counting, implement
`AlwaysRefCounted` instead.

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> +///   kernel is case it frees the object, etc.).

This invariant sounds weird to me. It's vague "a state which the kernel
expects" and difficult to use (what needs this invariant?).

> +pub unsafe trait Ownable {
> +    /// Releases the object (frees it or returns it to foreign ownership).

Let's remove the part in parenthesis.

> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - `this` points to a valid `Self`.
> +    /// - The object is no longer referenced after this call.

s/The object/`*this`/

s/referenced/used/

> +    unsafe fn release(this: NonNull<Self>);
> +}
> +
> +/// Type where [`Owned<Self>`] derefs to `&mut Self`.

How about:

    Type allowing mutable access via [`Owned<Self>`].

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that access to a `&mut T` is safe, implying that:

s/T/Self/

> +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> +///   (i.e. most kernel types).

Can't we implicitly pin `Owned`?

> +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> +///   the usual rules) while Rust owns it.
> +pub unsafe trait OwnableMut: Ownable {}
> +
> +/// An owned reference to an ownable kernel object.

How about
    
    An owned `T`.

> +///
> +/// The object is automatically freed or released when an instance of [`Owned`] is
> +/// dropped.

I don't think we need to say this, I always assume this for all Rust
types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
and thus also `Opaque`.)

How about we provide some examples here?

> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.

What exactly is "owned" supposed to mean? It depends on the concrete `T`
and that isn't well-defined (since it's a generic)...

Maybe we should give `Ownable` the task to document the exact ownership
semantics of `T`?

> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +    _p: PhantomData<T>,
> +}
> +
> +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).

How does this amount to sending a `&mut T`?

I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
list grows...

I'll try to come up with something to simplify this design a bit wrt the
safety docs.

> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).

Same here.

> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new instance of [`Owned`].
> +    ///
> +    /// It takes over ownership of the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - Ownership of the underlying object can be transferred to the `Owned<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 `Owned<T>` is dropped).
> +    /// - 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.
> +    /// - In case `T` implements [`OwnableMut`] 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.
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> +        // reference.

This doesn't follow for me. Well the first issue is that the safety
invariant of `Self` isn't well-defined, so let's revisit this when that
is fixed.

---
Cheers,
Benno

> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    /// Consumes the [`Owned`], returning a raw pointer.
> +    ///
> +    /// This function does not actually relinquish ownership of the object. After calling this
> +    /// function, the caller is responsible for ownership previously managed
> +    /// by the [`Owned`].
> +    pub fn into_raw(me: Self) -> NonNull<T> {
> +        ManuallyDrop::new(me).ptr
> +    }
> +}
> +
> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: OwnableMut> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> +        // return a mutable reference to it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}
> +
> +impl<T: Ownable> Drop for Owned<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: The type invariants guarantee that the `Owned` owns the object we're about to
> +        // release.
> +        unsafe { T::release(self.ptr) };
> +    }
> +}

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

[Oliver Mangold]

On 250702 1303, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > From: Asahi Lina <lina+kernel@asahilina.net>
> >
> > 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.
> >
> > Conceptually, this is similar to a `KBox<T>`, except that it delegates
> > resource management to the `T` instead of using a generic allocator.
> >
> > Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > [ om:
> >   - split code into separate file and `pub use` it from types.rs
> >   - make from_raw() and into_raw() public
> >   - fixes to documentation and commit message
> > ]
> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/types.rs         |   7 +++
> >  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
> 
> I think we should name this file `owned.rs` instead. It's also what
> we'll have for `ARef` when that is moved to `sync/`.
> 
> Also, I do wonder does this really belong into the `types` module? I
> feel like it's becoming our `utils` module and while it does fit, I
> think we should just make this a top level module. So
> `rust/kernel/owned.rs`. Thoughts?

I don't have much of an opinion on on that. But maybe refactoring types.rs
should be an independent task?

> 
> > +
> > +use core::{
> > +    marker::PhantomData,
> > +    mem::ManuallyDrop,
> > +    ops::{Deref, DerefMut},
> > +    ptr::NonNull,
> > +};
> > +
> > +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.
> 
> This seems wrong, `var: Owned<T>` should life until `min(var, T)`, so
> whatever is earlier: until the user drops the `var` or `T`'s lifetime
> ends.

Yes, I guess that sounds sloppy.

> How about we just say:
> 
>     Type allocated and destroyed on the C side, but owned by Rust.

Would be okay with me.

> > +///
> > +/// It allows such types to define their own custom destructor function to be called when a
> > +/// Rust-owned reference is dropped.
> 
> We shouldn't call this a reference. Also we should start the first
> paragraph with how this trait enables the usage of `Owned<Self>`.
> 
> > +///
> > +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> > +///
> > +/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> > +/// provide reference counting but represents a unique, owned reference. If reference counting is
> > +/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
> > +/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).
> 
> I think this is more confusing than helpful. We should mention
> `AlwaysRefCounted`, but the phrasing needs to be changed. Something
> along the lines: if you need reference counting, implement
> `AlwaysRefCounted` instead.

I guess you have a point. The shortened version is probably good enough.

> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> > +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> > +///   kernel is case it frees the object, etc.).
> 
> This invariant sounds weird to me. It's vague "a state which the kernel
> expects" and difficult to use (what needs this invariant?).
> 
> > +pub unsafe trait Ownable {
> > +    /// Releases the object (frees it or returns it to foreign ownership).
> 
> Let's remove the part in parenthesis.
> 
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that:
> > +    /// - `this` points to a valid `Self`.
> > +    /// - The object is no longer referenced after this call.
> 
> s/The object/`*this`/
> 
> s/referenced/used/
> 
> > +    unsafe fn release(this: NonNull<Self>);
> > +}
> > +
> > +/// Type where [`Owned<Self>`] derefs to `&mut Self`.
> 
> How about:
> 
>     Type allowing mutable access via [`Owned<Self>`].
> 
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that access to a `&mut T` is safe, implying that:
> 
> s/T/Self/

Okay with all of the above.

> > +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> > +///   (i.e. most kernel types).
> 
> Can't we implicitly pin `Owned`?

I have been thinking about that. But this would mean that the blanket
implementation for `Deref` would conflict with the one for `OwnableMut`.

> > +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> > +///   the usual rules) while Rust owns it.
> > +pub unsafe trait OwnableMut: Ownable {}
> > +
> > +/// An owned reference to an ownable kernel object.
> 
> How about
> 
>     An owned `T`.

    A wrapper around `T`.

maybe?

> > +///
> > +/// The object is automatically freed or released when an instance of [`Owned`] is
> > +/// dropped.
> 
> I don't think we need to say this, I always assume this for all Rust
> types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
> and thus also `Opaque`.)

Hmm, it is an important feature of the wrapper that it turns the `*Ownable`
into an object that is automatically dropped. So shouldn't that be
mentioned here?

> How about we provide some examples here?
> 
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
> 
> What exactly is "owned" supposed to mean? It depends on the concrete `T`
> and that isn't well-defined (since it's a generic)...

"owned" means that access to the `T` is exclusive through the `Owned<T>`,
so normal Rust semantics can be applied.

> Maybe we should give `Ownable` the task to document the exact ownership
> semantics of `T`?
> 
> > +pub struct Owned<T: Ownable> {
> > +    ptr: NonNull<T>,
> > +    _p: PhantomData<T>,
> > +}
> > +
> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> 
> How does this amount to sending a `&mut T`?

Good point. That documentation hasn't been updated since `&mut T` access
has been split out into `OwnableMut`. Not sure how to phrase it now.

> I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
> list grows...
> 
> I'll try to come up with something to simplify this design a bit wrt the
> safety docs.
> 
> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> > +
> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> 
> Same here.
> 
> > +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> > +
> > +impl<T: Ownable> Owned<T> {
> > +    /// Creates a new instance of [`Owned`].
> > +    ///
> > +    /// It takes over ownership of the underlying object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that:
> > +    /// - Ownership of the underlying object can be transferred to the `Owned<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 `Owned<T>` is dropped).
> > +    /// - 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.
> > +    /// - In case `T` implements [`OwnableMut`] 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.
> > +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> > +        // reference.
> 
> This doesn't follow for me. Well the first issue is that the safety
> invariant of `Self` isn't well-defined, so let's revisit this when that
> is fixed.

I guess it follows from:

> > +    /// It takes over ownership of the underlying object.

Okay, sure, it not in the safety requirement.

Best,

Oliver

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

[Benno Lossin]

On Mon Jul 7, 2025 at 8:58 AM CEST, Oliver Mangold wrote:
> On 250702 1303, Benno Lossin wrote:
>> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> > From: Asahi Lina <lina+kernel@asahilina.net>
>> >
>> > 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.
>> >
>> > Conceptually, this is similar to a `KBox<T>`, except that it delegates
>> > resource management to the `T` instead of using a generic allocator.
>> >
>> > Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
>> > Signed-off-by: Asahi Lina <lina@asahilina.net>
>> > [ om:
>> >   - split code into separate file and `pub use` it from types.rs
>> >   - make from_raw() and into_raw() public
>> >   - fixes to documentation and commit message
>> > ]
>> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
>> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>> > ---
>> >  rust/kernel/types.rs         |   7 +++
>> >  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
>> 
>> I think we should name this file `owned.rs` instead. It's also what
>> we'll have for `ARef` when that is moved to `sync/`.
>> 
>> Also, I do wonder does this really belong into the `types` module? I
>> feel like it's becoming our `utils` module and while it does fit, I
>> think we should just make this a top level module. So
>> `rust/kernel/owned.rs`. Thoughts?
>
> I don't have much of an opinion on on that. But maybe refactoring types.rs
> should be an independent task?

But you're adding the new file there? Just add it under
`rust/kernel/owned.rs` instead.

>> > +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
>> > +///   (i.e. most kernel types).
>> 
>> Can't we implicitly pin `Owned`?
>
> I have been thinking about that. But this would mean that the blanket
> implementation for `Deref` would conflict with the one for `OwnableMut`.

Yeah we could not implement `DerefMut` in that case (or only for `T:
Unpin`).

>> > +/// - The kernel will never access the underlying object (excluding internal mutability that follows
>> > +///   the usual rules) while Rust owns it.
>> > +pub unsafe trait OwnableMut: Ownable {}
>> > +
>> > +/// An owned reference to an ownable kernel object.
>> 
>> How about
>> 
>>     An owned `T`.
>
>     A wrapper around `T`.
>
> maybe?

"wrapper" seems wrong, since a wrapper in my mind is a newtype. So it
would be `struct Owned(T)` which is wrong. The `T` is stored elsewhere.

>> > +///
>> > +/// The object is automatically freed or released when an instance of [`Owned`] is
>> > +/// dropped.
>> 
>> I don't think we need to say this, I always assume this for all Rust
>> types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
>> and thus also `Opaque`.)
>
> Hmm, it is an important feature of the wrapper that it turns the `*Ownable`
> into an object that is automatically dropped. So shouldn't that be
> mentioned here?

I would expect that that happens, but sure we can leave it here.

>> How about we provide some examples here?
>> 
>> > +///
>> > +/// # Invariants
>> > +///
>> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
>> 
>> What exactly is "owned" supposed to mean? It depends on the concrete `T`
>> and that isn't well-defined (since it's a generic)...
>
> "owned" means that access to the `T` is exclusive through the `Owned<T>`,
> so normal Rust semantics can be applied.

Okay, in that case just say that `ptr` has exclusive access.

>> Maybe we should give `Ownable` the task to document the exact ownership
>> semantics of `T`?
>> 
>> > +pub struct Owned<T: Ownable> {
>> > +    ptr: NonNull<T>,
>> > +    _p: PhantomData<T>,
>> > +}
>> > +
>> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
>> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
>> 
>> How does this amount to sending a `&mut T`?
>
> Good point. That documentation hasn't been updated since `&mut T` access
> has been split out into `OwnableMut`. Not sure how to phrase it now.

I'm also unsure, also for the correct trait bounds on this impl.

---
Cheers,
Benno

>> I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
>> list grows...
>> 
>> I'll try to come up with something to simplify this design a bit wrt the
>> safety docs.
>> 
>> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
>> > +
>> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
>> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
>> 
>> Same here.
>> 
>> > +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}

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

[Oliver Mangold]

On 250707 1123, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 8:58 AM CEST, Oliver Mangold wrote:
> > On 250702 1303, Benno Lossin wrote:
> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> >> > From: Asahi Lina <lina+kernel@asahilina.net>
> >> >
> >> > 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.
> >> >
> >> > Conceptually, this is similar to a `KBox<T>`, except that it delegates
> >> > resource management to the `T` instead of using a generic allocator.
> >> >
> >> > Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> >> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> >> > [ om:
> >> >   - split code into separate file and `pub use` it from types.rs
> >> >   - make from_raw() and into_raw() public
> >> >   - fixes to documentation and commit message
> >> > ]
> >> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> >> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> >> > ---
> >> >  rust/kernel/types.rs         |   7 +++
> >> >  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
> >>
> >> I think we should name this file `owned.rs` instead. It's also what
> >> we'll have for `ARef` when that is moved to `sync/`.
> >>
> >> Also, I do wonder does this really belong into the `types` module? I
> >> feel like it's becoming our `utils` module and while it does fit, I
> >> think we should just make this a top level module. So
> >> `rust/kernel/owned.rs`. Thoughts?
> >
> > I don't have much of an opinion on on that. But maybe refactoring types.rs
> > should be an independent task?
> 
> But you're adding the new file there? Just add it under
> `rust/kernel/owned.rs` instead.

To be honest, I don't really mind.

Note, though, that I already moved it from types.rs to types/ownable.rs on
request. It seems to me different people here have different ideas where it
should be placed. I feel now, that it would make sense to come to an
agreement between the interested parties about where it should finally be
placed, before I move it again. Could I ask that we settle that question
once and for all before my next revision?

> >> > +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> >> > +///   (i.e. most kernel types).
> >>
> >> Can't we implicitly pin `Owned`?
> >
> > I have been thinking about that. But this would mean that the blanket
> > implementation for `Deref` would conflict with the one for `OwnableMut`.
> 
> Yeah we could not implement `DerefMut` in that case (or only for `T:
> Unpin`).
> 
> >> > +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> >> > +///   the usual rules) while Rust owns it.
> >> > +pub unsafe trait OwnableMut: Ownable {}
> >> > +
> >> > +/// An owned reference to an ownable kernel object.
> >>
> >> How about
> >>
> >>     An owned `T`.
> >
> >     A wrapper around `T`.
> >
> > maybe?
> 
> "wrapper" seems wrong, since a wrapper in my mind is a newtype. So it
> would be `struct Owned(T)` which is wrong. The `T` is stored elsewhere.
> 
> >> > +///
> >> > +/// The object is automatically freed or released when an instance of [`Owned`] is
> >> > +/// dropped.
> >>
> >> I don't think we need to say this, I always assume this for all Rust
> >> types except they document otherwise (eg `ManuallyDrop`, `MaybeUninit`
> >> and thus also `Opaque`.)
> >
> > Hmm, it is an important feature of the wrapper that it turns the `*Ownable`
> > into an object that is automatically dropped. So shouldn't that be
> > mentioned here?
> 
> I would expect that that happens, but sure we can leave it here.
> 
> >> How about we provide some examples here?
> >>
> >> > +///
> >> > +/// # Invariants
> >> > +///
> >> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
> >>
> >> What exactly is "owned" supposed to mean? It depends on the concrete `T`
> >> and that isn't well-defined (since it's a generic)...
> >
> > "owned" means that access to the `T` is exclusive through the `Owned<T>`,
> > so normal Rust semantics can be applied.
> 
> Okay, in that case just say that `ptr` has exclusive access.

Or, ehm, sorry, I forgot, ownership also implies that the allocation of the
underlying resource/object is now under the responsibility of the owner,
i.e. the owner should free it at the appropriate time.

In short, just the standard meaning of ownership in Rust.

https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html

> >> Maybe we should give `Ownable` the task to document the exact ownership
> >> semantics of `T`?
> >>
> >> > +pub struct Owned<T: Ownable> {
> >> > +    ptr: NonNull<T>,
> >> > +    _p: PhantomData<T>,
> >> > +}
> >> > +
> >> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> >> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> >>
> >> How does this amount to sending a `&mut T`?
> >
> > Good point. That documentation hasn't been updated since `&mut T` access
> > has been split out into `OwnableMut`. Not sure how to phrase it now.
> 
> I'm also unsure, also for the correct trait bounds on this impl.
> 
> ---
> Cheers,
> Benno
> 
> >> I guess this also needs to be guaranteed by `Owned::from_raw`... ah the
> >> list grows...
> >>
> >> I'll try to come up with something to simplify this design a bit wrt the
> >> safety docs.
> >>
> >> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> >> > +
> >> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> >> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> >>
> >> Same here.
> >>
> >> > +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}

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

[Miguel Ojeda]

On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> Note, though, that I already moved it from types.rs to types/ownable.rs on
> request. It seems to me different people here have different ideas where it
> should be placed. I feel now, that it would make sense to come to an
> agreement between the interested parties about where it should finally be
> placed, before I move it again. Could I ask that we settle that question
> once and for all before my next revision?

Yeah, if there is a disagreement with something said previously, then
it should be resolved before starting to ping-pong between approaches
with more and more patch versions. Reviewers can forget or they may
not have read an earlier comment, but you did the right thing
mentioning there is such a conflict in opinions.

Cheers,
Miguel

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

[Benno Lossin]

On Tue Jul 8, 2025 at 12:16 PM CEST, Miguel Ojeda wrote:
> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>
>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>> request. It seems to me different people here have different ideas where it
>> should be placed. I feel now, that it would make sense to come to an
>> agreement between the interested parties about where it should finally be
>> placed, before I move it again. Could I ask that we settle that question
>> once and for all before my next revision?
>
> Yeah, if there is a disagreement with something said previously, then
> it should be resolved before starting to ping-pong between approaches
> with more and more patch versions. Reviewers can forget or they may
> not have read an earlier comment, but you did the right thing
> mentioning there is such a conflict in opinions.

Yeah, I checked and that was Andreas on v9. @Andreas what do you think?

I think we should just get rid of `types.rs` and split it into:

* `opaque.rs`
* `foreign.rs`
* `scope_guard.rs` (this might need a better name)

`Either` can just be removed entirely, `AlwaysRefcounted` & `ARef`
should be in the `sync` module (I already created an issue for this) as
well as `NotThreadSafe` (or we could create a `marker` module for that).
Thoughts?

---
Cheers,
Benno

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

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jul 8, 2025 at 12:16 PM CEST, Miguel Ojeda wrote:
>> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>>
>>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>>> request. It seems to me different people here have different ideas where it
>>> should be placed. I feel now, that it would make sense to come to an
>>> agreement between the interested parties about where it should finally be
>>> placed, before I move it again. Could I ask that we settle that question
>>> once and for all before my next revision?
>>
>> Yeah, if there is a disagreement with something said previously, then
>> it should be resolved before starting to ping-pong between approaches
>> with more and more patch versions. Reviewers can forget or they may
>> not have read an earlier comment, but you did the right thing
>> mentioning there is such a conflict in opinions.
>
> Yeah, I checked and that was Andreas on v9. @Andreas what do you think?
>
> I think we should just get rid of `types.rs` and split it into:
>
> * `opaque.rs`
> * `foreign.rs`
> * `scope_guard.rs` (this might need a better name)
>
> `Either` can just be removed entirely, `AlwaysRefcounted` & `ARef`
> should be in the `sync` module (I already created an issue for this) as
> well as `NotThreadSafe` (or we could create a `marker` module for that).
> Thoughts?

Sounds good. I just wanted to prevent us from cramming everything into
types.rs.

But we should probably move `Owned` into `sync` with `ARef` et. al.,
right?


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Tue Jul 8, 2025 at 8:30 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Tue Jul 8, 2025 at 12:16 PM CEST, Miguel Ojeda wrote:
>>> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>>>
>>>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>>>> request. It seems to me different people here have different ideas where it
>>>> should be placed. I feel now, that it would make sense to come to an
>>>> agreement between the interested parties about where it should finally be
>>>> placed, before I move it again. Could I ask that we settle that question
>>>> once and for all before my next revision?
>>>
>>> Yeah, if there is a disagreement with something said previously, then
>>> it should be resolved before starting to ping-pong between approaches
>>> with more and more patch versions. Reviewers can forget or they may
>>> not have read an earlier comment, but you did the right thing
>>> mentioning there is such a conflict in opinions.
>>
>> Yeah, I checked and that was Andreas on v9. @Andreas what do you think?
>>
>> I think we should just get rid of `types.rs` and split it into:
>>
>> * `opaque.rs`
>> * `foreign.rs`
>> * `scope_guard.rs` (this might need a better name)
>>
>> `Either` can just be removed entirely, `AlwaysRefcounted` & `ARef`
>> should be in the `sync` module (I already created an issue for this) as
>> well as `NotThreadSafe` (or we could create a `marker` module for that).
>> Thoughts?
>
> Sounds good. I just wanted to prevent us from cramming everything into
> types.rs.

Yeah me too :)

> But we should probably move `Owned` into `sync` with `ARef` et. al.,
> right?

I don't think it fits into `sync`... It's more like a `Box` and could
very well store something that is `!Send` & `!Sync`.

---
Cheers,
Benno

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

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jul 8, 2025 at 8:30 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Tue Jul 8, 2025 at 12:16 PM CEST, Miguel Ojeda wrote:
>>>> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>>>>
>>>>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>>>>> request. It seems to me different people here have different ideas where it
>>>>> should be placed. I feel now, that it would make sense to come to an
>>>>> agreement between the interested parties about where it should finally be
>>>>> placed, before I move it again. Could I ask that we settle that question
>>>>> once and for all before my next revision?
>>>>
>>>> Yeah, if there is a disagreement with something said previously, then
>>>> it should be resolved before starting to ping-pong between approaches
>>>> with more and more patch versions. Reviewers can forget or they may
>>>> not have read an earlier comment, but you did the right thing
>>>> mentioning there is such a conflict in opinions.
>>>
>>> Yeah, I checked and that was Andreas on v9. @Andreas what do you think?
>>>
>>> I think we should just get rid of `types.rs` and split it into:
>>>
>>> * `opaque.rs`
>>> * `foreign.rs`
>>> * `scope_guard.rs` (this might need a better name)
>>>
>>> `Either` can just be removed entirely, `AlwaysRefcounted` & `ARef`
>>> should be in the `sync` module (I already created an issue for this) as
>>> well as `NotThreadSafe` (or we could create a `marker` module for that).
>>> Thoughts?
>>
>> Sounds good. I just wanted to prevent us from cramming everything into
>> types.rs.
>
> Yeah me too :)
>
>> But we should probably move `Owned` into `sync` with `ARef` et. al.,
>> right?
>
> I don't think it fits into `sync`... It's more like a `Box` and could
> very well store something that is `!Send` & `!Sync`.

Good point. My primary use case is `OwnableRefCounted`. Where do you
want tput that? It is strongly tied to `Ownable` and `Owned`, but
revolving around refcounts.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Wed Jul 9, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Tue Jul 8, 2025 at 8:30 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>>> On Tue Jul 8, 2025 at 12:16 PM CEST, Miguel Ojeda wrote:
>>>>> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>>>>>>
>>>>>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>>>>>> request. It seems to me different people here have different ideas where it
>>>>>> should be placed. I feel now, that it would make sense to come to an
>>>>>> agreement between the interested parties about where it should finally be
>>>>>> placed, before I move it again. Could I ask that we settle that question
>>>>>> once and for all before my next revision?
>>>>>
>>>>> Yeah, if there is a disagreement with something said previously, then
>>>>> it should be resolved before starting to ping-pong between approaches
>>>>> with more and more patch versions. Reviewers can forget or they may
>>>>> not have read an earlier comment, but you did the right thing
>>>>> mentioning there is such a conflict in opinions.
>>>>
>>>> Yeah, I checked and that was Andreas on v9. @Andreas what do you think?
>>>>
>>>> I think we should just get rid of `types.rs` and split it into:
>>>>
>>>> * `opaque.rs`
>>>> * `foreign.rs`
>>>> * `scope_guard.rs` (this might need a better name)
>>>>
>>>> `Either` can just be removed entirely, `AlwaysRefcounted` & `ARef`
>>>> should be in the `sync` module (I already created an issue for this) as
>>>> well as `NotThreadSafe` (or we could create a `marker` module for that).
>>>> Thoughts?
>>>
>>> Sounds good. I just wanted to prevent us from cramming everything into
>>> types.rs.
>>
>> Yeah me too :)
>>
>>> But we should probably move `Owned` into `sync` with `ARef` et. al.,
>>> right?
>>
>> I don't think it fits into `sync`... It's more like a `Box` and could
>> very well store something that is `!Send` & `!Sync`.
>
> Good point. My primary use case is `OwnableRefCounted`. Where do you
> want tput that? It is strongly tied to `Ownable` and `Owned`, but
> revolving around refcounts.

Yeah... Let's just put it in owned.rs too.

---
Cheers,
Benno

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

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pmme> wrote:
>>
>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>> request. It seems to me different people here have different ideas where it
>> should be placed. I feel now, that it would make sense to come to an
>> agreement between the interested parties about where it should finally be
>> placed, before I move it again. Could I ask that we settle that question
>> once and for all before my next revision?
>
> Yeah, if there is a disagreement with something said previously, then
> it should be resolved before starting to ping-pong between approaches
> with more and more patch versions. Reviewers can forget or they may
> not have read an earlier comment, but you did the right thing
> mentioning there is such a conflict in opinions.

Didn't we start with this placed in the top level kernel crate?

Anyway, I think it should go next to `ARef` and `AlwaysRefCounted`,
which is in `types`. I think I asked to have it moved to a submodule at
some point to keep types.rs manageable.

If we place this one to top level, we should move `ARef` and
`AlwaysRefCounted` as well.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Tue Jul 8, 2025 at 3:22 PM CEST, Andreas Hindborg wrote:
> "Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:
>> On Tue, Jul 8, 2025 at 11:57 AM Oliver Mangold <oliver.mangold@pmme> wrote:
>>> Note, though, that I already moved it from types.rs to types/ownable.rs on
>>> request. It seems to me different people here have different ideas where it
>>> should be placed. I feel now, that it would make sense to come to an
>>> agreement between the interested parties about where it should finally be
>>> placed, before I move it again. Could I ask that we settle that question
>>> once and for all before my next revision?
>>
>> Yeah, if there is a disagreement with something said previously, then
>> it should be resolved before starting to ping-pong between approaches
>> with more and more patch versions. Reviewers can forget or they may
>> not have read an earlier comment, but you did the right thing
>> mentioning there is such a conflict in opinions.
>
> Didn't we start with this placed in the top level kernel crate?
>
> Anyway, I think it should go next to `ARef` and `AlwaysRefCounted`,
> which is in `types`. I think I asked to have it moved to a submodule at
> some point to keep types.rs manageable.

I don't think we should have the `types` module at all, see my other
response.

> If we place this one to top level, we should move `ARef` and
> `AlwaysRefCounted` as well.

I already created an issue [1] & a patch was sent [2].

[1]: https://github.com/Rust-for-Linux/linux/issues/1173
[2]: https://lore.kernel.org/rust-for-linux/20250623192530.266103-1-shankari.ak0208@gmail.com/

---
Cheers,
Benno

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

[Benno Lossin]

On Tue Jul 8, 2025 at 11:56 AM CEST, Oliver Mangold wrote:
> On 250707 1123, Benno Lossin wrote:
>> On Mon Jul 7, 2025 at 8:58 AM CEST, Oliver Mangold wrote:
>> > On 250702 1303, Benno Lossin wrote:
>> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> >> > +///
>> >> > +/// # Invariants
>> >> > +///
>> >> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
>> >>
>> >> What exactly is "owned" supposed to mean? It depends on the concrete `T`
>> >> and that isn't well-defined (since it's a generic)...
>> >
>> > "owned" means that access to the `T` is exclusive through the `Owned<T>`,
>> > so normal Rust semantics can be applied.
>> 
>> Okay, in that case just say that `ptr` has exclusive access.
>
> Or, ehm, sorry, I forgot, ownership also implies that the allocation of the
> underlying resource/object is now under the responsibility of the owner,
> i.e. the owner should free it at the appropriate time.
>
> In short, just the standard meaning of ownership in Rust.
>
> https://doc.rust-lang.org/book/ch04-01-what-is-ownership.html

Okay that's good to hear. I think what tripped me up the most was the
"can be considered" wording. Let's just say:

    /// # Invariants
    ///
    /// - `ptr` is valid,
    /// - `*ptr` is owned by `self`,
    /// - `ptr` is an "owning pointer" according to the [`Ownable`] implementation for `T`.

And then on `Ownable` we add:

    /// # Invariants
    ///
    /// An implementer of this trait needs to define which pointers can be supplied to
    /// [`Self::release`]. These pointers are called "owning pointers".

This should be as general as possible and still give us exactly the
guarantees that we need to implement `Owned`.

`Owned::from_raw` can then require that the pointer is an owning
pointer (& it's valid) and that the caller yields ownership to
`from_raw`.

---
Cheers,
Benno

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

[Miguel Ojeda]

On Mon, Jul 7, 2025 at 8:58 AM Oliver Mangold <oliver.mangold@pm.me> wrote:
>
> I don't have much of an opinion on on that. But maybe refactoring types.rs
> should be an independent task?

Yeah; however, putting things in the final place now (even if we don't
move the rest, which as you say, is independent) avoids a move later
on. Moves are themselves easy, but they can end up being painful to
coordinate between trees / across kernel cycles, and they also make
things harder for backports.

So, if possible, it is best to try to add them in the right place, and
move the rest later.

Thanks!

Cheers,
Miguel

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

[Andreas Hindborg]

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> From: Asahi Lina <lina+kernel@asahilina.net>
>
> 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.
>
> Conceptually, this is similar to a `KBox<T>`, except that it delegates
> resource management to the `T` instead of using a generic allocator.
>
> Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> [ om:
>   - split code into separate file and `pub use` it from types.rs
>   - make from_raw() and into_raw() public
>   - fixes to documentation and commit message
> ]
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  rust/kernel/types.rs         |   7 +++
>  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+)
>
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -11,6 +11,9 @@
>  };
>  use pin_init::{PinInit, Zeroable};
>
> +pub mod ownable;
> +pub use ownable::{Ownable, OwnableMut, Owned};
> +
>  /// Used to transfer ownership to and from foreign (non-Rust) languages.
>  ///
>  /// Ownership is transferred from Rust to a foreign language by calling [`Self::into_foreign`] and
> @@ -425,6 +428,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>  /// instances of a type.
>  ///
> +/// 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`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
> +///
>  /// # Safety
>  ///
>  /// Implementers must ensure that increments to the reference count keep the object alive in memory
> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..f4065a0d627a62d3ecb15edabf306e9b812556e1
> --- /dev/null
> +++ b/rust/kernel/types/ownable.rs
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Owned reference types.
> +
> +use core::{
> +    marker::PhantomData,
> +    mem::ManuallyDrop,
> +    ops::{Deref, DerefMut},
> +    ptr::NonNull,
> +};
> +
> +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.
> +///
> +/// It allows such types to define their own custom destructor function to be called when a
> +/// Rust-owned reference is dropped.
> +///
> +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> +///
> +/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> +/// provide reference counting but represents a unique, owned reference. If reference counting is
> +/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
> +/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> +///   kernel is case it frees the object, etc.).
> +pub unsafe trait Ownable {
> +    /// Releases the object (frees it or returns it to foreign ownership).
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - `this` points to a valid `Self`.
> +    /// - The object is no longer referenced after this call.
> +    unsafe fn release(this: NonNull<Self>);
> +}
> +
> +/// Type where [`Owned<Self>`] derefs to `&mut Self`.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that access to a `&mut T` is safe, implying that:
> +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> +///   (i.e. most kernel types).
> +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> +///   the usual rules) while Rust owns it.
> +pub unsafe trait OwnableMut: Ownable {}
> +
> +/// An owned reference to an ownable kernel object.
> +///
> +/// The object is automatically freed or released when an instance of [`Owned`] is
> +/// dropped.
> +///
> +/// # Invariants
> +///
> +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
> +pub struct Owned<T: Ownable> {
> +    ptr: NonNull<T>,
> +    _p: PhantomData<T>,
> +}
> +
> +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> +
> +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> +
> +impl<T: Ownable> Owned<T> {
> +    /// Creates a new instance of [`Owned`].
> +    ///
> +    /// It takes over ownership of the underlying object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that:
> +    /// - Ownership of the underlying object can be transferred to the `Owned<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 `Owned<T>` is dropped).
> +    /// - 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.
> +    /// - In case `T` implements [`OwnableMut`] 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.
> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> +        // reference.
> +        Self {
> +            ptr,
> +            _p: PhantomData,
> +        }
> +    }
> +
> +    /// Consumes the [`Owned`], returning a raw pointer.
> +    ///
> +    /// This function does not actually relinquish ownership of the object. After calling this
> +    /// function, the caller is responsible for ownership previously managed
> +    /// by the [`Owned`].
> +    pub fn into_raw(me: Self) -> NonNull<T> {
> +        ManuallyDrop::new(me).ptr
> +    }
> +}
> +
> +impl<T: Ownable> Deref for Owned<T> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid.
> +        unsafe { self.ptr.as_ref() }
> +    }
> +}
> +
> +impl<T: OwnableMut> DerefMut for Owned<T> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> +        // return a mutable reference to it.
> +        unsafe { self.ptr.as_mut() }
> +    }
> +}

I think someone mentioned this before, but handing out mutable
references can be a problem if `T: !Unpin`. For instance, we don't want
to hand out `&mut Page` in case of `Owned<Page>`.

Could we do something like this:

  diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
  index 29729dc10cb4..52d21e41c26b 100644
  --- a/rust/kernel/types/ownable.rs
  +++ b/rust/kernel/types/ownable.rs
  @@ -3,6 +3,7 @@
  //! Owned reference types.

  use crate::{
  +    prelude::*,
      sync::aref::{ARef, RefCounted},
      types::ForeignOwnable,
  };
  @@ -112,6 +113,17 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
      pub fn into_raw(me: Self) -> NonNull<T> {
          ManuallyDrop::new(me).ptr
      }
  +
  +    /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
  +    pub fn get_pin_mut(&mut self) -> Pin<&mut T> {
  +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
  +        // return a mutable reference to it.
  +        let unpinned = unsafe { self.ptr.as_mut() };
  +
  +        // SAFETY: We never hand out unpinned mutable references to the data in
  +        // `Self`, unless the contained type is `Unpin`.
  +        unsafe { Pin::new_unchecked(unpinned) }
  +    }
  }

  impl<T: Ownable> Deref for Owned<T> {
  @@ -123,7 +135,7 @@ fn deref(&self) -> &Self::Target {
      }
  }

  -impl<T: OwnableMut> DerefMut for Owned<T> {
  +impl<T: OwnableMut + Unpin> DerefMut for Owned<T> {
      fn deref_mut(&mut self) -> &mut Self::Target {
          // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
          // return a mutable reference to it.


Best regards,
Andreas Hindborg

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

[Oliver Mangold]

On 250818 1446, Andreas Hindborg wrote:
> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> 
> > From: Asahi Lina <lina+kernel@asahilina.net>
> >
> > 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.
> >
> > Conceptually, this is similar to a `KBox<T>`, except that it delegates
> > resource management to the `T` instead of using a generic allocator.
> >
> > Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
> > Signed-off-by: Asahi Lina <lina@asahilina.net>
> > [ om:
> >   - split code into separate file and `pub use` it from types.rs
> >   - make from_raw() and into_raw() public
> >   - fixes to documentation and commit message
> > ]
> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> > Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  rust/kernel/types.rs         |   7 +++
> >  rust/kernel/types/ownable.rs | 134 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 141 insertions(+)
> >
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -11,6 +11,9 @@
> >  };
> >  use pin_init::{PinInit, Zeroable};
> >
> > +pub mod ownable;
> > +pub use ownable::{Ownable, OwnableMut, Owned};
> > +
> >  /// Used to transfer ownership to and from foreign (non-Rust) languages.
> >  ///
> >  /// Ownership is transferred from Rust to a foreign language by calling [`Self::into_foreign`] and
> > @@ -425,6 +428,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
> >  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> >  /// instances of a type.
> >  ///
> > +/// 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`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
> > +///
> >  /// # Safety
> >  ///
> >  /// Implementers must ensure that increments to the reference count keep the object alive in memory
> > diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f4065a0d627a62d3ecb15edabf306e9b812556e1
> > --- /dev/null
> > +++ b/rust/kernel/types/ownable.rs
> > @@ -0,0 +1,134 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +//! Owned reference types.
> > +
> > +use core::{
> > +    marker::PhantomData,
> > +    mem::ManuallyDrop,
> > +    ops::{Deref, DerefMut},
> > +    ptr::NonNull,
> > +};
> > +
> > +/// Types that may be owned by Rust code or borrowed, but have a lifetime managed by C code.
> > +///
> > +/// It allows such types to define their own custom destructor function to be called when a
> > +/// Rust-owned reference is dropped.
> > +///
> > +/// This is usually implemented by wrappers to existing structures on the C side of the code.
> > +///
> > +/// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> > +/// provide reference counting but represents a unique, owned reference. If reference counting is
> > +/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
> > +/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +/// - The [`release()`](Ownable::release) method leaves the underlying object in a state which the
> > +///   kernel expects after ownership has been relinquished (i.e. no dangling references in the
> > +///   kernel is case it frees the object, etc.).
> > +pub unsafe trait Ownable {
> > +    /// Releases the object (frees it or returns it to foreign ownership).
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that:
> > +    /// - `this` points to a valid `Self`.
> > +    /// - The object is no longer referenced after this call.
> > +    unsafe fn release(this: NonNull<Self>);
> > +}
> > +
> > +/// Type where [`Owned<Self>`] derefs to `&mut Self`.
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that access to a `&mut T` is safe, implying that:
> > +/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> > +///   (i.e. most kernel types).
> > +/// - The kernel will never access the underlying object (excluding internal mutability that follows
> > +///   the usual rules) while Rust owns it.
> > +pub unsafe trait OwnableMut: Ownable {}
> > +
> > +/// An owned reference to an ownable kernel object.
> > +///
> > +/// The object is automatically freed or released when an instance of [`Owned`] is
> > +/// dropped.
> > +///
> > +/// # Invariants
> > +///
> > +/// The pointer stored in `ptr` can be considered owned by the [`Owned`] instance.
> > +pub struct Owned<T: Ownable> {
> > +    ptr: NonNull<T>,
> > +    _p: PhantomData<T>,
> > +}
> > +
> > +// SAFETY: It is safe to send `Owned<T>` to another thread when the underlying `T` is `Send` because
> > +// it effectively means sending a `&mut T` (which is safe because `T` is `Send`).
> > +unsafe impl<T: Ownable + Send> Send for Owned<T> {}
> > +
> > +// SAFETY: It is safe to send `&Owned<T>` to another thread when the underlying `T` is `Sync`
> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`).
> > +unsafe impl<T: Ownable + Sync> Sync for Owned<T> {}
> > +
> > +impl<T: Ownable> Owned<T> {
> > +    /// Creates a new instance of [`Owned`].
> > +    ///
> > +    /// It takes over ownership of the underlying object.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// Callers must ensure that:
> > +    /// - Ownership of the underlying object can be transferred to the `Owned<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 `Owned<T>` is dropped).
> > +    /// - 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.
> > +    /// - In case `T` implements [`OwnableMut`] 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.
> > +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> > +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> > +        // reference.
> > +        Self {
> > +            ptr,
> > +            _p: PhantomData,
> > +        }
> > +    }
> > +
> > +    /// Consumes the [`Owned`], returning a raw pointer.
> > +    ///
> > +    /// This function does not actually relinquish ownership of the object. After calling this
> > +    /// function, the caller is responsible for ownership previously managed
> > +    /// by the [`Owned`].
> > +    pub fn into_raw(me: Self) -> NonNull<T> {
> > +        ManuallyDrop::new(me).ptr
> > +    }
> > +}
> > +
> > +impl<T: Ownable> Deref for Owned<T> {
> > +    type Target = T;
> > +
> > +    fn deref(&self) -> &Self::Target {
> > +        // SAFETY: The type invariants guarantee that the object is valid.
> > +        unsafe { self.ptr.as_ref() }
> > +    }
> > +}
> > +
> > +impl<T: OwnableMut> DerefMut for Owned<T> {
> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> > +        // return a mutable reference to it.
> > +        unsafe { self.ptr.as_mut() }
> > +    }
> > +}
> 
> I think someone mentioned this before, but handing out mutable
> references can be a problem if `T: !Unpin`. For instance, we don't want
> to hand out `&mut Page` in case of `Owned<Page>`.
>

That was the reason, why `OwnableMut` was introduced in the first place.
It's clear, I guess, that as-is it cannot be implemented on many classes.

> Could we do something like this:
> 
>   diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
>   index 29729dc10cb4..52d21e41c26b 100644
>   --- a/rust/kernel/types/ownable.rs
>   +++ b/rust/kernel/types/ownable.rs
>   @@ -3,6 +3,7 @@
>   //! Owned reference types.
> 
>   use crate::{
>   +    prelude::*,
>       sync::aref::{ARef, RefCounted},
>       types::ForeignOwnable,
>   };
>   @@ -112,6 +113,17 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>       pub fn into_raw(me: Self) -> NonNull<T> {
>           ManuallyDrop::new(me).ptr
>       }
>   +
>   +    /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
>   +    pub fn get_pin_mut(&mut self) -> Pin<&mut T> {
>   +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>   +        // return a mutable reference to it.
>   +        let unpinned = unsafe { self.ptr.as_mut() };
>   +
>   +        // SAFETY: We never hand out unpinned mutable references to the data in
>   +        // `Self`, unless the contained type is `Unpin`.
>   +        unsafe { Pin::new_unchecked(unpinned) }
>   +    }
>   }
> 
>   impl<T: Ownable> Deref for Owned<T> {
>   @@ -123,7 +135,7 @@ fn deref(&self) -> &Self::Target {
>       }
>   }
> 
>   -impl<T: OwnableMut> DerefMut for Owned<T> {
>   +impl<T: OwnableMut + Unpin> DerefMut for Owned<T> {
>       fn deref_mut(&mut self) -> &mut Self::Target {
>           // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>           // return a mutable reference to it.
> 

Good question, I have been thinking about it, too. But it might
be, that it isn't needed at all. As I understand, usually Rust wrappers
are around non-movable C structs. Do we actually have a useful application
for OwnableMut?

Best,

Oliver

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

[Benno Lossin]

On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
> On 250818 1446, Andreas Hindborg wrote:
>> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> > +        // return a mutable reference to it.
>> > +        unsafe { self.ptr.as_mut() }
>> > +    }
>> > +}
>> 
>> I think someone mentioned this before, but handing out mutable
>> references can be a problem if `T: !Unpin`. For instance, we don't want
>> to hand out `&mut Page` in case of `Owned<Page>`.
>>
>
> That was the reason, why `OwnableMut` was introduced in the first place.
> It's clear, I guess, that as-is it cannot be implemented on many classes.

Yeah the safety requirements ensure that you can't implement it on
`!Unpin` types.

But I'm not sure it's useful then? As you said there aren't many types
that will implement the type then, so how about we change the meaning
and make it give out a pinned mutable reference instead?

> Good question, I have been thinking about it, too. But it might
> be, that it isn't needed at all. As I understand, usually Rust wrappers
> are around non-movable C structs. Do we actually have a useful application
> for OwnableMut?

Also, do we even need two different traits? Which types would only
implement `Ownable` but not `OwnableMut`?

---
Cheers,
Benno

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

[Oliver Mangold]

On 250819 0027, Benno Lossin wrote:
> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
> > On 250818 1446, Andreas Hindborg wrote:
> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> >> > +        // return a mutable reference to it.
> >> > +        unsafe { self.ptr.as_mut() }
> >> > +    }
> >> > +}
> >>
> >> I think someone mentioned this before, but handing out mutable
> >> references can be a problem if `T: !Unpin`. For instance, we don't want
> >> to hand out `&mut Page` in case of `Owned<Page>`.
> >>
> >
> > That was the reason, why `OwnableMut` was introduced in the first place.
> > It's clear, I guess, that as-is it cannot be implemented on many classes.
> 
> Yeah the safety requirements ensure that you can't implement it on
> `!Unpin` types.
> 
> But I'm not sure it's useful then? As you said there aren't many types
> that will implement the type then, so how about we change the meaning
> and make it give out a pinned mutable reference instead?

Making `deref_mut()` give out a pinned type won't work. The return types of
deref() are required to match.

> > Good question, I have been thinking about it, too. But it might
> > be, that it isn't needed at all. As I understand, usually Rust wrappers
> > are around non-movable C structs. Do we actually have a useful application
> > for OwnableMut?
> 
> Also, do we even need two different traits? Which types would only
> implement `Ownable` but not `OwnableMut`?

I'm not 100% sure, but on a quick glance it looks indeed be safe to
substitute `OwnableMut` by `Unpin`.

If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.

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

[Benno Lossin]

On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
> On 250819 0027, Benno Lossin wrote:
>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>> > On 250818 1446, Andreas Hindborg wrote:
>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> >> > +        // return a mutable reference to it.
>> >> > +        unsafe { self.ptr.as_mut() }
>> >> > +    }
>> >> > +}
>> >>
>> >> I think someone mentioned this before, but handing out mutable
>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>> >>
>> >
>> > That was the reason, why `OwnableMut` was introduced in the first place.
>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>> 
>> Yeah the safety requirements ensure that you can't implement it on
>> `!Unpin` types.
>> 
>> But I'm not sure it's useful then? As you said there aren't many types
>> that will implement the type then, so how about we change the meaning
>> and make it give out a pinned mutable reference instead?
>
> Making `deref_mut()` give out a pinned type won't work. The return types of
> deref() are required to match.

I meant the changes that Andreas suggested.

>> > Good question, I have been thinking about it, too. But it might
>> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>> > are around non-movable C structs. Do we actually have a useful application
>> > for OwnableMut?
>> 
>> Also, do we even need two different traits? Which types would only
>> implement `Ownable` but not `OwnableMut`?
>
> I'm not 100% sure, but on a quick glance it looks indeed be safe to
> substitute `OwnableMut` by `Unpin`.

We just have to change the safety requirements of `OwnableMut`.

> If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
> it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.

Well the `DerefMut` impl still is convenient in the `Unpin` case.

---
Cheers,
Benno

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

[Oliver Mangold]

On 250819 1026, Benno Lossin wrote:
> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
> > On 250819 0027, Benno Lossin wrote:
> >> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
> >> > On 250818 1446, Andreas Hindborg wrote:
> >> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> >> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
> >> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> >> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> >> >> > +        // return a mutable reference to it.
> >> >> > +        unsafe { self.ptr.as_mut() }
> >> >> > +    }
> >> >> > +}
> >> >>
> >> >> I think someone mentioned this before, but handing out mutable
> >> >> references can be a problem if `T: !Unpin`. For instance, we don't want
> >> >> to hand out `&mut Page` in case of `Owned<Page>`.
> >> >>
> >> >
> >> > That was the reason, why `OwnableMut` was introduced in the first place.
> >> > It's clear, I guess, that as-is it cannot be implemented on many classes.
> >>
> >> Yeah the safety requirements ensure that you can't implement it on
> >> `!Unpin` types.
> >>
> >> But I'm not sure it's useful then? As you said there aren't many types
> >> that will implement the type then, so how about we change the meaning
> >> and make it give out a pinned mutable reference instead?
> >
> > Making `deref_mut()` give out a pinned type won't work. The return types of
> > deref() are required to match.
> 
> I meant the changes that Andreas suggested.
> 
> >> > Good question, I have been thinking about it, too. But it might
> >> > be, that it isn't needed at all. As I understand, usually Rust wrappers
> >> > are around non-movable C structs. Do we actually have a useful application
> >> > for OwnableMut?
> >>
> >> Also, do we even need two different traits? Which types would only
> >> implement `Ownable` but not `OwnableMut`?
> >
> > I'm not 100% sure, but on a quick glance it looks indeed be safe to
> > substitute `OwnableMut` by `Unpin`.
> 
> We just have to change the safety requirements of `OwnableMut`.

You mean of `Ownable`, when `OwnableMut` is removed? Yes. A good question
in that context is, what it actually means to have an `&mut Opaque<T>`
where `T` is `Unpin`. If that implies being allowed to obtain an `&mut T`,
it would we easy, I guess.

> > If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
> > it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
> 
> Well the `DerefMut` impl still is convenient in the `Unpin` case.

I agree. What I meant is, it could not introduce an extra safety
requirement having it, if that indirect method can be used anyway.

But what I am wondering is, if we actually want to start using `Pin`
at all. Isn't `Opaque` currently used about everywhere pinning is needed?

Best,

Oliver

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

[Andreas Hindborg]

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> On 250819 1026, Benno Lossin wrote:
>> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>> > On 250819 0027, Benno Lossin wrote:
>> >> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>> >> > On 250818 1446, Andreas Hindborg wrote:
>> >> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> >> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>> >> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>> >> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> >> >> > +        // return a mutable reference to it.
>> >> >> > +        unsafe { self.ptr.as_mut() }
>> >> >> > +    }
>> >> >> > +}
>> >> >>
>> >> >> I think someone mentioned this before, but handing out mutable
>> >> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>> >> >> to hand out `&mut Page` in case of `Owned<Page>`.
>> >> >>
>> >> >
>> >> > That was the reason, why `OwnableMut` was introduced in the first place.
>> >> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>> >>
>> >> Yeah the safety requirements ensure that you can't implement it on
>> >> `!Unpin` types.
>> >>
>> >> But I'm not sure it's useful then? As you said there aren't many types
>> >> that will implement the type then, so how about we change the meaning
>> >> and make it give out a pinned mutable reference instead?
>> >
>> > Making `deref_mut()` give out a pinned type won't work. The return types of
>> > deref() are required to match.
>>
>> I meant the changes that Andreas suggested.
>>
>> >> > Good question, I have been thinking about it, too. But it might
>> >> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>> >> > are around non-movable C structs. Do we actually have a useful application
>> >> > for OwnableMut?
>> >>
>> >> Also, do we even need two different traits? Which types would only
>> >> implement `Ownable` but not `OwnableMut`?
>> >
>> > I'm not 100% sure, but on a quick glance it looks indeed be safe to
>> > substitute `OwnableMut` by `Unpin`.
>>
>> We just have to change the safety requirements of `OwnableMut`.
>
> You mean of `Ownable`, when `OwnableMut` is removed? Yes. A good question
> in that context is, what it actually means to have an `&mut Opaque<T>`
> where `T` is `Unpin`. If that implies being allowed to obtain an `&mut T`,
> it would we easy, I guess.

You should not be able to get a `&mut T` from a `&mut Opaque<T>`.
`Opaque` opts out of invariants that normally hold for rust references.

>
>> > If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
>> > it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
>>
>> Well the `DerefMut` impl still is convenient in the `Unpin` case.
>
> I agree. What I meant is, it could not introduce an extra safety
> requirement having it, if that indirect method can be used anyway.

As I mention in my other email, I think we can still have `OwnableMut`
if we add a trait bound on `Unpin`.

>
> But what I am wondering is, if we actually want to start using `Pin`
> at all. Isn't `Opaque` currently used about everywhere pinning is needed?

`Opaque` is `!Unpin`, but pinning guarantees does not come into effect
until we produce a `Pin<Opaque<T>>`.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Tue Aug 19, 2025 at 11:00 AM CEST, Andreas Hindborg wrote:
> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> You mean of `Ownable`, when `OwnableMut` is removed? Yes. A good question
>> in that context is, what it actually means to have an `&mut Opaque<T>`
>> where `T` is `Unpin`. If that implies being allowed to obtain an `&mut T`,
>> it would we easy, I guess.
>
> You should not be able to get a `&mut T` from a `&mut Opaque<T>`.
> `Opaque` opts out of invariants that normally hold for rust references.

Yes, that function mustn't exist.

>> But what I am wondering is, if we actually want to start using `Pin`
>> at all. Isn't `Opaque` currently used about everywhere pinning is needed?
>
> `Opaque` is `!Unpin`, but pinning guarantees does not come into effect
> until we produce a `Pin<Opaque<T>>`.

`Pin<Opaque<T>>` isn't really a thing. You still need a pointer
indirection, so `Pin<P>` where `P: DerefMut<Target = Opaque<T>>` so for
example `Pin<Box<Opaque<T>>>`.

---
Cheers,
Benno

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

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Aug 19, 2025 at 11:00 AM CEST, Andreas Hindborg wrote:
>> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>>> You mean of `Ownable`, when `OwnableMut` is removed? Yes. A good question
>>> in that context is, what it actually means to have an `&mut Opaque<T>`
>>> where `T` is `Unpin`. If that implies being allowed to obtain an `&mut T`,
>>> it would we easy, I guess.
>>
>> You should not be able to get a `&mut T` from a `&mut Opaque<T>`.
>> `Opaque` opts out of invariants that normally hold for rust references.
>
> Yes, that function mustn't exist.
>
>>> But what I am wondering is, if we actually want to start using `Pin`
>>> at all. Isn't `Opaque` currently used about everywhere pinning is needed?
>>
>> `Opaque` is `!Unpin`, but pinning guarantees does not come into effect
>> until we produce a `Pin<Opaque<T>>`.
>
> `Pin<Opaque<T>>` isn't really a thing. You still need a pointer
> indirection, so `Pin<P>` where `P: DerefMut<Target = Opaque<T>>` so for
> example `Pin<Box<Opaque<T>>>`.

Right, that is what I meant, but not what I typed. Thanks for pointing
that out :)


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>> On 250819 0027, Benno Lossin wrote:
>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>>> > On 250818 1446, Andreas Hindborg wrote:
>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>>> >> > +        // return a mutable reference to it.
>>> >> > +        unsafe { self.ptr.as_mut() }
>>> >> > +    }
>>> >> > +}
>>> >>
>>> >> I think someone mentioned this before, but handing out mutable
>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>>> >>
>>> >
>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>>>
>>> Yeah the safety requirements ensure that you can't implement it on
>>> `!Unpin` types.
>>>
>>> But I'm not sure it's useful then? As you said there aren't many types
>>> that will implement the type then, so how about we change the meaning
>>> and make it give out a pinned mutable reference instead?
>>
>> Making `deref_mut()` give out a pinned type won't work. The return types of
>> deref() are required to match.
>
> I meant the changes that Andreas suggested.

Not sure what you are asking, but I need to assert exclusive access to
an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
`Pin<&mut Page>`. I think the latter is more agnostic.

>
>>> > Good question, I have been thinking about it, too. But it might
>>> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>>> > are around non-movable C structs. Do we actually have a useful application
>>> > for OwnableMut?
>>>
>>> Also, do we even need two different traits? Which types would only
>>> implement `Ownable` but not `OwnableMut`?
>>
>> I'm not 100% sure, but on a quick glance it looks indeed be safe to
>> substitute `OwnableMut` by `Unpin`.
>
> We just have to change the safety requirements of `OwnableMut`.

`OwnableMut` already requires `Unpin`, it just does not say so directly:


/// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
///   (i.e. most kernel types).

We could remove this and then just add a trait bound on `Unpin`.

>
>> If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
>> it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
>
> Well the `DerefMut` impl still is convenient in the `Unpin` case.

`OwnableMut` is probably not that useful, since all the types we want to
implement `Ownable` for is `!Unpin`. We could remove it, but I felt it
was neat to add the `DerefMut` impl for `Unpin` types.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>>> On 250819 0027, Benno Lossin wrote:
>>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>>>> > On 250818 1446, Andreas Hindborg wrote:
>>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>>>> >> > +        // return a mutable reference to it.
>>>> >> > +        unsafe { self.ptr.as_mut() }
>>>> >> > +    }
>>>> >> > +}
>>>> >>
>>>> >> I think someone mentioned this before, but handing out mutable
>>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>>>> >>
>>>> >
>>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>>>>
>>>> Yeah the safety requirements ensure that you can't implement it on
>>>> `!Unpin` types.
>>>>
>>>> But I'm not sure it's useful then? As you said there aren't many types
>>>> that will implement the type then, so how about we change the meaning
>>>> and make it give out a pinned mutable reference instead?
>>>
>>> Making `deref_mut()` give out a pinned type won't work. The return types of
>>> deref() are required to match.
>>
>> I meant the changes that Andreas suggested.
>
> Not sure what you are asking, but I need to assert exclusive access to
> an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
> `Pin<&mut Page>`. I think the latter is more agnostic.

The former isn't really correct? It's like having a `&mut Box<Page>`
which is weird. I was saying we can have a `DerefMut` impl gated on `T:
Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.

>>>> > Good question, I have been thinking about it, too. But it might
>>>> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>>>> > are around non-movable C structs. Do we actually have a useful application
>>>> > for OwnableMut?
>>>>
>>>> Also, do we even need two different traits? Which types would only
>>>> implement `Ownable` but not `OwnableMut`?
>>>
>>> I'm not 100% sure, but on a quick glance it looks indeed be safe to
>>> substitute `OwnableMut` by `Unpin`.
>>
>> We just have to change the safety requirements of `OwnableMut`.
>
> `OwnableMut` already requires `Unpin`, it just does not say so directly:
>
>
> /// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
> ///   (i.e. most kernel types).
>
> We could remove this and then just add a trait bound on `Unpin`.

Oh I happened to not have read it that thoroughly then... I don't think
it makes sense to have `OwnableMut` then. So I agree with your suggested
change :)

>>> If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
>>> it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
>>
>> Well the `DerefMut` impl still is convenient in the `Unpin` case.
>
> `OwnableMut` is probably not that useful, since all the types we want to
> implement `Ownable` for is `!Unpin`. We could remove it, but I felt it
> was neat to add the `DerefMut` impl for `Unpin` types.

But we don't need `OwnableMut` in that case?

Let's just do the following if it makes sense:
* remove `OwnableMut`
* allow obtaining a `Pin<&mut T>` from `Owned<T>` via a `&mut self`
  method (we need a new safety requirement for this on `Ownable`)
* have the `DerefMut` impl require `T: Unpin`

---
Cheers,
Benno

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

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>>>> On 250819 0027, Benno Lossin wrote:
>>>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>>>>> > On 250818 1446, Andreas Hindborg wrote:
>>>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>>>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>>>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>>>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>>>>> >> > +        // return a mutable reference to it.
>>>>> >> > +        unsafe { self.ptr.as_mut() }
>>>>> >> > +    }
>>>>> >> > +}
>>>>> >>
>>>>> >> I think someone mentioned this before, but handing out mutable
>>>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>>>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>>>>> >>
>>>>> >
>>>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>>>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>>>>>
>>>>> Yeah the safety requirements ensure that you can't implement it on
>>>>> `!Unpin` types.
>>>>>
>>>>> But I'm not sure it's useful then? As you said there aren't many types
>>>>> that will implement the type then, so how about we change the meaning
>>>>> and make it give out a pinned mutable reference instead?
>>>>
>>>> Making `deref_mut()` give out a pinned type won't work. The return types of
>>>> deref() are required to match.
>>>
>>> I meant the changes that Andreas suggested.
>>
>> Not sure what you are asking, but I need to assert exclusive access to
>> an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
>> `Pin<&mut Page>`. I think the latter is more agnostic.
>
> The former isn't really correct? It's like having a `&mut Box<Page>`
> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.
>
>>>>> > Good question, I have been thinking about it, too. But it might
>>>>> > be, that it isn't needed at all. As I understand, usually Rust wrappers
>>>>> > are around non-movable C structs. Do we actually have a useful application
>>>>> > for OwnableMut?
>>>>>
>>>>> Also, do we even need two different traits? Which types would only
>>>>> implement `Ownable` but not `OwnableMut`?
>>>>
>>>> I'm not 100% sure, but on a quick glance it looks indeed be safe to
>>>> substitute `OwnableMut` by `Unpin`.
>>>
>>> We just have to change the safety requirements of `OwnableMut`.
>>
>> `OwnableMut` already requires `Unpin`, it just does not say so directly:
>>
>>
>> /// - It is safe to call [`core::mem::swap`] on the [`Ownable`]. This excludes pinned types
>> ///   (i.e. most kernel types).
>>
>> We could remove this and then just add a trait bound on `Unpin`.
>
> Oh I happened to not have read it that thoroughly then... I don't think
> it makes sense to have `OwnableMut` then. So I agree with your suggested
> change :)
>
>>>> If we add `get_pin_mut(&mut self) -> Pin<&mut T>` as Andreas suggested,
>>>> it would be possible to obtain an `&mut T` anyway, then, if T is `Unpin`.
>>>
>>> Well the `DerefMut` impl still is convenient in the `Unpin` case.
>>
>> `OwnableMut` is probably not that useful, since all the types we want to
>> implement `Ownable` for is `!Unpin`. We could remove it, but I felt it
>> was neat to add the `DerefMut` impl for `Unpin` types.
>
> But we don't need `OwnableMut` in that case?

Right, we can directly limit the `DerefMut` impl.

>
> Let's just do the following if it makes sense:
> * remove `OwnableMut`
> * allow obtaining a `Pin<&mut T>` from `Owned<T>` via a `&mut self`
>   method (we need a new safety requirement for this on `Ownable`)
> * have the `DerefMut` impl require `T: Unpin`

Sounds good to me 👍


Best regards,
Andreas Hindborg

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

[Oliver Mangold]

On 250819 1913, Benno Lossin wrote:
> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
> > "Benno Lossin" <lossin@kernel.org> writes:
> >> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
> >>> On 250819 0027, Benno Lossin wrote:
> >>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
> >>>> > On 250818 1446, Andreas Hindborg wrote:
> >>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> >>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
> >>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> >>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> >>>> >> > +        // return a mutable reference to it.
> >>>> >> > +        unsafe { self.ptr.as_mut() }
> >>>> >> > +    }
> >>>> >> > +}
> >>>> >>
> >>>> >> I think someone mentioned this before, but handing out mutable
> >>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
> >>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
> >>>> >>
> >>>> >
> >>>> > That was the reason, why `OwnableMut` was introduced in the first place.
> >>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
> >>>>
> >>>> Yeah the safety requirements ensure that you can't implement it on
> >>>> `!Unpin` types.
> >>>>
> >>>> But I'm not sure it's useful then? As you said there aren't many types
> >>>> that will implement the type then, so how about we change the meaning
> >>>> and make it give out a pinned mutable reference instead?
> >>>
> >>> Making `deref_mut()` give out a pinned type won't work. The return types of
> >>> deref() are required to match.
> >>
> >> I meant the changes that Andreas suggested.
> >
> > Not sure what you are asking, but I need to assert exclusive access to
> > an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
> > `Pin<&mut Page>`. I think the latter is more agnostic.
> 
> The former isn't really correct? It's like having a `&mut Box<Page>`
> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.

Yes. I think `Page` is the wrong example, as it already has owned semantics
and does its own cleanup. Wrapping it in an Owned would be redundant.

Best,

Oliver

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

[Benno Lossin]

On Wed Aug 20, 2025 at 8:02 AM CEST, Oliver Mangold wrote:
> On 250819 1913, Benno Lossin wrote:
>> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
>> > "Benno Lossin" <lossin@kernel.org> writes:
>> >> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>> >>> On 250819 0027, Benno Lossin wrote:
>> >>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>> >>>> > On 250818 1446, Andreas Hindborg wrote:
>> >>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> >>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>> >>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>> >>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> >>>> >> > +        // return a mutable reference to it.
>> >>>> >> > +        unsafe { self.ptr.as_mut() }
>> >>>> >> > +    }
>> >>>> >> > +}
>> >>>> >>
>> >>>> >> I think someone mentioned this before, but handing out mutable
>> >>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>> >>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>> >>>> >>
>> >>>> >
>> >>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>> >>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>> >>>>
>> >>>> Yeah the safety requirements ensure that you can't implement it on
>> >>>> `!Unpin` types.
>> >>>>
>> >>>> But I'm not sure it's useful then? As you said there aren't many types
>> >>>> that will implement the type then, so how about we change the meaning
>> >>>> and make it give out a pinned mutable reference instead?
>> >>>
>> >>> Making `deref_mut()` give out a pinned type won't work. The return types of
>> >>> deref() are required to match.
>> >>
>> >> I meant the changes that Andreas suggested.
>> >
>> > Not sure what you are asking, but I need to assert exclusive access to
>> > an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
>> > `Pin<&mut Page>`. I think the latter is more agnostic.
>> 
>> The former isn't really correct? It's like having a `&mut Box<Page>`
>> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
>> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.
>
> Yes. I think `Page` is the wrong example, as it already has owned semantics
> and does its own cleanup. Wrapping it in an Owned would be redundant.

After we have these owned patches, we are going to change `Page` to
`Opaque<bindings::page>`.

---
Cheers,
Benno

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

[Oliver Mangold]

On 250820 0941, Benno Lossin wrote:
> On Wed Aug 20, 2025 at 8:02 AM CEST, Oliver Mangold wrote:
> > On 250819 1913, Benno Lossin wrote:
> >> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
> >> > "Benno Lossin" <lossin@kernel.org> writes:
> >> >> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
> >> >>> On 250819 0027, Benno Lossin wrote:
> >> >>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
> >> >>>> > On 250818 1446, Andreas Hindborg wrote:
> >> >>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> >> >>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
> >> >>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
> >> >>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
> >> >>>> >> > +        // return a mutable reference to it.
> >> >>>> >> > +        unsafe { self.ptr.as_mut() }
> >> >>>> >> > +    }
> >> >>>> >> > +}
> >> >>>> >>
> >> >>>> >> I think someone mentioned this before, but handing out mutable
> >> >>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
> >> >>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
> >> >>>> >>
> >> >>>> >
> >> >>>> > That was the reason, why `OwnableMut` was introduced in the first place.
> >> >>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
> >> >>>>
> >> >>>> Yeah the safety requirements ensure that you can't implement it on
> >> >>>> `!Unpin` types.
> >> >>>>
> >> >>>> But I'm not sure it's useful then? As you said there aren't many types
> >> >>>> that will implement the type then, so how about we change the meaning
> >> >>>> and make it give out a pinned mutable reference instead?
> >> >>>
> >> >>> Making `deref_mut()` give out a pinned type won't work. The return types of
> >> >>> deref() are required to match.
> >> >>
> >> >> I meant the changes that Andreas suggested.
> >> >
> >> > Not sure what you are asking, but I need to assert exclusive access to
> >> > an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
> >> > `Pin<&mut Page>`. I think the latter is more agnostic.
> >>
> >> The former isn't really correct? It's like having a `&mut Box<Page>`
> >> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
> >> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.
> >
> > Yes. I think `Page` is the wrong example, as it already has owned semantics
> > and does its own cleanup. Wrapping it in an Owned would be redundant.
> 
> After we have these owned patches, we are going to change `Page` to
> `Opaque<bindings::page>`.

Ah, okay. Makes sense, I guess.

Best,

Oliver

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

[Andreas Hindborg]

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> On 250820 0941, Benno Lossin wrote:
>> On Wed Aug 20, 2025 at 8:02 AM CEST, Oliver Mangold wrote:
>> > On 250819 1913, Benno Lossin wrote:
>> >> On Tue Aug 19, 2025 at 10:53 AM CEST, Andreas Hindborg wrote:
>> >> > "Benno Lossin" <lossin@kernel.org> writes:
>> >> >> On Tue Aug 19, 2025 at 8:04 AM CEST, Oliver Mangold wrote:
>> >> >>> On 250819 0027, Benno Lossin wrote:
>> >> >>>> On Mon Aug 18, 2025 at 3:04 PM CEST, Oliver Mangold wrote:
>> >> >>>> > On 250818 1446, Andreas Hindborg wrote:
>> >> >>>> >> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> >> >>>> >> > +impl<T: OwnableMut> DerefMut for Owned<T> {
>> >> >>>> >> > +    fn deref_mut(&mut self) -> &mut Self::Target {
>> >> >>>> >> > +        // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> >> >>>> >> > +        // return a mutable reference to it.
>> >> >>>> >> > +        unsafe { self.ptr.as_mut() }
>> >> >>>> >> > +    }
>> >> >>>> >> > +}
>> >> >>>> >>
>> >> >>>> >> I think someone mentioned this before, but handing out mutable
>> >> >>>> >> references can be a problem if `T: !Unpin`. For instance, we don't want
>> >> >>>> >> to hand out `&mut Page` in case of `Owned<Page>`.
>> >> >>>> >>
>> >> >>>> >
>> >> >>>> > That was the reason, why `OwnableMut` was introduced in the first place.
>> >> >>>> > It's clear, I guess, that as-is it cannot be implemented on many classes.
>> >> >>>>
>> >> >>>> Yeah the safety requirements ensure that you can't implement it on
>> >> >>>> `!Unpin` types.
>> >> >>>>
>> >> >>>> But I'm not sure it's useful then? As you said there aren't many types
>> >> >>>> that will implement the type then, so how about we change the meaning
>> >> >>>> and make it give out a pinned mutable reference instead?
>> >> >>>
>> >> >>> Making `deref_mut()` give out a pinned type won't work. The return types of
>> >> >>> deref() are required to match.
>> >> >>
>> >> >> I meant the changes that Andreas suggested.
>> >> >
>> >> > Not sure what you are asking, but I need to assert exclusive access to
>> >> > an `Page`. I could either get this by taking a `&mut Owned<Page>` or a
>> >> > `Pin<&mut Page>`. I think the latter is more agnostic.
>> >>
>> >> The former isn't really correct? It's like having a `&mut Box<Page>`
>> >> which is weird. I was saying we can have a `DerefMut` impl gated on `T:
>> >> Unpin` and a `fn get_pin_mut(&mut self) -> Pin<&mut T>`.
>> >
>> > Yes. I think `Page` is the wrong example, as it already has owned semantics
>> > and does its own cleanup. Wrapping it in an Owned would be redundant.
>>
>> After we have these owned patches, we are going to change `Page` to
>> `Opaque<bindings::page>`.
>
> Ah, okay. Makes sense, I guess.

I applied Linas patch with this change to my tree [1].

Best regards,
Andreas Hindborg


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

[PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

[Oliver Mangold]

`AlwaysRefCounted` is renamed to `RefCounted`.

`AlwaysRefCounted` will become a marker trait to indicate that it is
allowed to obtain an `ARef<T>` from a `&T`, which cannot be allowed for
types which are also Ownable.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
Suggested-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/block/mq/request.rs | 15 +++++++++------
 rust/kernel/cred.rs             |  8 ++++++--
 rust/kernel/device.rs           |  8 ++++++--
 rust/kernel/fs/file.rs          | 10 +++++++---
 rust/kernel/mm.rs               | 13 ++++++++++---
 rust/kernel/mm/mmput_async.rs   |  9 +++++++--
 rust/kernel/opp.rs              |  8 ++++++--
 rust/kernel/pci.rs              |  6 +++++-
 rust/kernel/pid_namespace.rs    |  8 ++++++--
 rust/kernel/platform.rs         |  6 +++++-
 rust/kernel/task.rs             |  6 +++++-
 rust/kernel/types.rs            | 41 ++++++++++++++++++++++++-----------------
 rust/kernel/types/ownable.rs    |  4 ++--
 13 files changed, 98 insertions(+), 44 deletions(-)

diff --git a/rust/kernel/block/mq/request.rs b/rust/kernel/block/mq/request.rs
index 4a5b7ec914efa598c65881b07c4ece59214fd7e7..ca02d07f13ade252bc3b4d2ca3e5e21a16a7288e 100644
--- a/rust/kernel/block/mq/request.rs
+++ b/rust/kernel/block/mq/request.rs
@@ -8,7 +8,7 @@
     bindings,
     block::mq::Operations,
     error::Result,
-    types::{ARef, AlwaysRefCounted, Opaque},
+    types::{ARef, AlwaysRefCounted, Opaque, RefCounted},
 };
 use core::{
     marker::PhantomData,
@@ -226,11 +226,10 @@ fn atomic_relaxed_op_unless(target: &AtomicU64, op: impl Fn(u64) -> u64, pred: u
         .is_ok()
 }
 
-// SAFETY: All instances of `Request<T>` are reference counted. This
-// implementation of `AlwaysRefCounted` ensure that increments to the ref count
-// keeps the object alive in memory at least until a matching reference count
-// decrement is executed.
-unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {
+// SAFETY: All instances of `Request<T>` are reference counted. This implementation of `RefCounted`
+// ensure that increments to the ref count keeps the object alive in memory at least until a
+// matching reference count decrement is executed.
+unsafe impl<T: Operations> RefCounted for Request<T> {
     fn inc_ref(&self) {
         let refcount = &self.wrapper_ref().refcount();
 
@@ -260,3 +259,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
         }
     }
 }
+
+// SAFETY: We currently do not implement `Ownable`, thus it is okay to can obtain an `ARef<Request>`
+// from a `&Request` (but this will change in the future).
+unsafe impl<T: Operations> AlwaysRefCounted for Request<T> {}
diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs
index 2599f01e8b285f2106aefd27c315ae2aff25293c..87fa2808050dd8a2838a0f5c21cd7f567ba6b534 100644
--- a/rust/kernel/cred.rs
+++ b/rust/kernel/cred.rs
@@ -11,7 +11,7 @@
 use crate::{
     bindings,
     task::Kuid,
-    types::{AlwaysRefCounted, Opaque},
+    types::{AlwaysRefCounted, Opaque, RefCounted},
 };
 
 /// Wraps the kernel's `struct cred`.
@@ -74,7 +74,7 @@ pub fn euid(&self) -> Kuid {
 }
 
 // SAFETY: The type invariants guarantee that `Credential` is always ref-counted.
-unsafe impl AlwaysRefCounted for Credential {
+unsafe impl RefCounted for Credential {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
@@ -88,3 +88,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Credential>) {
         unsafe { bindings::put_cred(obj.cast().as_ptr()) };
     }
 }
+
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<Credential>` from a
+// `&Credential`.
+unsafe impl AlwaysRefCounted for Credential {}
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index dea06b79ecb536cee4d2b90c21b74658658417c7..afddb4d70d2f375c891facac7f83501cd9918f54 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -7,7 +7,7 @@
 use crate::{
     bindings,
     str::CStr,
-    types::{ARef, Opaque},
+    types::{ARef, AlwaysRefCounted, Opaque, RefCounted},
 };
 use core::{fmt, marker::PhantomData, ptr};
 
@@ -216,7 +216,7 @@ pub fn property_present(&self, name: &CStr) -> bool {
 kernel::impl_device_context_into_aref!(Device);
 
 // SAFETY: Instances of `Device` are always reference-counted.
-unsafe impl crate::types::AlwaysRefCounted for Device {
+unsafe impl RefCounted for Device {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
         unsafe { bindings::get_device(self.as_raw()) };
@@ -228,6 +228,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `Device<Task>` from a
+// `&Device`.
+unsafe impl AlwaysRefCounted for Device {}
+
 // SAFETY: As by the type invariant `Device` can be sent to any thread.
 unsafe impl Send for Device {}
 
diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
index 72d84fb0e2664643619ad7fbcbbb55b3adc9f9b4..489f6d1f17508af7e064e3f506349797fff497ae 100644
--- a/rust/kernel/fs/file.rs
+++ b/rust/kernel/fs/file.rs
@@ -11,7 +11,7 @@
     bindings,
     cred::Credential,
     error::{code::*, Error, Result},
-    types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
+    types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque, RefCounted},
 };
 use core::ptr;
 
@@ -190,7 +190,7 @@ unsafe impl Sync for File {}
 
 // SAFETY: The type invariants guarantee that `File` is always ref-counted. This implementation
 // makes `ARef<File>` own a normal refcount.
-unsafe impl AlwaysRefCounted for File {
+unsafe impl RefCounted for File {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
@@ -205,6 +205,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<File>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<File>` from a
+// `&File`.
+unsafe impl AlwaysRefCounted for File {}
+
 /// Wraps the kernel's `struct file`. Not thread safe.
 ///
 /// This type represents a file that is not known to be safe to transfer across thread boundaries.
@@ -226,7 +230,7 @@ pub struct LocalFile {
 
 // SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
 // makes `ARef<LocalFile>` own a normal refcount.
-unsafe impl AlwaysRefCounted for LocalFile {
+unsafe impl RefCounted for LocalFile {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
diff --git a/rust/kernel/mm.rs b/rust/kernel/mm.rs
index 43f525c0d16ce87340ba4f991c45d4e82a050eae..9bbb317896eaa06f0e654873993ae9ff531d4c61 100644
--- a/rust/kernel/mm.rs
+++ b/rust/kernel/mm.rs
@@ -13,7 +13,7 @@
 
 use crate::{
     bindings,
-    types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque},
+    types::{ARef, AlwaysRefCounted, NotThreadSafe, Opaque, RefCounted},
 };
 use core::{ops::Deref, ptr::NonNull};
 
@@ -54,7 +54,7 @@ unsafe impl Send for Mm {}
 unsafe impl Sync for Mm {}
 
 // SAFETY: By the type invariants, this type is always refcounted.
-unsafe impl AlwaysRefCounted for Mm {
+unsafe impl RefCounted for Mm {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The pointer is valid since self is a reference.
@@ -68,6 +68,9 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<Mm>` from a `&Mm`.
+unsafe impl AlwaysRefCounted for Mm {}
+
 /// A wrapper for the kernel's `struct mm_struct`.
 ///
 /// This type is like [`Mm`], but with non-zero `mm_users`. It can only be used when `mm_users` can
@@ -90,7 +93,7 @@ unsafe impl Send for MmWithUser {}
 unsafe impl Sync for MmWithUser {}
 
 // SAFETY: By the type invariants, this type is always refcounted.
-unsafe impl AlwaysRefCounted for MmWithUser {
+unsafe impl RefCounted for MmWithUser {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The pointer is valid since self is a reference.
@@ -104,6 +107,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<MmWithUser>` from a
+// `&MmWithUser`.
+unsafe impl AlwaysRefCounted for MmWithUser {}
+
 // Make all `Mm` methods available on `MmWithUser`.
 impl Deref for MmWithUser {
     type Target = Mm;
diff --git a/rust/kernel/mm/mmput_async.rs b/rust/kernel/mm/mmput_async.rs
index 9289e05f7a676b577e4edf45949c0fab6aacec14..1b8c5cc32123ed406079aa1505e623ea6af81011 100644
--- a/rust/kernel/mm/mmput_async.rs
+++ b/rust/kernel/mm/mmput_async.rs
@@ -10,7 +10,7 @@
 use crate::{
     bindings,
     mm::MmWithUser,
-    types::{ARef, AlwaysRefCounted},
+    types::{ARef, AlwaysRefCounted, RefCounted},
 };
 use core::{ops::Deref, ptr::NonNull};
 
@@ -34,7 +34,7 @@ unsafe impl Send for MmWithUserAsync {}
 unsafe impl Sync for MmWithUserAsync {}
 
 // SAFETY: By the type invariants, this type is always refcounted.
-unsafe impl AlwaysRefCounted for MmWithUserAsync {
+unsafe impl RefCounted for MmWithUserAsync {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The pointer is valid since self is a reference.
@@ -48,6 +48,11 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<MmWithUserAsync>`
+// from a `&MmWithUserAsync`.
+unsafe impl AlwaysRefCounted for MmWithUserAsync {}
+
 // Make all `MmWithUser` methods available on `MmWithUserAsync`.
 impl Deref for MmWithUserAsync {
     type Target = MmWithUser;
diff --git a/rust/kernel/opp.rs b/rust/kernel/opp.rs
index a566fc3e7dcb87237c68eb7d174efa5658712ddb..b8a3dace7a616cd8944e3f647293cc4ca79235bd 100644
--- a/rust/kernel/opp.rs
+++ b/rust/kernel/opp.rs
@@ -16,7 +16,7 @@
     ffi::c_ulong,
     prelude::*,
     str::CString,
-    types::{ARef, AlwaysRefCounted, Opaque},
+    types::{ARef, AlwaysRefCounted, Opaque, RefCounted},
 };
 
 #[cfg(CONFIG_CPU_FREQ)]
@@ -1042,7 +1042,7 @@ unsafe impl Send for OPP {}
 unsafe impl Sync for OPP {}
 
 /// SAFETY: The type invariants guarantee that [`OPP`] is always refcounted.
-unsafe impl AlwaysRefCounted for OPP {
+unsafe impl RefCounted for OPP {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
         unsafe { bindings::dev_pm_opp_get(self.0.get()) };
@@ -1054,6 +1054,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<OPP>` from an
+// `&OPP`.
+unsafe impl AlwaysRefCounted for OPP {}
+
 impl OPP {
     /// Creates an owned reference to a [`OPP`] from a valid pointer.
     ///
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38129ccc3495e7c4d3237fcaa97ad9..e56f48bfe1e60096208bc49963046fbdd070afee 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -435,7 +435,7 @@ pub fn set_master(&self) {
 kernel::impl_device_context_into_aref!(Device);
 
 // SAFETY: Instances of `Device` are always reference-counted.
-unsafe impl crate::types::AlwaysRefCounted for Device {
+unsafe impl crate::types::RefCounted for Device {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
         unsafe { bindings::pci_dev_get(self.as_raw()) };
@@ -447,6 +447,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<Device>` from a
+// `&Device`.
+unsafe impl crate::types::AlwaysRefCounted for Device {}
+
 impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
     fn as_ref(&self) -> &device::Device<Ctx> {
         // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
diff --git a/rust/kernel/pid_namespace.rs b/rust/kernel/pid_namespace.rs
index 0e93808e4639b37dd77add5d79f64058dac7cb87..b5e319fa050002179fa920dd40f02a08f8473b22 100644
--- a/rust/kernel/pid_namespace.rs
+++ b/rust/kernel/pid_namespace.rs
@@ -9,7 +9,7 @@
 
 use crate::{
     bindings,
-    types::{AlwaysRefCounted, Opaque},
+    types::{AlwaysRefCounted, Opaque, RefCounted},
 };
 use core::ptr;
 
@@ -44,7 +44,7 @@ pub unsafe fn from_ptr<'a>(ptr: *const bindings::pid_namespace) -> &'a Self {
 }
 
 // SAFETY: Instances of `PidNamespace` are always reference-counted.
-unsafe impl AlwaysRefCounted for PidNamespace {
+unsafe impl RefCounted for PidNamespace {
     #[inline]
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
@@ -58,6 +58,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<PidNamespace>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<PidNamespace>` from
+// a `&PidNamespace`.
+unsafe impl AlwaysRefCounted for PidNamespace {}
+
 // SAFETY:
 // - `PidNamespace::dec_ref` can be called from any thread.
 // - It is okay to send ownership of `PidNamespace` across thread boundaries.
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 5b21fa517e55348582622ec10471918919502959..c37ba50e5dcc53891e08706343fcd446d640cda1 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -196,7 +196,7 @@ fn as_raw(&self) -> *mut bindings::platform_device {
 kernel::impl_device_context_into_aref!(Device);
 
 // SAFETY: Instances of `Device` are always reference-counted.
-unsafe impl crate::types::AlwaysRefCounted for Device {
+unsafe impl crate::types::RefCounted for Device {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
         unsafe { bindings::get_device(self.as_ref().as_raw()) };
@@ -208,6 +208,10 @@ unsafe fn dec_ref(obj: NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<Device>` from a
+// `&Device`.
+unsafe impl crate::types::AlwaysRefCounted for Device {}
+
 impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
     fn as_ref(&self) -> &device::Device<Ctx> {
         // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index 927413d854846477578cbaf06e27d1fc867d0682..6c16f03c50591d6aafe4b9176c5c934e1a7b81ba 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -340,7 +340,7 @@ pub fn active_pid_ns(&self) -> Option<&PidNamespace> {
 }
 
 // SAFETY: The type invariants guarantee that `Task` is always refcounted.
-unsafe impl crate::types::AlwaysRefCounted for Task {
+unsafe impl crate::types::RefCounted for Task {
     fn inc_ref(&self) {
         // SAFETY: The existence of a shared reference means that the refcount is nonzero.
         unsafe { bindings::get_task_struct(self.as_ptr()) };
@@ -352,6 +352,10 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
     }
 }
 
+// SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<Task>` from a
+// `&Task`.
+unsafe impl crate::types::AlwaysRefCounted for Task {}
+
 impl Kuid {
     /// Get the current euid.
     #[inline]
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1..40c0138bd336057e7d3a835a9e81391baa2fd2b1 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -418,11 +418,9 @@ pub const fn raw_get(this: *const Self) -> *mut T {
     }
 }
 
-/// Types that are _always_ reference counted.
+/// Types that are internally reference counted.
 ///
 /// It allows such types to define their own custom ref increment and decrement functions.
-/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
-/// [`ARef<T>`].
 ///
 /// This is usually implemented by wrappers to existing structures on the C side of the code. For
 /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
@@ -438,9 +436,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 /// at least until matching decrements are performed.
 ///
 /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
-/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
-/// alive.)
-pub unsafe trait AlwaysRefCounted {
+/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
+pub unsafe trait RefCounted {
     /// Increments the reference count on the object.
     fn inc_ref(&self);
 
@@ -453,11 +450,21 @@ pub unsafe trait AlwaysRefCounted {
     /// Callers must ensure that there was a previous matching increment to the reference count,
     /// and that the object is no longer used after its reference count is decremented (as it may
     /// result in the object being freed), unless the caller owns another increment on the refcount
-    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
-    /// [`AlwaysRefCounted::dec_ref`] once).
+    /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
     unsafe fn dec_ref(obj: NonNull<Self>);
 }
 
+/// An extension to RefCounted, which declares that it is allowed to convert from a shared reference
+/// `&T` to an owned reference [`ARef<T>`].
+///
+/// # Safety
+///
+/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
+/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`Ownable`] cannot be
+/// implemented for the same type, as this would allow to violate the uniqueness guarantee of
+/// [`Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from that.
+pub unsafe trait AlwaysRefCounted: RefCounted {}
+
 /// An owned reference to an always-reference-counted object.
 ///
 /// The object's reference count is automatically decremented when an instance of [`ARef`] is
@@ -468,7 +475,7 @@ pub unsafe trait AlwaysRefCounted {
 ///
 /// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
 /// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
-pub struct ARef<T: AlwaysRefCounted> {
+pub struct ARef<T: RefCounted> {
     ptr: NonNull<T>,
     _p: PhantomData<T>,
 }
@@ -477,16 +484,16 @@ pub struct ARef<T: AlwaysRefCounted> {
 // it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it needs
 // `T` to be `Send` because any thread that has an `ARef<T>` may ultimately access `T` using a
 // mutable reference, for example, when the reference count reaches zero and `T` is dropped.
-unsafe impl<T: AlwaysRefCounted + Sync + Send> Send for ARef<T> {}
+unsafe impl<T: RefCounted + Sync + Send> Send for ARef<T> {}
 
 // SAFETY: It is safe to send `&ARef<T>` to another thread when the underlying `T` is `Sync`
 // because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally,
 // it needs `T` to be `Send` because any thread that has a `&ARef<T>` may clone it and get an
 // `ARef<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
 // example, when the reference count reaches zero and `T` is dropped.
-unsafe impl<T: AlwaysRefCounted + Sync + Send> Sync for ARef<T> {}
+unsafe impl<T: RefCounted + Sync + Send> Sync for ARef<T> {}
 
-impl<T: AlwaysRefCounted> ARef<T> {
+impl<T: RefCounted> ARef<T> {
     /// Creates a new instance of [`ARef`].
     ///
     /// It takes over an increment of the reference count on the underlying object.
@@ -515,12 +522,12 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
     ///
     /// ```
     /// use core::ptr::NonNull;
-    /// use kernel::types::{ARef, AlwaysRefCounted};
+    /// use kernel::types::{ARef, RefCounted};
     ///
     /// struct Empty {}
     ///
     /// # // SAFETY: TODO.
-    /// unsafe impl AlwaysRefCounted for Empty {
+    /// unsafe impl RefCounted for Empty {
     ///     fn inc_ref(&self) {}
     ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
     /// }
@@ -538,7 +545,7 @@ pub fn into_raw(me: Self) -> NonNull<T> {
     }
 }
 
-impl<T: AlwaysRefCounted> Clone for ARef<T> {
+impl<T: RefCounted> Clone for ARef<T> {
     fn clone(&self) -> Self {
         self.inc_ref();
         // SAFETY: We just incremented the refcount above.
@@ -546,7 +553,7 @@ fn clone(&self) -> Self {
     }
 }
 
-impl<T: AlwaysRefCounted> Deref for ARef<T> {
+impl<T: RefCounted> Deref for ARef<T> {
     type Target = T;
 
     fn deref(&self) -> &Self::Target {
@@ -563,7 +570,7 @@ fn from(b: &T) -> Self {
     }
 }
 
-impl<T: AlwaysRefCounted> Drop for ARef<T> {
+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
         // decrement.
diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
index f4065a0d627a62d3ecb15edabf306e9b812556e1..80cd990f6601767aa5a742a6c0997f4f67d06453 100644
--- a/rust/kernel/types/ownable.rs
+++ b/rust/kernel/types/ownable.rs
@@ -18,8 +18,8 @@
 ///
 /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
 /// provide reference counting but represents a unique, owned reference. If reference counting is
-/// required [`AlwaysRefCounted`](crate::types::AlwaysRefCounted) should be implemented which allows
-/// types to be wrapped in an [`ARef<Self>`](crate::types::ARef).
+/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
+/// wrapped in an [`ARef<Self>`](crate::types::ARef).
 ///
 /// # Safety
 ///

-- 
2.49.0

Re: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

[kernel test robot]

Hi Oliver,

kernel test robot noticed the following build errors:

[auto build test ERROR on e04c78d86a9699d136910cfc0bdcf01087e3267e]

url:    https://github.com/intel-lab-lkp/linux/commits/Oliver-Mangold/rust-types-Add-Ownable-Owned-types/20250618-203524
base:   e04c78d86a9699d136910cfc0bdcf01087e3267e
patch link:    https://lore.kernel.org/r/20250618-unique-ref-v11-2-49eadcdc0aa6%40pm.me
patch subject: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250619/202506191023.smOZ1Mpy-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250619/202506191023.smOZ1Mpy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506191023.smOZ1Mpy-lkp@intel.com/

All errors (new ones prefixed by >>):

   PATH=/opt/cross/clang-18/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   INFO PATH=/opt/cross/rustc-1.78.0-bindgen-0.65.1/cargo/bin:/opt/cross/clang-18/bin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
   /usr/bin/timeout -k 100 12h /usr/bin/make KCFLAGS= -Wno-error=return-type -Wreturn-type -funsigned-char -Wundef W=1 --keep-going LLVM=1 -j32 -C source O=/kbuild/obj/consumer/x86_64-rhel-9.4-rust ARCH=x86_64 SHELL=/bin/bash rustfmtcheck
   make: Entering directory '/kbuild/src/consumer'
   make[1]: Entering directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
>> Diff in rust/kernel/mm/mmput_async.rs at line 48:
        }
    }
    
   -
    // SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<MmWithUserAsync>`
    // from a `&MmWithUserAsync`.
    unsafe impl AlwaysRefCounted for MmWithUserAsync {}
>> Diff in rust/kernel/mm/mmput_async.rs at line 48:
        }
    }
    
   -
    // SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<MmWithUserAsync>`
    // from a `&MmWithUserAsync`.
    unsafe impl AlwaysRefCounted for MmWithUserAsync {}
>> Diff in rust/kernel/mm/mmput_async.rs at line 48:
        }
    }
    
   -
    // SAFETY: We do not implement `Ownable`, thus it is okay to can obtain an `ARef<MmWithUserAsync>`
    // from a `&MmWithUserAsync`.
    unsafe impl AlwaysRefCounted for MmWithUserAsync {}
   make[2]: *** [Makefile:1825: rustfmt] Error 123
   make[2]: Target 'rustfmtcheck' not remade because of errors.
   make[1]: Leaving directory '/kbuild/obj/consumer/x86_64-rhel-9.4-rust'
   make[1]: *** [Makefile:248: __sub-make] Error 2
   make[1]: Target 'rustfmtcheck' not remade because of errors.
   make: *** [Makefile:248: __sub-make] Error 2
   make: Target 'rustfmtcheck' not remade because of errors.
   make: Leaving directory '/kbuild/src/consumer'
--
>> error[E0277]: the trait bound `auxiliary::Device: types::RefCounted` is not satisfied
   --> rust/kernel/device.rs:329:56
   |
   329 |         impl ::core::convert::From<&$device<$src>> for $crate::types::ARef<$device> {
   |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `types::RefCounted` is not implemented for `auxiliary::Device`
   |
   ::: rust/kernel/auxiliary.rs:250:1
   |
   250 | kernel::impl_device_context_into_aref!(Device);
   | ---------------------------------------------- in this macro invocation
   |
   = help: the following other types implement trait `types::RefCounted`:
   block::mq::request::Request<T>
   cred::Credential
   device::Device
   fs::file::File
   fs::file::LocalFile
   mm::mmput_async::MmWithUserAsync
   mm::Mm
   mm::MmWithUser
   and 4 others
   note: required by a bound in `types::ARef`
   --> rust/kernel/types.rs:478:20
   |
   478 | pub struct ARef<T: RefCounted> {
   |                    ^^^^^^^^^^ required by this bound in `ARef`
   = note: this error originates in the macro `::kernel::__impl_device_context_into_aref` which comes from the expansion of the macro `kernel::impl_device_context_into_aref` (in Nightly builds, run with -Z macro-backtrace for more info)
--
>> error[E0277]: the trait bound `auxiliary::Device: types::RefCounted` is not satisfied
   --> rust/kernel/auxiliary.rs:253:48
   |
   253 | unsafe impl crate::types::AlwaysRefCounted for Device {
   |                                                ^^^^^^ the trait `types::RefCounted` is not implemented for `auxiliary::Device`
   |
   = help: the following other types implement trait `types::RefCounted`:
   block::mq::request::Request<T>
   cred::Credential
   device::Device
   fs::file::File
   fs::file::LocalFile
   mm::mmput_async::MmWithUserAsync
   mm::Mm
   mm::MmWithUser
   and 4 others
   note: required by a bound in `types::AlwaysRefCounted`
   --> rust/kernel/types.rs:466:36
   |
   466 | pub unsafe trait AlwaysRefCounted: RefCounted {}
   |                                    ^^^^^^^^^^ required by this bound in `AlwaysRefCounted`
--
>> error[E0277]: the trait bound `auxiliary::Device: types::RefCounted` is not satisfied
   --> rust/kernel/device.rs:330:45
   |
   330 |             fn from(dev: &$device<$src>) -> Self {
   |                                             ^^^^ the trait `types::RefCounted` is not implemented for `auxiliary::Device`
   |
   ::: rust/kernel/auxiliary.rs:250:1
   |
   250 | kernel::impl_device_context_into_aref!(Device);
   | ---------------------------------------------- in this macro invocation
   |
   = help: the following other types implement trait `types::RefCounted`:
   block::mq::request::Request<T>
   cred::Credential
   device::Device
   fs::file::File
   fs::file::LocalFile
   mm::mmput_async::MmWithUserAsync
   mm::Mm
   mm::MmWithUser
   and 4 others
   note: required by a bound in `types::ARef`
   --> rust/kernel/types.rs:478:20
   |
   478 | pub struct ARef<T: RefCounted> {
   |                    ^^^^^^^^^^ required by this bound in `ARef`
   = note: this error originates in the macro `::kernel::__impl_device_context_into_aref` which comes from the expansion of the macro `kernel::impl_device_context_into_aref` (in Nightly builds, run with -Z macro-backtrace for more info)
--
>> error[E0277]: the trait bound `auxiliary::Device: types::RefCounted` is not satisfied
   --> rust/kernel/device.rs:331:17
   |
   331 |                 (&**dev).into()
   |                 ^^^^^^^^^^^^^^^ the trait `types::RefCounted` is not implemented for `auxiliary::Device`
   |
   ::: rust/kernel/auxiliary.rs:250:1
   |
   250 | kernel::impl_device_context_into_aref!(Device);
   | ---------------------------------------------- in this macro invocation
   |
   = help: the following other types implement trait `types::RefCounted`:
   block::mq::request::Request<T>
   cred::Credential
   device::Device
   fs::file::File
   fs::file::LocalFile
   mm::mmput_async::MmWithUserAsync
   mm::Mm
   mm::MmWithUser
   and 4 others
   note: required by a bound in `types::ARef`
   --> rust/kernel/types.rs:478:20
   |
   478 | pub struct ARef<T: RefCounted> {
   |                    ^^^^^^^^^^ required by this bound in `ARef`
   = note: this error originates in the macro `::kernel::__impl_device_context_into_aref` which comes from the expansion of the macro `kernel::impl_device_context_into_aref` (in Nightly builds, run with -Z macro-backtrace for more info)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

[Benno Lossin]

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1..40c0138bd336057e7d3a835a9e81391baa2fd2b1 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -418,11 +418,9 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>      }
>  }
>  
> -/// Types that are _always_ reference counted.
> +/// Types that are internally reference counted.
>  ///
>  /// It allows such types to define their own custom ref increment and decrement functions.
> -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> -/// [`ARef<T>`].
>  ///
>  /// This is usually implemented by wrappers to existing structures on the C side of the code. For
>  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> @@ -438,9 +436,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>  /// at least until matching decrements are performed.
>  ///
>  /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> -/// alive.)
> -pub unsafe trait AlwaysRefCounted {
> +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
> +pub unsafe trait RefCounted {
>      /// Increments the reference count on the object.
>      fn inc_ref(&self);

This seems a bit problematic for `Owned`, since now I can do:

    fn bad<T: Ownable + RefCounted>(t: &Owned<T>) {
        t.inc_ref();
    }

And now the `Owned<T>` is no longer "unique" in the sense that the
refcount is 1...

Similarly, we should probably make this an associated function, such
that people don't accidentally call `.inc_ref()` on `ARef<T>`.

> @@ -453,11 +450,21 @@ pub unsafe trait AlwaysRefCounted {
>      /// Callers must ensure that there was a previous matching increment to the reference count,
>      /// and that the object is no longer used after its reference count is decremented (as it may
>      /// result in the object being freed), unless the caller owns another increment on the refcount
> -    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> -    /// [`AlwaysRefCounted::dec_ref`] once).
> +    /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
>      unsafe fn dec_ref(obj: NonNull<Self>);
>  }
>  
> +/// An extension to RefCounted, which declares that it is allowed to convert from a shared reference
> +/// `&T` to an owned reference [`ARef<T>`].

This is a bit too long for the first sentence... How about

    Always reference counted type.

    Allows the creation of `ARef<T>` from `&T`.

Feel free to add more information.

> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
> +/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`Ownable`] cannot be
> +/// implemented for the same type, as this would allow to violate the uniqueness guarantee of
> +/// [`Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from that.
> +pub unsafe trait AlwaysRefCounted: RefCounted {}

It's a bit sad that we can't just say `: !Ownable` (or rather a
blanket-implemented marker trait, since that might land earlier). Anyone
aware of progress in this area?

---
Cheers,
Benno

> +
>  /// An owned reference to an always-reference-counted object.
>  ///
>  /// The object's reference count is automatically decremented when an instance of [`ARef`] is

Re: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

[Oliver Mangold]

On 250702 1323, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> > index c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1..40c0138bd336057e7d3a835a9e81391baa2fd2b1 100644
> > --- a/rust/kernel/types.rs
> > +++ b/rust/kernel/types.rs
> > @@ -418,11 +418,9 @@ pub const fn raw_get(this: *const Self) -> *mut T {
> >      }
> >  }
> >
> > -/// Types that are _always_ reference counted.
> > +/// Types that are internally reference counted.
> >  ///
> >  /// It allows such types to define their own custom ref increment and decrement functions.
> > -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
> > -/// [`ARef<T>`].
> >  ///
> >  /// This is usually implemented by wrappers to existing structures on the C side of the code. For
> >  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
> > @@ -438,9 +436,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
> >  /// at least until matching decrements are performed.
> >  ///
> >  /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
> > -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
> > -/// alive.)
> > -pub unsafe trait AlwaysRefCounted {
> > +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
> > +pub unsafe trait RefCounted {
> >      /// Increments the reference count on the object.
> >      fn inc_ref(&self);
> 
> This seems a bit problematic for `Owned`, since now I can do:
> 
>     fn bad<T: Ownable + RefCounted>(t: &Owned<T>) {
>         t.inc_ref();
>     }
> 
> And now the `Owned<T>` is no longer "unique" in the sense that the
> refcount is 1...

Yes, that is clear. But that isn't a soundness issue or is it? It just
means the `T` can be leaked, but that cannot be prevented anyway.

> Similarly, we should probably make this an associated function, such
> that people don't accidentally call `.inc_ref()` on `ARef<T>`.
> 
> > @@ -453,11 +450,21 @@ pub unsafe trait AlwaysRefCounted {
> >      /// Callers must ensure that there was a previous matching increment to the reference count,
> >      /// and that the object is no longer used after its reference count is decremented (as it may
> >      /// result in the object being freed), unless the caller owns another increment on the refcount
> > -    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> > -    /// [`AlwaysRefCounted::dec_ref`] once).
> > +    /// (e.g., it calls [`RefCounted::inc_ref`] twice, then calls [`RefCounted::dec_ref`] once).
> >      unsafe fn dec_ref(obj: NonNull<Self>);
> >  }
> >
> > +/// An extension to RefCounted, which declares that it is allowed to convert from a shared reference
> > +/// `&T` to an owned reference [`ARef<T>`].
> 
> This is a bit too long for the first sentence... How about
> 
>     Always reference counted type.
> 
>     Allows the creation of `ARef<T>` from `&T`.
> 
> Feel free to add more information.

Yes, should be okay.

> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that no safety invariants are violated by upgrading an `&T` to an
> > +/// [`ARef<T>`]. In particular that implies [`AlwaysRefCounted`] and [`Ownable`] cannot be
> > +/// implemented for the same type, as this would allow to violate the uniqueness guarantee of
> > +/// [`Owned<T>`] by derefencing it into an `&T` and obtaining an [`ARef`] from that.
> > +pub unsafe trait AlwaysRefCounted: RefCounted {}
> 
> It's a bit sad that we can't just say `: !Ownable` (or rather a
> blanket-implemented marker trait, since that might land earlier). Anyone
> aware of progress in this area?

Yes. But as far as I am aware negative constraints are considered to be
deeply problematic because of combinatoric explosion in binary logic.

Best,

Oliver

Re: [PATCH v11 2/4] rust: Split `AlwaysRefCounted` into two traits

[Benno Lossin]

On Mon Jul 7, 2025 at 9:42 AM CEST, Oliver Mangold wrote:
> On 250702 1323, Benno Lossin wrote:
>> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> > index c12ff4d2a3f2d79b760c34c0b84a51b507d0cfb1..40c0138bd336057e7d3a835a9e81391baa2fd2b1 100644
>> > --- a/rust/kernel/types.rs
>> > +++ b/rust/kernel/types.rs
>> > @@ -418,11 +418,9 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>> >      }
>> >  }
>> >
>> > -/// Types that are _always_ reference counted.
>> > +/// Types that are internally reference counted.
>> >  ///
>> >  /// It allows such types to define their own custom ref increment and decrement functions.
>> > -/// Additionally, it allows users to convert from a shared reference `&T` to an owned reference
>> > -/// [`ARef<T>`].
>> >  ///
>> >  /// This is usually implemented by wrappers to existing structures on the C side of the code. For
>> >  /// Rust code, the recommendation is to use [`Arc`](crate::sync::Arc) to create reference-counted
>> > @@ -438,9 +436,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
>> >  /// at least until matching decrements are performed.
>> >  ///
>> >  /// Implementers must also ensure that all instances are reference-counted. (Otherwise they
>> > -/// won't be able to honour the requirement that [`AlwaysRefCounted::inc_ref`] keep the object
>> > -/// alive.)
>> > -pub unsafe trait AlwaysRefCounted {
>> > +/// won't be able to honour the requirement that [`RefCounted::inc_ref`] keep the object alive.)
>> > +pub unsafe trait RefCounted {
>> >      /// Increments the reference count on the object.
>> >      fn inc_ref(&self);
>> 
>> This seems a bit problematic for `Owned`, since now I can do:
>> 
>>     fn bad<T: Ownable + RefCounted>(t: &Owned<T>) {
>>         t.inc_ref();
>>     }
>> 
>> And now the `Owned<T>` is no longer "unique" in the sense that the
>> refcount is 1...
>
> Yes, that is clear. But that isn't a soundness issue or is it? It just
> means the `T` can be leaked, but that cannot be prevented anyway.

Yeah that is true.

>> Similarly, we should probably make this an associated function, such
>> that people don't accidentally call `.inc_ref()` on `ARef<T>`.

Filed https://github.com/Rust-for-Linux/linux/issues/1177

---
Cheers,
Benno

[PATCH v11 3/4] rust: Add missing SAFETY documentation for `ARef` example

[Oliver Mangold]

SAFETY comment in rustdoc example was just 'TODO'. Fixed.

Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
---
 rust/kernel/types.rs | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 40c0138bd336057e7d3a835a9e81391baa2fd2b1..a22d5d39d89edfe357fdfe903ef42e5d5404237f 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -526,7 +526,9 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
     ///
     /// struct Empty {}
     ///
-    /// # // SAFETY: TODO.
+    /// // SAFETY: The `RefCounted` implementation for `Empty` does not count references and
+    /// // never frees the underlying object. Thus we can act as having a refcount on the object
+    /// // that we pass to the newly created `ARef`.
     /// unsafe impl RefCounted for Empty {
     ///     fn inc_ref(&self) {}
     ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
@@ -534,7 +536,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
     ///
     /// let mut data = Empty {};
     /// let ptr = NonNull::<Empty>::new(&mut data).unwrap();
-    /// # // SAFETY: TODO.
+    /// // SAFETY: We keep `data` around longer than the `ARef`.
     /// let data_ref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
     /// let raw_ptr: NonNull<Empty> = ARef::into_raw(data_ref);
     ///

-- 
2.49.0

[PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Oliver Mangold]

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@pm.me>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/types.rs         |  10 +++-
 rust/kernel/types/ownable.rs | 127 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 134 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index a22d5d39d89edfe357fdfe903ef42e5d5404237f..af0bda4ea50f54aed3c51327db0e43f960868501 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -12,7 +12,7 @@
 use pin_init::{PinInit, Zeroable};
 
 pub mod ownable;
-pub use ownable::{Ownable, OwnableMut, Owned};
+pub use ownable::{Ownable, OwnableMut, OwnableRefCounted, Owned};
 
 /// Used to transfer ownership to and from foreign (non-Rust) languages.
 ///
@@ -429,6 +429,8 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 /// 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`] should be implemented which allows types to be wrapped in an [`Owned<Self>`].
+/// Implementing the trait [`OwnableRefCounted`] allows to convert between unique and shared
+/// references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
 ///
 /// # Safety
 ///
@@ -572,6 +574,12 @@ fn from(b: &T) -> Self {
     }
 }
 
+impl<T: OwnableRefCounted> From<Owned<T>> for ARef<T> {
+    fn from(b: Owned<T>) -> Self {
+        T::into_shared(b)
+    }
+}
+
 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/ownable.rs b/rust/kernel/types/ownable.rs
index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
--- a/rust/kernel/types/ownable.rs
+++ b/rust/kernel/types/ownable.rs
@@ -2,6 +2,7 @@
 
 //! Owned reference types.
 
+use crate::types::{ARef, RefCounted};
 use core::{
     marker::PhantomData,
     mem::ManuallyDrop,
@@ -18,8 +19,9 @@
 ///
 /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
 /// provide reference counting but represents a unique, owned reference. If reference counting is
-/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
-/// wrapped in an [`ARef<Self>`](crate::types::ARef).
+/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
+/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
+/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
 ///
 /// # Safety
 ///
@@ -132,3 +134,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`].
+///
+/// # Safety
+///
+/// Implementers must ensure that:
+///
+/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
+///   exactly one [`ARef<Self>`] exists.
+/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
+///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
+///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
+///   implementation, which just rewraps the underlying object, the reference count needs not to be
+///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
+///
+/// # 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::types::{
+///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
+/// };
+///
+/// struct Foo {
+///     refcount: Cell<usize>,
+/// }
+///
+/// impl Foo {
+///     fn new() -> Result<Owned<Self>, AllocError> {
+///         // Use a `KBox` to handle the actual allocation.
+///         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 `Foo`, thus it is valid.
+///         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: The underlying object is always valid when the function is called.
+///         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 `Box<Foo>` is still alive as the old refcount is 1.
+///             unsafe { KBox::from_raw(this.as_ptr()) };
+///         } else {
+///             refcount.replace(new_refcount);
+///         }
+///     }
+/// }
+///
+/// // SAFETY: We only convert into an `Owned` when the refcount is 1.
+/// unsafe 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 as the refcount is 1.
+///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
+///         } else {
+///             Err(this)
+///         }
+///     }
+/// }
+///
+/// // SAFETY: We are not `AlwaysRefCounted`.
+/// 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().unwrap();
+/// 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 unsafe 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>>;
+
+    /// 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)
+    }
+}

-- 
2.49.0

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
> --- a/rust/kernel/types/ownable.rs
> +++ b/rust/kernel/types/ownable.rs
> @@ -2,6 +2,7 @@
>  
>  //! Owned reference types.
>  
> +use crate::types::{ARef, RefCounted};
>  use core::{
>      marker::PhantomData,
>      mem::ManuallyDrop,
> @@ -18,8 +19,9 @@
>  ///
>  /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
>  /// provide reference counting but represents a unique, owned reference. If reference counting is
> -/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
> -/// wrapped in an [`ARef<Self>`](crate::types::ARef).
> +/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
> +/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
> +/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).

This change should probably be in the earlier patch.

>  ///
>  /// # Safety
>  ///
> @@ -132,3 +134,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`].
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that:
> +///
> +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
> +///   exactly one [`ARef<Self>`] exists.

This shouldn't be required?

> +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
> +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
> +///   implementation, which just rewraps the underlying object, the reference count needs not to be
> +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].

This also seems pretty weird...

I feel like `OwnableRefCounted` is essentially just a compatibility
condition between `Ownable` and `RefCounted`. It ensures that the
ownership declared in `Ownable` corresponds to exactly one refcount
declared in `RefCounted`.

That being said, I think a `RefCounted` *always* canonically is
`Ownable` by the following impl:

    unsafe impl<T: RefCounted> Ownable for T {
        unsafe fn release(this: NonNull<Self>) {
            T::dec_ref(this)
        }
    }

So I don't think that we need this trait at all?

> +///
> +/// # Examples

If we're having an example here, then we should also have on on `Owned`.

> +///
> +/// 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::types::{
> +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
> +/// };
> +///
> +/// struct Foo {
> +///     refcount: Cell<usize>,
> +/// }
> +///
> +/// impl Foo {
> +///     fn new() -> Result<Owned<Self>, AllocError> {
> +///         // Use a `KBox` to handle the actual allocation.
> +///         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.");

I'm not really convinced that an example using `KBox` is a good one...
Maybe we should just have a local invisible `bindings` module that
exposes a `-> *mut foo`. (internally it can just create a KBox`)

> +///         // SAFETY: We just allocated the `Foo`, thus it is valid.

This isn't going through all the requirements on `from_raw`...

---
Cheers,
Benno

> +///         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: The underlying object is always valid when the function is called.
> +///         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 `Box<Foo>` is still alive as the old refcount is 1.
> +///             unsafe { KBox::from_raw(this.as_ptr()) };
> +///         } else {
> +///             refcount.replace(new_refcount);
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We only convert into an `Owned` when the refcount is 1.
> +/// unsafe 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 as the refcount is 1.
> +///             Ok(unsafe { Owned::from_raw(ARef::into_raw(this)) })
> +///         } else {
> +///             Err(this)
> +///         }
> +///     }
> +/// }
> +///
> +/// // SAFETY: We are not `AlwaysRefCounted`.
> +/// 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().unwrap();
> +/// 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 unsafe 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>>;
> +
> +    /// 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)
> +    }
> +}

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Oliver Mangold]

On 250702 1524, Benno Lossin wrote:
> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> > diff --git a/rust/kernel/types/ownable.rs b/rust/kernel/types/ownable.rs
> > index 80cd990f6601767aa5a742a6c0997f4f67d06453..b5626dead6bb25ea76a0ae577db1b130308d98b1 100644
> > --- a/rust/kernel/types/ownable.rs
> > +++ b/rust/kernel/types/ownable.rs
> > @@ -2,6 +2,7 @@
> >
> >  //! Owned reference types.
> >
> > +use crate::types::{ARef, RefCounted};
> >  use core::{
> >      marker::PhantomData,
> >      mem::ManuallyDrop,
> > @@ -18,8 +19,9 @@
> >  ///
> >  /// Note: Implementing this trait allows types to be wrapped in an [`Owned<Self>`]. This does not
> >  /// provide reference counting but represents a unique, owned reference. If reference counting is
> > -/// required [`RefCounted`](crate::types::RefCounted) should be implemented which allows types to be
> > -/// wrapped in an [`ARef<Self>`](crate::types::ARef).
> > +/// required [`RefCounted`] should be implemented which allows types to be wrapped in an
> > +/// [`ARef<Self>`]. Implementing the trait [`OwnableRefCounted`] allows to convert between unique
> > +/// and shared references (i.e. [`Owned<Self>`] and [`ARef<Self>`]).
> 
> This change should probably be in the earlier patch.

Ah, yes. Must have happened while splitting the patches.

> >  ///
> >  /// # Safety
> >  ///
> > @@ -132,3 +134,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`].
> > +///
> > +/// # Safety
> > +///
> > +/// Implementers must ensure that:
> > +///
> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
> > +///   exactly one [`ARef<Self>`] exists.
> 
> This shouldn't be required?

Ehm, why not? `Owned<T>` is supposed to be unique.

> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
> 
> This also seems pretty weird...
> 
> I feel like `OwnableRefCounted` is essentially just a compatibility
> condition between `Ownable` and `RefCounted`. It ensures that the
> ownership declared in `Ownable` corresponds to exactly one refcount
> declared in `RefCounted`.
> 
> That being said, I think a `RefCounted` *always* canonically is
> `Ownable` by the following impl:
> 
>     unsafe impl<T: RefCounted> Ownable for T {
>         unsafe fn release(this: NonNull<Self>) {
>             T::dec_ref(this)
>         }
>     }
> 
> So I don't think that we need this trait at all?

No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
`try_from_shared()` implementation. It is not even a given that the
function can implemented, if all the kernel exposes are some kind of
`inc_ref()` and `dec_ref()`.

Also there are more complicated cases like with `Mq::Request`, where the
existence of an `Owned<T>` cannot be represented by the same refcount value
as the existence of exactly one `ARef<T>`.

> > +///
> > +/// # Examples
> 
> If we're having an example here, then we should also have on on `Owned`.

Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
because it is a more complex idea than `Ownable`.

If I remember correctly, I didn't create one for `Owned`, as it should
probably more or less the same as for `ARef` and the one there has even
more problems of the kind you are pointing out. So maybe it would be best
to wait until someone fixes that and have the fixed version copied over to
`Owned` in the process?

> > +///
> > +/// 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::types::{
> > +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
> > +/// };
> > +///
> > +/// struct Foo {
> > +///     refcount: Cell<usize>,
> > +/// }
> > +///
> > +/// impl Foo {
> > +///     fn new() -> Result<Owned<Self>, AllocError> {
> > +///         // Use a `KBox` to handle the actual allocation.
> > +///         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.");
> 
> I'm not really convinced that an example using `KBox` is a good one...
> Maybe we should just have a local invisible `bindings` module that
> exposes a `-> *mut foo`. (internally it can just create a KBox`)

The example would become quite a bit more complicated then, no?

> > +///         // SAFETY: We just allocated the `Foo`, thus it is valid.
> 
> This isn't going through all the requirements on `from_raw`...

Yes, this comment should probably be detailed.

Best,

Oliver

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
> On 250702 1524, Benno Lossin wrote:
>> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> > @@ -132,3 +134,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`].
>> > +///
>> > +/// # Safety
>> > +///
>> > +/// Implementers must ensure that:
>> > +///
>> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
>> > +///   exactly one [`ARef<Self>`] exists.
>> 
>> This shouldn't be required?
>
> Ehm, why not? `Owned<T>` is supposed to be unique.

It's not needed as a safety requirement for implementing the trait. If
the implementation only contains sound code, then `Owned::from_raw`
should already ensure that `Owned<Self>` is only created if there is
exactly one reference to it.

>> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
>> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
>> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
>> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
>> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
>> 
>> This also seems pretty weird...
>> 
>> I feel like `OwnableRefCounted` is essentially just a compatibility
>> condition between `Ownable` and `RefCounted`. It ensures that the
>> ownership declared in `Ownable` corresponds to exactly one refcount
>> declared in `RefCounted`.
>> 
>> That being said, I think a `RefCounted` *always* canonically is
>> `Ownable` by the following impl:
>> 
>>     unsafe impl<T: RefCounted> Ownable for T {
>>         unsafe fn release(this: NonNull<Self>) {
>>             T::dec_ref(this)
>>         }
>>     }
>> 
>> So I don't think that we need this trait at all?
>
> No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
> `try_from_shared()` implementation. It is not even a given that the
> function can implemented, if all the kernel exposes are some kind of
> `inc_ref()` and `dec_ref()`.

I don't understand this paragraph.

> Also there are more complicated cases like with `Mq::Request`, where the
> existence of an `Owned<T>` cannot be represented by the same refcount value
> as the existence of exactly one `ARef<T>`.

Ah right, I forgot about this. What was the refcount characteristics of
this again?

*  1 = in flight, owned by C
*  2 = in flight, owned by Rust
* >2 = in flight, owned by Rust + additional references used by Rust
       code

Correct? Maybe @Andreas can check.

>> > +///
>> > +/// # Examples
>> 
>> If we're having an example here, then we should also have on on `Owned`.
>
> Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
> because it is a more complex idea than `Ownable`.
>
> If I remember correctly, I didn't create one for `Owned`, as it should
> probably more or less the same as for `ARef` and the one there has even
> more problems of the kind you are pointing out. So maybe it would be best
> to wait until someone fixes that and have the fixed version copied over to
> `Owned` in the process?

Wait which problems on `ARef` do you mean? I disagree that `Owned` and
`ARef` have the same example. `Owned` should expose operations that
`ARef` can't otherwise there would be no value in using `Owned`.

>> > +///
>> > +/// 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::types::{
>> > +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
>> > +/// };
>> > +///
>> > +/// struct Foo {
>> > +///     refcount: Cell<usize>,
>> > +/// }
>> > +///
>> > +/// impl Foo {
>> > +///     fn new() -> Result<Owned<Self>, AllocError> {
>> > +///         // Use a `KBox` to handle the actual allocation.
>> > +///         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.");
>> 
>> I'm not really convinced that an example using `KBox` is a good one...
>> Maybe we should just have a local invisible `bindings` module that
>> exposes a `-> *mut foo`. (internally it can just create a KBox`)
>
> The example would become quite a bit more complicated then, no?

Just hide those parts behind `#` lines in the example.

---
Cheers,
Benno

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
>> On 250702 1524, Benno Lossin wrote:
>>> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:

[...]

>>> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
>>> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
>>> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
>>> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
>>> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
>>>
>>> This also seems pretty weird...
>>>
>>> I feel like `OwnableRefCounted` is essentially just a compatibility
>>> condition between `Ownable` and `RefCounted`. It ensures that the
>>> ownership declared in `Ownable` corresponds to exactly one refcount
>>> declared in `RefCounted`.
>>>
>>> That being said, I think a `RefCounted` *always* canonically is
>>> `Ownable` by the following impl:
>>>
>>>     unsafe impl<T: RefCounted> Ownable for T {
>>>         unsafe fn release(this: NonNull<Self>) {
>>>             T::dec_ref(this)
>>>         }
>>>     }
>>>
>>> So I don't think that we need this trait at all?
>>
>> No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
>> `try_from_shared()` implementation. It is not even a given that the
>> function can implemented, if all the kernel exposes are some kind of
>> `inc_ref()` and `dec_ref()`.
>
> I don't understand this paragraph.
>
>> Also there are more complicated cases like with `Mq::Request`, where the
>> existence of an `Owned<T>` cannot be represented by the same refcount value
>> as the existence of exactly one `ARef<T>`.
>
> Ah right, I forgot about this. What was the refcount characteristics of
> this again?
>
> *  1 = in flight, owned by C
> *  2 = in flight, owned by Rust
> * >2 = in flight, owned by Rust + additional references used by Rust
>        code
>
> Correct? Maybe @Andreas can check.

We have been a bit back and forth on this. This is how we would like it
going forward:


/// There are three states for a request that the Rust bindings care about:
///
/// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
/// - 1: The request is owned by Rust abstractions but is not referenced.
/// - 2+: There is one or more [`ARef`] instances referencing the request.


Best regards,
Andreas Hindborg

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> Ah right, I forgot about this. What was the refcount characteristics of
>> this again?
>>
>> *  1 = in flight, owned by C
>> *  2 = in flight, owned by Rust
>> * >2 = in flight, owned by Rust + additional references used by Rust
>>        code
>>
>> Correct? Maybe @Andreas can check.
>
> We have been a bit back and forth on this. This is how we would like it
> going forward:
>
>
> /// There are three states for a request that the Rust bindings care about:
> ///
> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
> /// - 1: The request is owned by Rust abstractions but is not referenced.
> /// - 2+: There is one or more [`ARef`] instances referencing the request.

Huh, now I'm more confused... Could you go into the details again?

---
Cheers,
Benno

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> Ah right, I forgot about this. What was the refcount characteristics of
>>> this again?
>>>
>>> *  1 = in flight, owned by C
>>> *  2 = in flight, owned by Rust
>>> * >2 = in flight, owned by Rust + additional references used by Rust
>>>        code
>>>
>>> Correct? Maybe @Andreas can check.
>>
>> We have been a bit back and forth on this. This is how we would like it
>> going forward:
>>
>>
>> /// There are three states for a request that the Rust bindings care about:
>> ///
>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>
> Huh, now I'm more confused... Could you go into the details again?

Well, there is not much to it. We found out we can alias "unique" and
"owned by C".

We initialize the refcount to 0 when we initialize the request
structure. This happens at queue creation time.

When C block layer hands over a request for processing to a Rust driver,
we `debug_assert!` that the refcount is 0. We unsafely invent an
`Owned<Request<_>>` and pass that to the driver.

The driver has the option of `into_shared` to obtain an
`ARef<Request<_>>`. We use this for remote completion and timer
completion in rnull.

In most drivers, when the driver hands off the request to the hardware,
the driver will stop accounting for the request. The `Owned<Request<_>>`
is consumed when issuing to the driver, or the driver could simply drop
it. Refcount goes to 1. An ID is passed along to hardware.

When a completion comes back from hardware, it carries the ID. We use
the C block layer `tag_to_rq` machinery to turn this ID back into an
`Owned<Request<_>>`. In that process, we check that `refcount == 1` and
if so, we set refcount to 0 and invent an `Owned<Request<_>>`.

There was simply no need to have two separate values for "owned by C"
and "owned by Rust, not referenced".

Best regards,
Andreas Hindborg

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>> Ah right, I forgot about this. What was the refcount characteristics of
>>>> this again?
>>>>
>>>> *  1 = in flight, owned by C
>>>> *  2 = in flight, owned by Rust
>>>> * >2 = in flight, owned by Rust + additional references used by Rust
>>>>        code
>>>>
>>>> Correct? Maybe @Andreas can check.
>>>
>>> We have been a bit back and forth on this. This is how we would like it
>>> going forward:
>>>
>>>
>>> /// There are three states for a request that the Rust bindings care about:
>>> ///
>>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>>
>> Huh, now I'm more confused... Could you go into the details again?
>
> Well, there is not much to it. We found out we can alias "unique" and
> "owned by C".
>
> We initialize the refcount to 0 when we initialize the request
> structure. This happens at queue creation time.

And IIRC this refcount is only on the Rust side, since you store it in
some private data, right?

> When C block layer hands over a request for processing to a Rust driver,
> we `debug_assert!` that the refcount is 0. We unsafely invent an
> `Owned<Request<_>>` and pass that to the driver.

And you don't increment the refcount?

> The driver has the option of `into_shared` to obtain an
> `ARef<Request<_>>`. We use this for remote completion and timer
> completion in rnull.

This operation will set the refcount to 2?

> In most drivers, when the driver hands off the request to the hardware,
> the driver will stop accounting for the request. The `Owned<Request<_>>`
> is consumed when issuing to the driver, or the driver could simply drop
> it. Refcount goes to 1. An ID is passed along to hardware.

And with the current API design it must let go of all `ARef<Request<_>>`
because otherwise it can't call `end_ok` (or some other method?).

> When a completion comes back from hardware, it carries the ID. We use
> the C block layer `tag_to_rq` machinery to turn this ID back into an
> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.

(because if not, this would be wrong)

I have some idea what we should do for the safety requirements of
`Ownable`, but still need some time to write it down.

---
Cheers,
Benno

> There was simply no need to have two separate values for "owned by C"
> and "owned by Rust, not referenced".
>
> Best regards,
> Andreas Hindborg

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Mon Jul 7, 2025 at 1:12 PM CEST, Andreas Hindborg wrote:
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>> Ah right, I forgot about this. What was the refcount characteristics of
>>>>> this again?
>>>>>
>>>>> *  1 = in flight, owned by C
>>>>> *  2 = in flight, owned by Rust
>>>>> * >2 = in flight, owned by Rust + additional references used by Rust
>>>>>        code
>>>>>
>>>>> Correct? Maybe @Andreas can check.
>>>>
>>>> We have been a bit back and forth on this. This is how we would like it
>>>> going forward:
>>>>
>>>>
>>>> /// There are three states for a request that the Rust bindings care about:
>>>> ///
>>>> /// - 0: The request is owned by C block layer or is uniquely referenced (by [`Owned<_>`]).
>>>> /// - 1: The request is owned by Rust abstractions but is not referenced.
>>>> /// - 2+: There is one or more [`ARef`] instances referencing the request.
>>>
>>> Huh, now I'm more confused... Could you go into the details again?
>>
>> Well, there is not much to it. We found out we can alias "unique" and
>> "owned by C".
>>
>> We initialize the refcount to 0 when we initialize the request
>> structure. This happens at queue creation time.
>
> And IIRC this refcount is only on the Rust side, since you store it in
> some private data, right?

Yes.

>
>> When C block layer hands over a request for processing to a Rust driver,
>> we `debug_assert!` that the refcount is 0. We unsafely invent an
>> `Owned<Request<_>>` and pass that to the driver.
>
> And you don't increment the refcount?

No, we leave it at 0.

>
>> The driver has the option of `into_shared` to obtain an
>> `ARef<Request<_>>`. We use this for remote completion and timer
>> completion in rnull.
>
> This operation will set the refcount to 2?

Yes.

>
>> In most drivers, when the driver hands off the request to the hardware,
>> the driver will stop accounting for the request. The `Owned<Request<_>>`
>> is consumed when issuing to the driver, or the driver could simply drop
>> it. Refcount goes to 1. An ID is passed along to hardware.
>
> And with the current API design it must let go of all `ARef<Request<_>>`
> because otherwise it can't call `end_ok` (or some other method?).

Exactly. That one will take an `Owned<Request<_>>` (previously `Unique`).

>
>> When a completion comes back from hardware, it carries the ID. We use
>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>
> (because if not, this would be wrong)

I hope it is not wrong 😆 You can see the latest and greatest here [1].

>
> I have some idea what we should do for the safety requirements of
> `Ownable`, but still need some time to write it down.

Great!


Best regards,
Andreas Hindborg


[1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Tue Jul 8, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>>> When a completion comes back from hardware, it carries the ID. We use
>>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>>
>> (because if not, this would be wrong)
>
> I hope it is not wrong 😆 You can see the latest and greatest here [1].

Thanks for the pointer, the "implementation details" in the docs on
`Request` were very helpful! This is another argument for not having the
blanket impl for `Ownable` when `T: Refcount` I mentioned in the other
thread.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398

The comment in line 424 looks wrong?

---
Cheers,
Benno

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jul 8, 2025 at 3:15 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>> On Mon Jul 7, 2025 at 3:21 PM CEST, Andreas Hindborg wrote:
>>>> When a completion comes back from hardware, it carries the ID. We use
>>>> the C block layer `tag_to_rq` machinery to turn this ID back into an
>>>> `Owned<Request<_>>`. In that process, we check that `refcount == 1` and
>>>> if so, we set refcount to 0 and invent an `Owned<Request<_>>`.
>>>
>>> (because if not, this would be wrong)
>>
>> I hope it is not wrong 😆 You can see the latest and greatest here [1].
>
> Thanks for the pointer, the "implementation details" in the docs on
> `Request` were very helpful! This is another argument for not having the
> blanket impl for `Ownable` when `T: Refcount` I mentioned in the other
> thread.
>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/tree/rust/kernel/block/mq/request.rs?h=rnull-v6.15#n398
>
> The comment in line 424 looks wrong?

Yes, nice catch. The assert is correct though.


Best regards,
Andreas Hindborg

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Oliver Mangold]

On 250707 1133, Benno Lossin wrote:
> On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
> > On 250702 1524, Benno Lossin wrote:
> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
> >> > @@ -132,3 +134,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`].
> >> > +///
> >> > +/// # Safety
> >> > +///
> >> > +/// Implementers must ensure that:
> >> > +///
> >> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
> >> > +///   exactly one [`ARef<Self>`] exists.
> >>
> >> This shouldn't be required?
> >
> > Ehm, why not? `Owned<T>` is supposed to be unique.
> 
> It's not needed as a safety requirement for implementing the trait. If
> the implementation only contains sound code, then `Owned::from_raw`
> should already ensure that `Owned<Self>` is only created if there is
> exactly one reference to it.

Okay, got it now. I guess you are right, it is not strictly needed. If the
requirement should be removed, not sure, though. Isn't it error-prone if it
explicitly stated here (again) that it is required?

> >> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
> >> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
> >> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
> >> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
> >> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
> >>
> >> This also seems pretty weird...
> >>
> >> I feel like `OwnableRefCounted` is essentially just a compatibility
> >> condition between `Ownable` and `RefCounted`. It ensures that the
> >> ownership declared in `Ownable` corresponds to exactly one refcount
> >> declared in `RefCounted`.
> >>
> >> That being said, I think a `RefCounted` *always* canonically is
> >> `Ownable` by the following impl:
> >>
> >>     unsafe impl<T: RefCounted> Ownable for T {
> >>         unsafe fn release(this: NonNull<Self>) {
> >>             T::dec_ref(this)
> >>         }
> >>     }
> >>
> >> So I don't think that we need this trait at all?
> >
> > No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
> > `try_from_shared()` implementation. It is not even a given that the
> > function can implemented, if all the kernel exposes are some kind of
> > `inc_ref()` and `dec_ref()`.
> 
> I don't understand this paragraph.

What I mean is, to convert from an `ARef` to an `Owned`, it is necessary to
check that there is only one reference. The API of the underlying object
might not provide that.

About the above documentation, it is a bit convoluted, because I had the
case of `mq::Request` in mind, where the refcount needs to be changed
during conversion.

> > Also there are more complicated cases like with `Mq::Request`, where the
> > existence of an `Owned<T>` cannot be represented by the same refcount value
> > as the existence of exactly one `ARef<T>`.
> 
> Ah right, I forgot about this. What was the refcount characteristics of
> this again?
> 
> *  1 = in flight, owned by C
> *  2 = in flight, owned by Rust
> * >2 = in flight, owned by Rust + additional references used by Rust
>        code
> 
> Correct? Maybe @Andreas can check.
> 
> >> > +///
> >> > +/// # Examples
> >>
> >> If we're having an example here, then we should also have on on `Owned`.
> >
> > Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
> > because it is a more complex idea than `Ownable`.
> >
> > If I remember correctly, I didn't create one for `Owned`, as it should
> > probably more or less the same as for `ARef` and the one there has even
> > more problems of the kind you are pointing out. So maybe it would be best
> > to wait until someone fixes that and have the fixed version copied over to
> > `Owned` in the process?
> 
> Wait which problems on `ARef` do you mean? I disagree that `Owned` and
> `ARef` have the same example. `Owned` should expose operations that
> `ARef` can't otherwise there would be no value in using `Owned`.

I mean it uses a `struct Empty {}`, which doesn't do any refcounting and
the safety requirements of `ARef::from_raw()` are also not fulfilled.

The point of `Owned` is not that it provides more operations than `ARef`
but rather that it provides less. The reference cannot be cloned. Actually
it is not supposed to provide much features at all, except for freeing the
underlying object when it is dropped.

It is supposed to just be a safe wrapper around a `*T`, marking that the
reference is Owned/Unique. Safe functions defined elsewhere can then take a
`Owned<T>` or `&mut Owned<T>` which provides the assurance of
ownership/uniqueness.

> >> > +///
> >> > +/// 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::types::{
> >> > +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
> >> > +/// };
> >> > +///
> >> > +/// struct Foo {
> >> > +///     refcount: Cell<usize>,
> >> > +/// }
> >> > +///
> >> > +/// impl Foo {
> >> > +///     fn new() -> Result<Owned<Self>, AllocError> {
> >> > +///         // Use a `KBox` to handle the actual allocation.
> >> > +///         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.");
> >>
> >> I'm not really convinced that an example using `KBox` is a good one...
> >> Maybe we should just have a local invisible `bindings` module that
> >> exposes a `-> *mut foo`. (internally it can just create a KBox`)
> >
> > The example would become quite a bit more complicated then, no?
> 
> Just hide those parts behind `#` lines in the example.

I don't know about this method. Can you give an example on how that works?

> ---
> Cheers,
> Benno

Re: [PATCH v11 4/4] rust: Add `OwnableRefCounted`

[Benno Lossin]

On Tue Jul 8, 2025 at 11:36 AM CEST, Oliver Mangold wrote:
> On 250707 1133, Benno Lossin wrote:
>> On Mon Jul 7, 2025 at 10:07 AM CEST, Oliver Mangold wrote:
>> > On 250702 1524, Benno Lossin wrote:
>> >> On Wed Jun 18, 2025 at 2:27 PM CEST, Oliver Mangold wrote:
>> >> > @@ -132,3 +134,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`].
>> >> > +///
>> >> > +/// # Safety
>> >> > +///
>> >> > +/// Implementers must ensure that:
>> >> > +///
>> >> > +/// - [`try_from_shared()`](OwnableRefCounted::into_shared) only returns an [`Owned<Self>`] if
>> >> > +///   exactly one [`ARef<Self>`] exists.
>> >>
>> >> This shouldn't be required?
>> >
>> > Ehm, why not? `Owned<T>` is supposed to be unique.
>> 
>> It's not needed as a safety requirement for implementing the trait. If
>> the implementation only contains sound code, then `Owned::from_raw`
>> should already ensure that `Owned<Self>` is only created if there is
>> exactly one reference to it.
>
> Okay, got it now. I guess you are right, it is not strictly needed. If the
> requirement should be removed, not sure, though. Isn't it error-prone if it
> explicitly stated here (again) that it is required?

You can move it to the normal docs of the trait, but it shouldn't be a
safety requirement.

>> >> > +/// - [`into_shared()`](OwnableRefCounted::into_shared) set the reference count to the value which
>> >> > +///   the returned [`ARef<Self>`] expects for an object with a single reference in existence. This
>> >> > +///   implies that if [`into_shared()`](OwnableRefCounted::into_shared) is left on the default
>> >> > +///   implementation, which just rewraps the underlying object, the reference count needs not to be
>> >> > +///   modified when converting an [`Owned<Self>`] to an [`ARef<Self>`].
>> >>
>> >> This also seems pretty weird...
>> >>
>> >> I feel like `OwnableRefCounted` is essentially just a compatibility
>> >> condition between `Ownable` and `RefCounted`. It ensures that the
>> >> ownership declared in `Ownable` corresponds to exactly one refcount
>> >> declared in `RefCounted`.
>> >>
>> >> That being said, I think a `RefCounted` *always* canonically is
>> >> `Ownable` by the following impl:
>> >>
>> >>     unsafe impl<T: RefCounted> Ownable for T {
>> >>         unsafe fn release(this: NonNull<Self>) {
>> >>             T::dec_ref(this)
>> >>         }
>> >>     }
>> >>
>> >> So I don't think that we need this trait at all?
>> >
>> > No. For an `ARef<T>` to be converted to an `Owned<T>` it requires a
>> > `try_from_shared()` implementation. It is not even a given that the
>> > function can implemented, if all the kernel exposes are some kind of
>> > `inc_ref()` and `dec_ref()`.
>> 
>> I don't understand this paragraph.
>
> What I mean is, to convert from an `ARef` to an `Owned`, it is necessary to
> check that there is only one reference. The API of the underlying object
> might not provide that.
>
> About the above documentation, it is a bit convoluted, because I had the
> case of `mq::Request` in mind, where the refcount needs to be changed
> during conversion.

So I think I got a bit confused with my initial question. We do need
this trait in order to enable the `ARef -> Owned` conversion. But the
other way is going to be fine if we choose to add the blanket impl
above.

Now the question is do we want `T: RefCounted` to imply `T: Ownable`? I
think the answer is yes, but it sadly conflicts with `AlwaysRefCounted`,
since if `T: AlwaysRefCounted` it must not implement `Ownable`.

So to me it seems this point has become moot.

>> > Also there are more complicated cases like with `Mq::Request`, where the
>> > existence of an `Owned<T>` cannot be represented by the same refcount value
>> > as the existence of exactly one `ARef<T>`.
>> 
>> Ah right, I forgot about this. What was the refcount characteristics of
>> this again?
>> 
>> *  1 = in flight, owned by C
>> *  2 = in flight, owned by Rust
>> * >2 = in flight, owned by Rust + additional references used by Rust
>>        code
>> 
>> Correct? Maybe @Andreas can check.
>> 
>> >> > +///
>> >> > +/// # Examples
>> >>
>> >> If we're having an example here, then we should also have on on `Owned`.
>> >
>> > Yes, maybe. I mostly felt the need to create one for `OwnableRefCounted`
>> > because it is a more complex idea than `Ownable`.
>> >
>> > If I remember correctly, I didn't create one for `Owned`, as it should
>> > probably more or less the same as for `ARef` and the one there has even
>> > more problems of the kind you are pointing out. So maybe it would be best
>> > to wait until someone fixes that and have the fixed version copied over to
>> > `Owned` in the process?
>> 
>> Wait which problems on `ARef` do you mean? I disagree that `Owned` and
>> `ARef` have the same example. `Owned` should expose operations that
>> `ARef` can't otherwise there would be no value in using `Owned`.
>
> I mean it uses a `struct Empty {}`, which doesn't do any refcounting and
> the safety requirements of `ARef::from_raw()` are also not fulfilled.

Right.

> The point of `Owned` is not that it provides more operations than `ARef`
> but rather that it provides less. The reference cannot be cloned. Actually
> it is not supposed to provide much features at all, except for freeing the
> underlying object when it is dropped.

Both our statements are true at the same time: `Owned<T>` itself has
less operations available compared to `ARef`, but `Owned<Specific>` has
potentially more operations than `ARef<Speceific>`. So eg in the
`Request` case, one can only call `end_ok` if you have an `Owned` one.

> It is supposed to just be a safe wrapper around a `*T`, marking that the
> reference is Owned/Unique. Safe functions defined elsewhere can then take a
> `Owned<T>` or `&mut Owned<T>` which provides the assurance of
> ownership/uniqueness.

Well it should also have compatibility with `RefCounted` if it
implements `OwnableRefCounted`.

>> >> > +///
>> >> > +/// 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::types::{
>> >> > +///     ARef, RefCounted, Owned, Ownable, OwnableRefCounted,
>> >> > +/// };
>> >> > +///
>> >> > +/// struct Foo {
>> >> > +///     refcount: Cell<usize>,
>> >> > +/// }
>> >> > +///
>> >> > +/// impl Foo {
>> >> > +///     fn new() -> Result<Owned<Self>, AllocError> {
>> >> > +///         // Use a `KBox` to handle the actual allocation.
>> >> > +///         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.");
>> >>
>> >> I'm not really convinced that an example using `KBox` is a good one...
>> >> Maybe we should just have a local invisible `bindings` module that
>> >> exposes a `-> *mut foo`. (internally it can just create a KBox`)
>> >
>> > The example would become quite a bit more complicated then, no?
>> 
>> Just hide those parts behind `#` lines in the example.
>
> I don't know about this method. Can you give an example on how that works?

See the second code block in [1].

[1]: https://doc.rust-lang.org/rustdoc/write-documentation/what-to-include.html#examples

---
Cheers,
Benno

Re: [PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[Danilo Krummrich]

On 6/18/25 2:27 PM, Oliver Mangold wrote:
> This allows to convert between ARef<T> and Owned<T> by
> implementing the new trait OwnedRefCounted.

Thanks for working on this!

Please make sure to use scripts/get_maintainer.pl in order to make sure to send
your series to all relevant maintainers and reviewers. I think you forgot some
of them.

Re: [PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[Oliver Mangold]

On 250805 1923, Danilo Krummrich wrote:
> On 6/18/25 2:27 PM, Oliver Mangold wrote:
> > This allows to convert between ARef<T> and Owned<T> by
> > implementing the new trait OwnedRefCounted.
> 
> Thanks for working on this!
> 
> Please make sure to use scripts/get_maintainer.pl in order to make sure to send
> your series to all relevant maintainers and reviewers. I think you forgot some
> of them.

Okay. Sorry, I will make sure for the next revision.

Re: [PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[Andreas Hindborg]

Hi Oliver,

"Oliver Mangold" <oliver.mangold@pm.me> writes:

> This allows to convert between ARef<T> and Owned<T> by
> implementing the new trait OwnedRefCounted.
>
> This way we will have a shared/unique reference counting scheme
> for types with built-in refcounts in analogy to Arc/UniqueArc.
>
> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> ---

In case it might save you some work, I rebased your series on v6.17-rc1:

https://github.com/metaspace/linux/commits/217c7ba3b95213abf9c1ce1a98ca496a48401f10


Best regards,
Andreas Hindborg

Re: [PATCH v11 0/4] New trait OwnableRefCounted for ARef<->Owned conversion.

[Oliver Mangold]

On 250815 1212, Andreas Hindborg wrote:
> Hi Oliver,
> 
> "Oliver Mangold" <oliver.mangold@pm.me> writes:
> 
> > This allows to convert between ARef<T> and Owned<T> by
> > implementing the new trait OwnedRefCounted.
> >
> > This way we will have a shared/unique reference counting scheme
> > for types with built-in refcounts in analogy to Arc/UniqueArc.
> >
> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
> > ---
> 
> In case it might save you some work, I rebased your series on v6.17-rc1:
> 
> https://github.com/metaspace/linux/commits/217c7ba3b95213abf9c1ce1a98ca496a48401f10
> 
Thanks a lot. I will use it. I hope I find some time soon.

Oliver