Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230411054543.21278-1-wedsonaf@gmail.com> <20230411054543.21278-8-wedsonaf@gmail.com>
 <f8575380-e710-d505-837c-bfcabe0eff00@proton.me> <CANeycqp_WfJxFDrxJGQ=UO2HOKtKYJCbOwGu50vZKyQSzYuiPQ@mail.gmail.com>
 <9619d06c-d631-1edb-cf92-3a998e7b98f2@proton.me>
In-Reply-To: <9619d06c-d631-1edb-cf92-3a998e7b98f2@proton.me>
From:   Wedson Almeida Filho <wedsonaf@gmail.com>
Date:   Fri, 14 Apr 2023 06:00:57 -0300
Message-ID: <CANeycqpT6thLZeuFOQqdOFwamYuFLHN5=vwEaSVgC9cNzFtg_A@mail.gmail.com>
Subject: Re: [PATCH v4 08/13] rust: introduce `ARef`
To:     Benno Lossin <benno.lossin@proton.me>
Cc:     rust-for-linux@vger.kernel.org, Miguel Ojeda <ojeda@kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>,
        =?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>,
        linux-kernel@vger.kernel.org,
        Wedson Almeida Filho <walmeida@microsoft.com>,
        Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 13 Apr 2023 at 19:30, Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 13.04.23 19:06, Wedson Almeida Filho wrote:
> > On Thu, 13 Apr 2023 at 06:19, Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> On 11.04.23 07:45, Wedson Almeida Filho wrote:
> >>> From: Wedson Almeida Filho <walmeida@microsoft.com>
> >>>
> >>> This is an owned reference to an object that is always ref-counted. This
> >>> is meant to be used in wrappers for C types that have their own ref
> >>> counting functions, for example, tasks, files, inodes, dentries, etc.
> >>>
> >>> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
> >>> Signed-off-by: Wedson Almeida Filho <walmeida@microsoft.com>
> >>> ---
> >>> v1 -> v2: No changes
> >>> v2 -> v3: No changes
> >>> v3 -> v4: No changes
> >>>
> >>>    rust/kernel/types.rs | 107 +++++++++++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 107 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >>> index a4b1e3778da7..29db59d6119a 100644
> >>> --- a/rust/kernel/types.rs
> >>> +++ b/rust/kernel/types.rs
> >>> @@ -6,8 +6,10 @@ use crate::init::{self, PinInit};
> >>>    use alloc::boxed::Box;
> >>>    use core::{
> >>>        cell::UnsafeCell,
> >>> +    marker::PhantomData,
> >>>        mem::MaybeUninit,
> >>>        ops::{Deref, DerefMut},
> >>> +    ptr::NonNull,
> >>>    };
> >>>
> >>>    /// Used to transfer ownership to and from foreign (non-Rust) languages.
> >>> @@ -268,6 +270,111 @@ impl<T> Opaque<T> {
> >>>        }
> >>>    }
> >>>
> >>> +/// Types that are _always_ 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
> >>> +/// instances of a type.
> >>> +///
> >>> +/// # Safety
> >>> +///
> >>> +/// Implementers must ensure that increments to the reference count keep the object alive in memory
> >>> +/// 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.)
> >>
> >> `dec_ref` states below that it 'Frees the object when the count reaches
> >> zero.', this should also be stated here, since implementers should adhere
> >> to that when implementing `dec_ref`.
> >
> > This section is for safety requirements. Freeing the object doesn't
> > fall into this category.
>
> It still needs to be upheld by the implementer, since it is guaranteed by
> the documentation on the `dec_ref` function. Even non-safety requirements
> are listed on the `unsafe` traits, if users should be able to rely on them.
> If users should not rely on this, then maybe change the docs of `dec_ref`
> to "when the refcount reaches zero, the object might be freed.".

I disagree that non-safety requirements should be listed under the
Safety section.

