[PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[Andreas Hindborg]

Clarify that implementers of `AlwaysReferenceCounted` must prevent the
implementer from being directly initialized by users.

It is a violation of the safety requirements of `AlwaysReferenceCounted` if
its implementers can be initialized on the stack by users. Although this
follows from the safety requirements, it is not immediately obvious.

The following example demonstrates the issue. Note that the safety
requirements for implementing `AlwaysRefCounted` and for calling
`ARef::from_raw` are satisfied.

  struct Empty {}

  unsafe impl AlwaysRefCounted for Empty {
      fn inc_ref(&self) {}
      unsafe fn dec_ref(_obj: NonNull<Self>) {}
  }

  fn unsound() -> ARef<Empty> {
      use core::ptr::NonNull;
      use kernel::types::{ARef, RefCounted};

      let mut data = Empty {};
      let ptr = NonNull::<Empty>::new(&mut data).unwrap();
      let aref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };

      aref
  }

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/types.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 9d0471afc9648f2973235488b441eb109069adb1..193a1356f0df6fede428498485aaef19b4c845c6 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -409,6 +409,10 @@ pub const fn raw_get(this: *const Self) -> *mut T {
 /// 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.)
+///
+/// Note: This means that implementers must prevent users from directly
+/// initializing the implementer. Otherwise users could initialize the
+/// implementer on the stack, which would violate the safety requirements.
 pub unsafe trait AlwaysRefCounted {
     /// Increments the reference count on the object.
     fn inc_ref(&self);

---
base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
change-id: 20250502-aref-from-raw-e110b3e6dbf5

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

Re: [PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[Alice Ryhl]

On Fri, May 02, 2025 at 01:53:57PM +0200, Andreas Hindborg wrote:
> Clarify that implementers of `AlwaysReferenceCounted` must prevent the
> implementer from being directly initialized by users.
> 
> It is a violation of the safety requirements of `AlwaysReferenceCounted` if
> its implementers can be initialized on the stack by users. Although this
> follows from the safety requirements, it is not immediately obvious.
> 
> The following example demonstrates the issue. Note that the safety
> requirements for implementing `AlwaysRefCounted` and for calling
> `ARef::from_raw` are satisfied.
> 
>   struct Empty {}
> 
>   unsafe impl AlwaysRefCounted for Empty {
>       fn inc_ref(&self) {}
>       unsafe fn dec_ref(_obj: NonNull<Self>) {}
>   }
> 
>   fn unsound() -> ARef<Empty> {
>       use core::ptr::NonNull;
>       use kernel::types::{ARef, RefCounted};
> 
>       let mut data = Empty {};
>       let ptr = NonNull::<Empty>::new(&mut data).unwrap();
>       let aref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
> 
>       aref
>   }

I don't think it's entirely impossible to write an AlwaysRefCounted
value that can be on the stack. The type just needs a lifetime
parameter. For example, this API is not unsound:

struct MyDataStorage {
    // ...
}

impl MyDataStorage {
    fn as_aref(&self) -> ARef<MyData<'_>> {
        unsafe { ARef::from_raw(ptr::from_ref(self).cast()) }
    }
}

#[repr(transparent)]
struct MyData<'s> {
    storage: MyDataStorage,
    _lifetime: PhantomData<&'s MyDataStorage>,
}

unsafe impl AlwaysRefCounted for MyData<'_> {
    fn inc_ref(&self) {}
    unsafe fn dec_ref(_obj: NonNull<Self>) {}
}

impl Deref for MyData<'_> {
    type Target = MyDataStorage;
    fn deref(&self) -> &MyDataStorage {
        &self.storage
    }
}

Alice

