Re: [PATCH v9 3/5] rust: Add missing SAFETY documentation for ARef example

[Andreas Hindborg <a.hindborg@kernel.org>]

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

> On 250409 1126, Andreas Hindborg wrote:
>> "Oliver Mangold" <oliver.mangold@pm.me> writes:
>> 
>> > SAFETY comment in rustdoc example was just 'TODO'. Fixed.
>> >
>> > Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
>> > ---
>> >  rust/kernel/types.rs | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> > index c8b78bcad259132808cc38c56b9f2bd525a0b755..db29f7c725e631c11099fa9122901ec2b3f4a039 100644
>> > --- a/rust/kernel/types.rs
>> > +++ b/rust/kernel/types.rs
>> > @@ -492,7 +492,7 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>> >      ///
>> >      /// struct Empty {}
>> >      ///
>> > -    /// # // SAFETY: TODO.
>> > +    /// // SAFETY: We do not free anything.
>> 
>> How about:
>> 
>>   This implementation will never free the underlying object, so the
>>   object is kept alive longer than the safety requirement specifies.
>
> Yes, was rather sloppy wording. Thanks, I will use your version.
>
>> >      /// unsafe impl RefCounted for Empty {
>> >      ///     fn inc_ref(&self) {}
>> >      ///     unsafe fn dec_ref(_obj: NonNull<Self>) {}
>> > @@ -500,7 +500,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`.
>> 
>> This is not sufficient. The safety requirement is:
>> 
>>   Callers must ensure that the reference count was incremented at least once, and that they
>>   are properly relinquishing one increment. That is, if there is only one increment, callers
>>   must not use the underlying object anymore -- it is only safe to do so via the newly
>>   created [`ARef`].
>> 
>> How about:
>> 
>>   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`.
>> 
>> I think this example actually exposes a soundness hole. When the
>> underlying object is allocated on the stack, the safety requirements are
>> not sufficient to ensure the lifetime of the object. We could safely
>> return `data_ref` and have the underlying object go away. We should add
>> to the safety requirement of `ARef::from_raw`:
>> 
>>   `ptr` must be valid while the refcount is positive.
>
> Wouldn't this already be covered by the below in the doc for
> AlwaysRefCounted?
>
>     Implementers must ensure that increments to the reference count keep
>     the object alive in memory at least until matching decrements are
>     performed."

No, I don't think that is enough. We can implement the trait properly
with refcounts and still we can allocate an object on the stack and then
do a `from_raw` on that object without violating any safety
requirements. I think the `ARef::from_raw` should have the safety
requirement above. But we can do that as a separate patch.

>
> OTOH, it also says this (which would be violated):
>
>     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.)"
>
> Should I change the example to one with an actual reference count?
> Not sure, would make it more complex and less readable, of course.

No I think that is fine. `Empty` is reference counted in the sense that
the refcount can considered to always be positive.



Best regards,
Andreas Hindborg