Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sat, 09 Mar 2024 12:55:35 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw` |
| |
On 2/28/24 14:00, Alice Ryhl wrote: > Allows access to a value in an `Arc` that is currently held as a raw > pointer due to use of `Arc::into_raw`, without destroying or otherwise > consuming that raw pointer. > > This is a dependency of the linked list that Rust Binder uses. The > linked list uses this method when iterating over the linked list. > > Signed-off-by: Alice Ryhl <aliceryhl@google.com> > --- > rust/kernel/sync/arc.rs | 76 +++++++++++++++++++++++++++++++++++++------------ > 1 file changed, 58 insertions(+), 18 deletions(-) > > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs > index 7d4c4bf58388..53addb8876c2 100644 > --- a/rust/kernel/sync/arc.rs > +++ b/rust/kernel/sync/arc.rs > @@ -137,6 +137,39 @@ struct ArcInner<T: ?Sized> { > data: T, > } > > +impl<T: ?Sized> ArcInner<T> { > + /// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`]. > + /// > + /// # Safety > + /// > + /// `ptr` must have been returned by a previous call to [`Arc::into_raw`], and the `Arc` must > + /// not yet have been destroyed. > + unsafe fn container_of(ptr: *const T) -> NonNull<ArcInner<T>> { > + let refcount_layout = Layout::new::<bindings::refcount_t>(); > + // SAFETY: The caller guarantees that the pointer is valid. > + let val_layout = Layout::for_value(unsafe { &*ptr }); > + // SAFETY: We're computing the layout of a real struct that existed when compiling this > + // binary, so its layout is not so large that it can trigger arithmetic overflow. > + let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 }; > + > + // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and > + // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field. > + // > + // This is documented at: > + // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>. > + let ptr = ptr as *const ArcInner<T>; > + > + // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the > + // pointer, since it originates from a previous call to `Arc::into_raw` on an `Arc` that is > + // still valid. > + let ptr = unsafe { ptr.byte_sub(val_offset) }; > + > + // SAFETY: The pointer can't be null since you can't have an `ArcInner<T>` value at the null > + // address. > + unsafe { NonNull::new_unchecked(ptr.cast_mut()) } > + } > +} > + > // This is to allow [`Arc`] (and variants) to be used as the type of `self`. > impl<T: ?Sized> core::ops::Receiver for Arc<T> {} > > @@ -232,27 +265,13 @@ pub fn into_raw(self) -> *const T { > /// `ptr` must have been returned by a previous call to [`Arc::into_raw`]. Additionally, it > /// must not be called more than once for each previous call to [`Arc::into_raw`]. > pub unsafe fn from_raw(ptr: *const T) -> Self { > - let refcount_layout = Layout::new::<bindings::refcount_t>(); > - // SAFETY: The caller guarantees that the pointer is valid. > - let val_layout = Layout::for_value(unsafe { &*ptr }); > - // SAFETY: We're computing the layout of a real struct that existed when compiling this > - // binary, so its layout is not so large that it can trigger arithmetic overflow. > - let val_offset = unsafe { refcount_layout.extend(val_layout).unwrap_unchecked().1 }; > - > - // Pointer casts leave the metadata unchanged. This is okay because the metadata of `T` and > - // `ArcInner<T>` is the same since `ArcInner` is a struct with `T` as its last field. > - // > - // This is documented at: > - // <https://doc.rust-lang.org/std/ptr/trait.Pointee.html>. > - let ptr = ptr as *const ArcInner<T>; > - > - // SAFETY: The pointer is in-bounds of an allocation both before and after offsetting the > - // pointer, since it originates from a previous call to `Arc::into_raw` and is still valid. > - let ptr = unsafe { ptr.byte_sub(val_offset) }; > + // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an > + // `Arc` that is still valid. > + let ptr = unsafe { ArcInner::container_of(ptr) }; > > // SAFETY: By the safety requirements we know that `ptr` came from `Arc::into_raw`, so the > // reference count held then will be owned by the new `Arc` object. > - unsafe { Self::from_inner(NonNull::new_unchecked(ptr.cast_mut())) } > + unsafe { Self::from_inner(ptr) } > } > > /// Returns an [`ArcBorrow`] from the given [`Arc`]. > @@ -453,6 +472,27 @@ unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self { > _p: PhantomData, > } > } > + > + /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with > + /// [`Arc::into_raw`]. > + /// > + /// # Safety > + /// > + /// * The provided pointer must originate from a call to [`Arc::into_raw`]. > + /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must > + /// not hit zero. > + /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a > + /// [`UniqueArc`] reference to this value.
I am a bit confused, this feels to me like it should be guaranteed by `UniqueArc` and not by this function. Currently there is not even a way of getting a `*const T` from a `UniqueArc`. So I think we can remove this requirement and instead have the requirement for creating `UniqueArc` that not only the refcount is exactly 1, but also that no `ArcBorrow` exists.
-- Cheers, Benno
> + pub unsafe fn from_raw(ptr: *const T) -> Self { > + // SAFETY: The caller promises that this pointer originates from a call to `into_raw` on an > + // `Arc` that is still valid. > + let ptr = unsafe { ArcInner::container_of(ptr) }; > + > + // SAFETY: The caller promises that the value remains valid since the reference count must > + // not hit zero, and no mutable reference will be created since that would involve a > + // `UniqueArc`. > + unsafe { Self::new(ptr) } > + } > } > > impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> { > > -- > 2.44.0.rc1.240.g4c46232300-goog >
| ![\](/images/icornerr.gif) |