Messages in this thread | | | From | Wedson Almeida Filho <> | Date | Thu, 13 Apr 2023 14:06:01 -0300 | Subject | Re: [PATCH v4 08/13] rust: introduce `ARef` |
| |
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.
> > +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`.
> > + > > +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 > > >
| |