lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANeycqrVsbNJ+A+A26LXkBezBNUHvnZU2Q3_whexCwwG5ZcgPQ@mail.gmail.com> Date: Wed, 4 Jan 2023 15:29:48 +0000 From: Wedson Almeida Filho <wedsonaf@...il.com> To: Gary Guo <gary@...yguo.net> Cc: rust-for-linux@...r.kernel.org, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow` On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@...yguo.net> wrote: > > On Wed, 28 Dec 2022 06:03:43 +0000 > Wedson Almeida Filho <wedsonaf@...il.com> wrote: > > > This allows us to create references to a ref-counted allocation without > > double-indirection and that still allow us to increment the refcount to > > a new `Arc<T>`. > > > > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com> > > --- > > rust/kernel/sync.rs | 2 +- > > rust/kernel/sync/arc.rs | 97 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 98 insertions(+), 1 deletion(-) > > > > diff --git a/rust/kernel/sync.rs b/rust/kernel/sync.rs > > index 39b379dd548f..5de03ea83ea1 100644 > > --- a/rust/kernel/sync.rs > > +++ b/rust/kernel/sync.rs > > @@ -7,4 +7,4 @@ > > > > mod arc; > > > > -pub use arc::Arc; > > +pub use arc::{Arc, ArcBorrow}; > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs > > index dbc7596cc3ce..f68bfc02c81a 100644 > > --- a/rust/kernel/sync/arc.rs > > +++ b/rust/kernel/sync/arc.rs > > @@ -19,6 +19,7 @@ use crate::{bindings, error::Result, types::Opaque}; > > use alloc::boxed::Box; > > use core::{ > > marker::{PhantomData, Unsize}, > > + mem::ManuallyDrop, > > ops::Deref, > > ptr::NonNull, > > }; > > @@ -164,6 +165,18 @@ impl<T: ?Sized> Arc<T> { > > _p: PhantomData, > > } > > } > > + > > + /// Returns an [`ArcBorrow`] from the given [`Arc`]. > > + /// > > + /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method > > + /// receiver), but we have an [`Arc`] instead. Getting an [`ArcBorrow`] is free when optimised. > > + #[inline] > > + pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> { > > + // SAFETY: The constraint that the lifetime of the shared reference must outlive that of > > + // the returned `ArcBorrow` ensures that the object remains alive and that no mutable > > + // reference can be created. > > + unsafe { ArcBorrow::new(self.ptr) } > > + } > > } > > > > impl<T: ?Sized> Deref for Arc<T> { > > @@ -208,3 +221,87 @@ impl<T: ?Sized> Drop for Arc<T> { > > } > > } > > } > > + > > +/// A borrowed reference to an [`Arc`] instance. > > +/// > > +/// For cases when one doesn't ever need to increment the refcount on the allocation, it is simpler > > +/// to use just `&T`, which we can trivially get from an `Arc<T>` instance. > > +/// > > +/// However, when one may need to increment the refcount, it is preferable to use an `ArcBorrow<T>` > > +/// over `&Arc<T>` because the latter results in a double-indirection: a pointer (shared reference) > > +/// to a pointer (`Arc<T>`) to the object (`T`). An [`ArcBorrow`] eliminates this double > > +/// indirection while still allowing one to increment the refcount and getting an `Arc<T>` when/if > > +/// needed. > > +/// > > +/// # Invariants > > +/// > > +/// There are no mutable references to the underlying [`Arc`], and it remains valid for the > > +/// lifetime of the [`ArcBorrow`] instance. > > +/// > > +/// # Example > > +/// > > +/// ``` > > +/// use crate::sync::{Arc, ArcBorrow}; > > +/// > > +/// struct Example; > > +/// > > +/// fn do_something(e: ArcBorrow<'_, Example>) -> Arc<Example> { > > +/// e.into() > > +/// } > > +/// > > +/// let obj = Arc::try_new(Example)?; > > +/// let cloned = do_something(obj.as_arc_borrow()); > > +/// > > +/// // Assert that both `obj` and `cloned` point to the same underlying object. > > +/// assert!(core::ptr::eq(&*obj, &*cloned)); > > +/// ``` > > +pub struct ArcBorrow<'a, T: ?Sized + 'a> { > > + inner: NonNull<ArcInner<T>>, > > + _p: PhantomData<&'a ()>, > > +} > > + > > +impl<T: ?Sized> Clone for ArcBorrow<'_, T> { > > + fn clone(&self) -> Self { > > + *self > > + } > > +} > > + > > +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {} > > Couldn't this just be derived `Clone` and `Copy`? Indeed. I'll send a v2 with this. > > > + > > +impl<T: ?Sized> ArcBorrow<'_, T> { > > + /// Creates a new [`ArcBorrow`] instance. > > + /// > > + /// # Safety > > + /// > > + /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance: > > + /// 1. That `inner` remains valid; > > + /// 2. That no mutable references to `inner` are created. > > + unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self { > > + // INVARIANT: The safety requirements guarantee the invariants. > > + Self { > > + inner, > > + _p: PhantomData, > > + } > > + } > > +} > > + > > +impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> { > > + fn from(b: ArcBorrow<'_, T>) -> Self { > > + // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop` > > + // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the > > + // increment. > > + ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) }) > > + .deref() > > + .clone() > > + } > > +} > > It might be easier to follow if this is jsut `bindings::refcount_inc` > followed by `Arc::from_inner`? I'd prefer to keep the interactions with `refcount_t` in `Arc` only so that we can more easily change it in the future if we so choose. > > + > > +impl<T: ?Sized> Deref for ArcBorrow<'_, T> { > > + type Target = T; > > + > > + fn deref(&self) -> &Self::Target { > > + // SAFETY: By the type invariant, the underlying object is still alive with no mutable > > + // references to it, so it is safe to create a shared reference. > > + unsafe { &self.inner.as_ref().data } > > + } > > +}
Powered by blists - more mailing lists