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

["Benno Lossin" <lossin@kernel.org>]

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