rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Arc methods for linked list
@ 2024-02-19 14:54 Alice Ryhl
  2024-02-19 14:54 ` [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw` Alice Ryhl
  2024-02-19 14:54 ` [PATCH 2/2] rust: sync: add `Arc::into_unique_or_drop` Alice Ryhl
  0 siblings, 2 replies; 5+ messages in thread
From: Alice Ryhl @ 2024-02-19 14:54 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg
  Cc: rust-for-linux, linux-kernel, Alice Ryhl

This patchset contains two useful methods for the Arc type. They will be
used in my Rust linked list implementation, which Rust Binder uses. See
the Rust Binder RFC [1] for more information. Both these commits and
the linked list that uses them are present in the branch referenced by
the RFC.

I will send the linked list to the mailing list soon.

This patchset is based on rust-next and depends on [2].

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-0-08ba9197f637@google.com/ [1]
Link: https://lore.kernel.org/all/20240215104601.1267763-1-aliceryhl@google.com/ [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (2):
      rust: sync: add `ArcBorrow::from_raw`
      rust: sync: add `Arc::into_unique_or_drop`

 rust/kernel/sync/arc.rs | 103 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 85 insertions(+), 18 deletions(-)
---
base-commit: e3c3d34507c7a146de1c5ce01bd0b2c0018b2609
change-id: 20240209-arc-for-list-a2c126c2ad5c

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw`
  2024-02-19 14:54 [PATCH 0/2] Arc methods for linked list Alice Ryhl
@ 2024-02-19 14:54 ` Alice Ryhl
  2024-02-20  5:36   ` Boqun Feng
  2024-02-19 14:54 ` [PATCH 2/2] rust: sync: add `Arc::into_unique_or_drop` Alice Ryhl
  1 sibling, 1 reply; 5+ messages in thread
From: Alice Ryhl @ 2024-02-19 14:54 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg
  Cc: rust-for-linux, linux-kernel, Alice Ryhl

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 | 72 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 54 insertions(+), 18 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 7d4c4bf58388..a5314df409e7 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -232,27 +232,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 pointer returned by `into_raw` points at the `data` field of an
+        // `ArcInner<T>`, as promised by the caller.
+        let ptr = unsafe { raw_to_inner_ptr(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`].
@@ -273,6 +259,35 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
     }
 }
 
+/// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].
+///
+/// # Safety
+///
+/// The provided pointer must point the `data` field of an `ArcInner<T>` value.
+unsafe fn raw_to_inner_ptr<T: ?Sized>(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` and 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()) }
+}
+
 impl<T: 'static> ForeignOwnable for Arc<T> {
     type Borrowed<'a> = ArcBorrow<'a, T>;
 
@@ -453,6 +468,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.
+    pub unsafe fn from_raw(ptr: *const T) -> Self {
+        // SAFETY: The pointer returned by `into_raw` points at the `data` field of an
+        // `ArcInner<T>`.
+        let ptr = unsafe { raw_to_inner_ptr(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.rc0.258.g7320e95886-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] rust: sync: add `Arc::into_unique_or_drop`
  2024-02-19 14:54 [PATCH 0/2] Arc methods for linked list Alice Ryhl
  2024-02-19 14:54 ` [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw` Alice Ryhl
@ 2024-02-19 14:54 ` Alice Ryhl
  1 sibling, 0 replies; 5+ messages in thread
From: Alice Ryhl @ 2024-02-19 14:54 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg
  Cc: rust-for-linux, linux-kernel, Alice Ryhl

Decrement the refcount of an `Arc`, but handle the case where it hits
zero by taking ownership of the now-unique `Arc`, instead of destroying
and deallocating it.

This is a dependency of the linked list that Rust Binder uses. The
linked list uses this method as part of its `ListArc` abstraction.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/sync/arc.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index a5314df409e7..a7e7f7ccace4 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -257,6 +257,37 @@ pub fn as_arc_borrow(&self) -> ArcBorrow<'_, T> {
     pub fn ptr_eq(this: &Self, other: &Self) -> bool {
         core::ptr::eq(this.ptr.as_ptr(), other.ptr.as_ptr())
     }
+
+    /// Converts this [`Arc`] into a [`UniqueArc`], or destroys it if it is not unique.
+    ///
+    /// When this destroys the `Arc`, it does so while properly avoiding races. This means that
+    /// this method will never call the destructor of the value.
+    pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
+        // We will manually manage the refcount in this method, so we disable the destructor.
+        let me = ManuallyDrop::new(self);
+        // SAFETY: We own a refcount, so the pointer is still valid.
+        let refcount = unsafe { me.ptr.as_ref() }.refcount.get();
+
+        // SAFETY: If the refcount reaches a non-zero value, then we have destroyed this `Arc` and
+        // will return without further touching the `Arc`. If the refcount reaches zero, then there
+        // are no other arcs, and we can create a `UniqueArc`.
+        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
+        if is_zero {
+            // SAFETY: We have exclusive access to the arc, so we can perform unsynchronized
+            // accesses to the refcount.
+            unsafe { core::ptr::write(refcount, bindings::REFCOUNT_INIT(1)) };
+
+            // SAFETY: We own one refcount, so we can create a `UniqueArc`. It needs to be pinned,
+            // since an `Arc` is pinned.
+            unsafe {
+                Some(Pin::new_unchecked(UniqueArc {
+                    inner: Arc::from_inner(me.ptr),
+                }))
+            }
+        } else {
+            None
+        }
+    }
 }
 
 /// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].