Re: [PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, May 02, 2025 at 01:53:57PM +0200, Andreas Hindborg wrote:
>> Clarify that implementers of `AlwaysReferenceCounted` must prevent the
>> implementer from being directly initialized by users.
>>
>> It is a violation of the safety requirements of `AlwaysReferenceCounted` if
>> its implementers can be initialized on the stack by users. Although this
>> follows from the safety requirements, it is not immediately obvious.
>>
>> The following example demonstrates the issue. Note that the safety
>> requirements for implementing `AlwaysRefCounted` and for calling
>> `ARef::from_raw` are satisfied.
>>
>>   struct Empty {}
>>
>>   unsafe impl AlwaysRefCounted for Empty {
>>       fn inc_ref(&self) {}
>>       unsafe fn dec_ref(_obj: NonNull<Self>) {}
>>   }
>>
>>   fn unsound() -> ARef<Empty> {
>>       use core::ptr::NonNull;
>>       use kernel::types::{ARef, RefCounted};
>>
>>       let mut data = Empty {};
>>       let ptr = NonNull::<Empty>::new(&mut data).unwrap();
>>       let aref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
>>
>>       aref
>>   }
>
> I don't think it's entirely impossible to write an AlwaysRefCounted
> value that can be on the stack. The type just needs a lifetime
> parameter. For example, this API is not unsound:
>
> struct MyDataStorage {
>     // ...
> }
>
> impl MyDataStorage {
>     fn as_aref(&self) -> ARef<MyData<'_>> {
>         unsafe { ARef::from_raw(ptr::from_ref(self).cast()) }
>     }
> }
>
> #[repr(transparent)]
> struct MyData<'s> {
>     storage: MyDataStorage,
>     _lifetime: PhantomData<&'s MyDataStorage>,
> }
>
> unsafe impl AlwaysRefCounted for MyData<'_> {
>     fn inc_ref(&self) {}
>     unsafe fn dec_ref(_obj: NonNull<Self>) {}
> }
>
> impl Deref for MyData<'_> {
>     type Target = MyDataStorage;
>     fn deref(&self) -> &MyDataStorage {
>         &self.storage
>     }
> }

Right. I would rephrase then:

It is a violation of the safety requirements of `AlwaysReferenceCounted`
if its implementers can be initialized on the stack by users and an
`ARef` referencing the object can outlive the object. Although this follows from
the safety requirements, it is not immediately obvious.

and

+/// Note: This means that implementers must prevent users from directly
+/// initializing the implementer when the implementer is `'static`. Otherwise users could
+/// initialize the implementer on
+/// the stack, which would violate the safety requirements.

What do you think?


Best regards,
Andreas Hindborg

Re: [PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[Alice Ryhl]

On Fri, May 2, 2025 at 2:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Alice Ryhl" <aliceryhl@google.com> writes:
>
> > On Fri, May 02, 2025 at 01:53:57PM +0200, Andreas Hindborg wrote:
> >> Clarify that implementers of `AlwaysReferenceCounted` must prevent the
> >> implementer from being directly initialized by users.
> >>
> >> It is a violation of the safety requirements of `AlwaysReferenceCounted` if
> >> its implementers can be initialized on the stack by users. Although this
> >> follows from the safety requirements, it is not immediately obvious.
> >>
> >> The following example demonstrates the issue. Note that the safety
> >> requirements for implementing `AlwaysRefCounted` and for calling
> >> `ARef::from_raw` are satisfied.
> >>
> >>   struct Empty {}
> >>
> >>   unsafe impl AlwaysRefCounted for Empty {
> >>       fn inc_ref(&self) {}
> >>       unsafe fn dec_ref(_obj: NonNull<Self>) {}
> >>   }
> >>
> >>   fn unsound() -> ARef<Empty> {
> >>       use core::ptr::NonNull;
> >>       use kernel::types::{ARef, RefCounted};
> >>
> >>       let mut data = Empty {};
> >>       let ptr = NonNull::<Empty>::new(&mut data).unwrap();
> >>       let aref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
> >>
> >>       aref
> >>   }
> >
> > I don't think it's entirely impossible to write an AlwaysRefCounted
> > value that can be on the stack. The type just needs a lifetime
> > parameter. For example, this API is not unsound:
> >
> > struct MyDataStorage {
> >     // ...
> > }
> >
> > impl MyDataStorage {
> >     fn as_aref(&self) -> ARef<MyData<'_>> {
> >         unsafe { ARef::from_raw(ptr::from_ref(self).cast()) }
> >     }
> > }
> >
> > #[repr(transparent)]
> > struct MyData<'s> {
> >     storage: MyDataStorage,
> >     _lifetime: PhantomData<&'s MyDataStorage>,
> > }
> >
> > unsafe impl AlwaysRefCounted for MyData<'_> {
> >     fn inc_ref(&self) {}
> >     unsafe fn dec_ref(_obj: NonNull<Self>) {}
> > }
> >
> > impl Deref for MyData<'_> {
> >     type Target = MyDataStorage;
> >     fn deref(&self) -> &MyDataStorage {
> >         &self.storage
> >     }
> > }
>
> Right. I would rephrase then:
>
> It is a violation of the safety requirements of `AlwaysReferenceCounted`
> if its implementers can be initialized on the stack by users and an
> `ARef` referencing the object can outlive the object. Although this follows from
> the safety requirements, it is not immediately obvious.
>
> and
>
> +/// Note: This means that implementers must prevent users from directly
> +/// initializing the implementer when the implementer is `'static`. Otherwise users could
> +/// initialize the implementer on
> +/// the stack, which would violate the safety requirements.
>
> What do you think?

