Messages in this thread | | | Date | Thu, 13 Apr 2023 09:19:00 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v4 08/13] rust: introduce `ARef` |
| |
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`.
> +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. }
Additionally, I think that `AlwaysRefCounted::inc_ref` should not be safe, as the caller must not deallocate the memory until the refcount is zero.
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.
> + > +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 >
| |