-- 
2.44.0.rc0.258.g7320e95886-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw`
  2024-02-19 14:54 ` [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw` Alice Ryhl
@ 2024-02-20  5:36   ` Boqun Feng
  2024-02-20  9:25     ` Alice Ryhl
  0 siblings, 1 reply; 5+ messages in thread
From: Boqun Feng @ 2024-02-20  5:36 UTC (permalink / raw)
  To: Alice Ryhl
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	rust-for-linux, linux-kernel

On Mon, Feb 19, 2024 at 02:54:11PM +0000, 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 | 72 ++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> index 7d4c4bf58388..a5314df409e7 100644
> --- a/rust/kernel/sync/arc.rs
> +++ b/rust/kernel/sync/arc.rs
> @@ -232,27 +232,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 pointer returned by `into_raw` points at the `data` field of an
> +        // `ArcInner<T>`, as promised by the caller.
> +        let ptr = unsafe { raw_to_inner_ptr(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`].
> @@ -273,6 +259,35 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
>      }
>  }
>  
> +/// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].
> +///
> +/// # Safety
> +///
> +/// The provided pointer must point the `data` field of an `ArcInner<T>` value.
> +unsafe fn raw_to_inner_ptr<T: ?Sized>(ptr: *const T) -> NonNull<ArcInner<T>> {

Nit: put this into an `impl<T:?Sized> ArcInner<T>` block maybe?

> +    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.

"since it originate from a previous call to `Arc::into_raw`" is the
safety requirement of `Arc::from_raw`, since the safety requirement of
`raw_to_inner_ptr` is different, so I think we should say "since the
function safety requirement guarantees `ptr` points to `data` field,
which is exactly `val_offset` away from the beginning of `ArcInner<T>`".
Thoughts?

BTW, in fat pointer cases, by "must point the `data` field of an
`ArcInner<T>` value", it means both the address and the metadata should
be the same as the original object in `ArcInner<T>`, right? In other
words, the following code should not be safe, i.e. the
raw_to_inner_ptr() safety requirement is not satisfied.

	let x: Arc<[u8]> // assume x.len() == 4

	let y = &(x[0..1]) as *const [u8] // y has the same address of
					  // the `data` field of `x`.
	
	let inner = unsafe { raw_to_inner_ptr(y) };
	// ^^^ the safety requirement is not satisfied???

This may not be important since the users of `raw_to_inner_ptr` all have
stronger safey guarantees ("`ptr` must come from `Arc::into_raw()`"),
and `raw_to_inner_ptr` is not a pub function, but I just wonder whether
we need to improve the current safety requirements, or "point" means
both address and metadata for fat pointers?

Regards,
Boqun

> +    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()) }
> +}
> +
>  impl<T: 'static> ForeignOwnable for Arc<T> {
>      type Borrowed<'a> = ArcBorrow<'a, T>;
>  
> @@ -453,6 +468,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.
> +    pub unsafe fn from_raw(ptr: *const T) -> Self {
> +        // SAFETY: The pointer returned by `into_raw` points at the `data` field of an
> +        // `ArcInner<T>`.
> +        let ptr = unsafe { raw_to_inner_ptr(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.rc0.258.g7320e95886-goog
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw`
  2024-02-20  5:36   ` Boqun Feng
@ 2024-02-20  9:25     ` Alice Ryhl
  0 siblings, 0 replies; 5+ messages in thread
From: Alice Ryhl @ 2024-02-20  9:25 UTC (permalink / raw)
  To: Boqun Feng
  Cc: Miguel Ojeda, Alex Gaynor, Wedson Almeida Filho, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	rust-for-linux, linux-kernel

On Tue, Feb 20, 2024 at 6:36 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Feb 19, 2024 at 02:54:11PM +0000, 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 | 72 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 54 insertions(+), 18 deletions(-)
> >
> > diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
> > index 7d4c4bf58388..a5314df409e7 100644
> > --- a/rust/kernel/sync/arc.rs
> > +++ b/rust/kernel/sync/arc.rs
> > @@ -232,27 +232,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 pointer returned by `into_raw` points at the `data` field of an
> > +        // `ArcInner<T>`, as promised by the caller.
> > +        let ptr = unsafe { raw_to_inner_ptr(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`].
> > @@ -273,6 +259,35 @@ pub fn ptr_eq(this: &Self, other: &Self) -> bool {
> >      }
> >  }
> >
> > +/// Converts a pointer to the contents of an [`Arc`] into a pointer to the [`ArcInner`].
> > +///
> > +/// # Safety
> > +///
> > +/// The provided pointer must point the `data` field of an `ArcInner<T>` value.
> > +unsafe fn raw_to_inner_ptr<T: ?Sized>(ptr: *const T) -> NonNull<ArcInner<T>> {
>
> Nit: put this into an `impl<T:?Sized> ArcInner<T>` block maybe?
>
> > +    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.
>
> "since it originate from a previous call to `Arc::into_raw`" is the
> safety requirement of `Arc::from_raw`, since the safety requirement of
> `raw_to_inner_ptr` is different, so I think we should say "since the
> function safety requirement guarantees `ptr` points to `data` field,
> which is exactly `val_offset` away from the beginning of `ArcInner<T>`".
> Thoughts?
>
> BTW, in fat pointer cases, by "must point the `data` field of an
> `ArcInner<T>` value", it means both the address and the metadata should
> be the same as the original object in `ArcInner<T>`, right? In other
> words, the following code should not be safe, i.e. the
> raw_to_inner_ptr() safety requirement is not satisfied.
>
>         let x: Arc<[u8]> // assume x.len() == 4
>
>         let y = &(x[0..1]) as *const [u8] // y has the same address of
>                                           // the `data` field of `x`.
>
>         let inner = unsafe { raw_to_inner_ptr(y) };
>         // ^^^ the safety requirement is not satisfied???
>
> This may not be important since the users of `raw_to_inner_ptr` all have
> stronger safey guarantees ("`ptr` must come from `Arc::into_raw()`"),
> and `raw_to_inner_ptr` is not a pub function, but I just wonder whether
> we need to improve the current safety requirements, or "point" means
> both address and metadata for fat pointers?

I mean, really, the same problem arises for Sized pointers where the
pointer points at the first field of a struct.

We may have to take inspiration from the std Arc::from_raw. It says
that the pointee must have the same size and alignment as what was
used when you called Arc::into_raw, and then it further says that if
the pointer type is not exactly the same, then you are effectively
performing a transmute.

Alice

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-02-20  9:25 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-19 14:54 [PATCH 0/2] Arc methods for linked list Alice Ryhl
2024-02-19 14:54 ` [PATCH 1/2] rust: sync: add `ArcBorrow::from_raw` Alice Ryhl
2024-02-20  5:36   ` Boqun Feng
2024-02-20  9:25     ` Alice Ryhl
2024-02-19 14:54 ` [PATCH 2/2] rust: sync: add `Arc::into_unique_or_drop` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).