Hmm. Perhaps we should say that you can never own it "by value". There
must always be pointer indirection.

Alice

Re: [PATCH] rust: elaborate safety requirements for `AlwaysReferenceCounted`

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, May 2, 2025 at 2:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Alice Ryhl" <aliceryhl@google.com> writes:
>>
>> > On Fri, May 02, 2025 at 01:53:57PM +0200, Andreas Hindborg wrote:
>> >> Clarify that implementers of `AlwaysReferenceCounted` must prevent the
>> >> implementer from being directly initialized by users.
>> >>
>> >> It is a violation of the safety requirements of `AlwaysReferenceCounted` if
>> >> its implementers can be initialized on the stack by users. Although this
>> >> follows from the safety requirements, it is not immediately obvious.
>> >>
>> >> The following example demonstrates the issue. Note that the safety
>> >> requirements for implementing `AlwaysRefCounted` and for calling
>> >> `ARef::from_raw` are satisfied.
>> >>
>> >>   struct Empty {}
>> >>
>> >>   unsafe impl AlwaysRefCounted for Empty {
>> >>       fn inc_ref(&self) {}
>> >>       unsafe fn dec_ref(_obj: NonNull<Self>) {}
>> >>   }
>> >>
>> >>   fn unsound() -> ARef<Empty> {
>> >>       use core::ptr::NonNull;
>> >>       use kernel::types::{ARef, RefCounted};
>> >>
>> >>       let mut data = Empty {};
>> >>       let ptr = NonNull::<Empty>::new(&mut data).unwrap();
>> >>       let aref: ARef<Empty> = unsafe { ARef::from_raw(ptr) };
>> >>
>> >>       aref
>> >>   }
>> >
>> > I don't think it's entirely impossible to write an AlwaysRefCounted
>> > value that can be on the stack. The type just needs a lifetime
>> > parameter. For example, this API is not unsound:
>> >
>> > struct MyDataStorage {
>> >     // ...
>> > }
>> >
>> > impl MyDataStorage {
>> >     fn as_aref(&self) -> ARef<MyData<'_>> {
>> >         unsafe { ARef::from_raw(ptr::from_ref(self).cast()) }
>> >     }
>> > }
>> >
>> > #[repr(transparent)]
>> > struct MyData<'s> {
>> >     storage: MyDataStorage,
>> >     _lifetime: PhantomData<&'s MyDataStorage>,
>> > }
>> >
>> > unsafe impl AlwaysRefCounted for MyData<'_> {
>> >     fn inc_ref(&self) {}
>> >     unsafe fn dec_ref(_obj: NonNull<Self>) {}
>> > }
>> >
>> > impl Deref for MyData<'_> {
>> >     type Target = MyDataStorage;
>> >     fn deref(&self) -> &MyDataStorage {
>> >         &self.storage
>> >     }
>> > }
>>
>> Right. I would rephrase then:
>>
>> It is a violation of the safety requirements of `AlwaysReferenceCounted`
>> if its implementers can be initialized on the stack by users and an
>> `ARef` referencing the object can outlive the object. Although this follows from
>> the safety requirements, it is not immediately obvious.
>>
>> and
>>
>> +/// Note: This means that implementers must prevent users from directly
>> +/// initializing the implementer when the implementer is `'static`. Otherwise users could
>> +/// initialize the implementer on
>> +/// the stack, which would violate the safety requirements.
>>
>> What do you think?
>
> Hmm. Perhaps we should say that you can never own it "by value". There
> must always be pointer indirection.

Yes, that could work. I'll send a new version.


Best regards,
Andreas Hindborg