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

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

Andreas Hindborg <a.hindborg@kernel.org> writes:

> 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.

On second thought, I think you are right. I was trying to implement a
counter example, and I think it is not possible to implement
`RefCounted` while following the safety requirements in a way that would
trigger this issue. Implementers will have to make sure that the the
type that implement `RefCounted` cannot be directly constructed. In
other words the implementation for `Empty` is illegal in this regard.

We can do this instead for the example


    mod empty {
        use crate::alloc::KBox;
        use core::ptr::NonNull;

        pub struct Empty {
            // Prevent direct construction
            _p: (),
        }

        // 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 super::RefCounted for Empty {
            fn inc_ref(&self) {}

            unsafe fn dec_ref(_obj: NonNull<Self>) {}
        }

        impl Empty {
            pub fn new() -> NonNull<Self> {
                NonNull::new(KBox::into_raw(
                    KBox::new(Self { _p: () }, kernel::alloc::flags::GFP_KERNEL).unwrap(),
                ))
                .unwrap()
            }
        }
    }

    let ptr = empty::Empty::new();

    // 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`.
    let data_ref: ARef<empty::Empty> = unsafe { ARef::from_raw(ptr) };
    let _raw_ptr: NonNull<empty::Empty> = ARef::into_raw(data_ref);



Best regards,
Andreas Hindborg