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

[Oliver Mangold <oliver.mangold@pm.me>]

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