[v9] New trait OwnableRefCounted for ARef<->Owned conversion.

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

Posted by Oliver Mangold 8 months, 4 weeks ago

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

-- 
2.49.0

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

Posted by Andreas Hindborg 8 months, 1 week ago

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

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

That would allow the code in this example, but prevent the issue
outlined above.

Best regards,
Andreas Hindborg

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

Posted by Oliver Mangold 8 months, 1 week ago

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

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.

Best regards,

Oliver

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

Posted by Andreas Hindborg 7 months, 2 weeks ago

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

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

Posted by Andreas Hindborg 7 months, 2 weeks ago

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

[PATCH v9 1/5] rust: types: Add Ownable/Owned types
[PATCH v9 2/5] rust: Rename AlwaysRefCounted to RefCounted
[PATCH v9 3/5] rust: Add missing SAFETY documentation for ARef example
[PATCH v9 4/5] rust: kbuild: provide `RUSTC_HAS_DO_NOT_RECOMMEND` symbol
[PATCH v9 5/5] rust: Add OwnableRefCounted and SimpleOwnableRefCounted