This section is meant for rules that implementers must adhere to to
ensure their implementations are safe. So it's usually read before
writing a "SAFETY:" comment for their "unsafe impl" blocks -- adding
extraneous information is counterproductive.

> >>> +pub unsafe trait AlwaysRefCounted {
> >>> +    /// Increments the reference count on the object.
> >>> +    fn inc_ref(&self);
> >>
> >>
> >>
> >>> +
> >>> +    /// Decrements the reference count on the object.
> >>> +    ///
> >>> +    /// Frees the object when the count reaches zero.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// Callers must ensure that there was a previous matching increment to the reference count,
> >>> +    /// and that the object is no longer used after its reference count is decremented (as it may
> >>> +    /// result in the object being freed), unless the caller owns another increment on the refcount
> >>> +    /// (e.g., it calls [`AlwaysRefCounted::inc_ref`] twice, then calls
> >>> +    /// [`AlwaysRefCounted::dec_ref`] once).
> >>> +    unsafe fn dec_ref(obj: NonNull<Self>);
> >>> +}
> >>> +
> >>> +/// An owned reference to an always-reference-counted object.
> >>> +///
> >>> +/// The object's reference count is automatically decremented when an instance of [`ARef`] is
> >>> +/// dropped. It is also automatically incremented when a new instance is created via
> >>> +/// [`ARef::clone`].
> >>> +///
> >>> +/// # Invariants
> >>> +///
> >>> +/// The pointer stored in `ptr` is non-null and valid for the lifetime of the [`ARef`] instance. In
> >>> +/// particular, the [`ARef`] instance owns an increment on the underlying object's reference count.
> >>> +pub struct ARef<T: AlwaysRefCounted> {
> >>> +    ptr: NonNull<T>,
> >>> +    _p: PhantomData<T>,
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> ARef<T> {
> >>> +    /// Creates a new instance of [`ARef`].
> >>> +    ///
> >>> +    /// It takes over an increment of the reference count on the underlying object.
> >>> +    ///
> >>> +    /// # Safety
> >>> +    ///
> >>> +    /// 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`].
> >>> +    pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
> >>> +        // INVARIANT: The safety requirements guarantee that the new instance now owns the
> >>> +        // increment on the refcount.
> >>> +        Self {
> >>> +            ptr,
> >>> +            _p: PhantomData,
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> Clone for ARef<T> {
> >>> +    fn clone(&self) -> Self {
> >>> +        self.inc_ref();
> >>> +        // SAFETY: We just incremented the refcount above.
> >>> +        unsafe { Self::from_raw(self.ptr) }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> Deref for ARef<T> {
> >>> +    type Target = T;
> >>> +
> >>> +    fn deref(&self) -> &Self::Target {
> >>> +        // SAFETY: The type invariants guarantee that the object is valid.
> >>> +        unsafe { self.ptr.as_ref() }
> >>> +    }
> >>> +}
> >>> +
> >>> +impl<T: AlwaysRefCounted> From<&T> for ARef<T> {
> >>> +    fn from(b: &T) -> Self {
> >>> +        b.inc_ref();
> >>> +        // SAFETY: We just incremented the refcount above.
> >>> +        unsafe { Self::from_raw(NonNull::from(b)) }
> >>> +    }
> >>> +}
> >>
> >> This impl seems unsound to me, as we can do this:
> >>
> >>       struct MyStruct {
> >>           raw: Opaque<bindings::my_struct>, // This has a `refcount_t` inside.
> >>       }
> >>
> >>       impl MyStruct {
> >>           fn new() -> Self { ... }
> >>       }
> >>
> >>       unsafe impl AlwaysRefCounted for MyStruct { ... } // Implemented correctly.
> >>
> >>       fn evil() -> ARef<MyStruct> {
> >>           let my_struct = MyStruct::new();
> >>           ARef::from(&my_struct) // We return a pointer to the stack!
> >>       }
> >>
> >> similarly, this can also be done with a `Box`:
> >>
> >>       fn evil2() -> ARef<MyStruct> {
> >>           let my_struct = Box::new(MyStruct::new());
> >>           ARef::from(&*my_struct)
> >>           // Box is freed here, even just dropping the `ARef` will result in
> >>           // a UAF.
> >>       }
> >
> > This implementation of `AlwaysRefCounted` is in violation of the
> > safety requirements of the trait, namely:
> >
> > /// Implementers must ensure that increments to the reference count
> > keep the object alive in memory
> > /// 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.)
> >
> > It boils down `MyStruct::new` in your example. It's not refcounted.
> >
> >> Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe,
> >> as the caller must not deallocate the memory until the refcount is zero.
> >
> > The existence of an `&T` is evidence that the refcount is non-zero, so
> > it is safe to increment it. The caller cannot free the object without
> > violating the safety requirements.
> >
> >> Another pitfall of `ARef`: it does not deallocate the memory when the
> >> refcount reaches zero. People might expect that this code would not leak
> >> memory:
> >>
> >>       let foo = Box::try_new(Foo::new())?;
> >>       let foo = Box::leak(foo); // Leak the box, such that we do not
> >>                                 // deallocate the memory too early.
> >>       let foo = ARef::from(foo);
> >>       drop(foo); // refcount is now zero, but the memory is never deallocated.
> >
> > This is also in violation of the safety requirements of `AlwaysRefCounted`.
>
> It seems I have misunderstood the term "always reference counted".
> We should document this in more detail, since this places a lot of
> constraints on the implementers:
>
>      Implementing `AlwaysRefCounted` for `T` places the following constraint on shared references `&T`:
>      - Every `&T` points to memory that is not deallocated until the reference count reaches zero.
>      - The existence of `&T` proves that the reference count is at least 1.

This is implied by the existing safety rules.

>      This has some important consequences:
>      - Exposing safe a way to get `T` is not allowed, since stack allocations are freed when the scope
>        ends even though the reference count is non-zero.

Stack allocations are ok, as long as they wait for the refcount to
drop to zero before the variable goes out of scope.

>      - Similarly giving safe access to `Box<T>` or other smart pointers is not allowed, since a `Box` can
>        be freed independent from the reference count.

`ARef<T>` is a smart pointer and it is definitely allowed.

Similarly to stack allocations I mention above, a `Box<T>`
implementation is conceivable as long as it ensures that the
allocation is freed only once the refcount reaches zero, for example,
by having a drop implementation that performs such a wait. (IOW, when
`Box<T>` goes out of scope, it always calls `drop` on `T` before
actually freeing the memory, so this implementation could block until
it is safe to do so, i.e., until the refcount reaches zero.)

>      This type is intended to be implemented for C types that embedd a `refcount_t` and that are both
>      created and destroyed by C. Static references also work with this type, since they stay live
>      indefinitely.

Embedding a `refcount_t` is not a requirement. I already mention in
the documentation that this is usually used for C structs and that
Rust code should use `Arc`.

>      Implementers must also ensure that they never give out `&mut T`, since
>      - it can be reborrowed as `&T`,
>      - converted to `ARef<T>`,
>      - which can yield a `&T` that is alive at the same time as the `&mut T`.

I agree with this. And I don't think this is a direct consequence of
the safety requirements, so I think it makes sense to add something
that covers this.

> >>> +
> >>> +impl<T: AlwaysRefCounted> Drop for ARef<T> {
> >>> +    fn drop(&mut self) {
> >>> +        // SAFETY: The type invariants guarantee that the `ARef` owns the reference we're about to
> >>> +        // decrement.
> >>> +        unsafe { T::dec_ref(self.ptr) };
> >>> +    }
> >>> +}
> >>> +
> >>>    /// A sum type that always holds either a value of type `L` or `R`.
> >>>    pub enum Either<L, R> {
> >>>        /// Constructs an instance of [`Either`] containing a value of type `L`.
> >>> --
> >>> 2.34.1
> >>>
> >>
>