Received-SPF: pass (google.com: domain of linux-kernel+bounces-97891-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sat, 09 Mar 2024 12:55:35 +0000
To: Alice Ryhl <aliceryhl@google.com>, Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, Andreas Hindborg <a.hindborg@samsung.com>
From: Benno Lossin <benno.lossin@proton.me>
Cc: rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] rust: sync: add `ArcBorrow::from_raw`
Message-ID: <e09ed8fc-d305-4740-8c6e-7308a634b822@proton.me>
In-Reply-To: <20240228-arc-for-list-v2-1-ae93201426b4@google.com>
References: <20240228-arc-for-list-v2-0-ae93201426b4@google.com> <20240228-arc-for-list-v2-1-ae93201426b4@google.com>
Feedback-ID: 71780778:user:proton
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

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.
>=20
> This is a dependency of the linked list that Rust Binder uses. The
> linked list uses this method when iterating over the linked list.
>=20
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>   rust/kernel/sync/arc.rs | 76 +++++++++++++++++++++++++++++++++++++-----=
-------
>   1 file changed, 58 insertions(+), 18 deletions(-)
>=20
> 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,
>   }
>=20
> +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 =3D Layout::new::<bindings::refcount_t>();
> +        // SAFETY: The caller guarantees that the pointer is valid.
> +        let val_layout =3D Layout::for_value(unsafe { &*ptr });
> +        // SAFETY: We're computing the layout of a real struct that exis=
ted when compiling this
> +        // binary, so its layout is not so large that it can trigger ari=
thmetic overflow.
> +        let val_offset =3D unsafe { refcount_layout.extend(val_layout).u=
nwrap_unchecked().1 };
> +
> +        // Pointer casts leave the metadata unchanged. This is okay beca=
use 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 =3D 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::in=
to_raw` on an `Arc` that is
> +        // still valid.
> +        let ptr =3D unsafe { ptr.byte_sub(val_offset) };
> +
> +        // SAFETY: The pointer can't be null since you can't have an `Ar=
cInner<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 `s=
elf`.
>   impl<T: ?Sized> core::ops::Receiver for Arc<T> {}
>=20
> @@ -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 [`A=
rc::into_raw`].
>       pub unsafe fn from_raw(ptr: *const T) -> Self {
> -        let refcount_layout =3D Layout::new::<bindings::refcount_t>();
> -        // SAFETY: The caller guarantees that the pointer is valid.
> -        let val_layout =3D Layout::for_value(unsafe { &*ptr });
> -        // SAFETY: We're computing the layout of a real struct that exis=
ted when compiling this
> -        // binary, so its layout is not so large that it can trigger ari=
thmetic overflow.
> -        let val_offset =3D unsafe { refcount_layout.extend(val_layout).u=
nwrap_unchecked().1 };
> -
> -        // Pointer casts leave the metadata unchanged. This is okay beca=
use 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 =3D 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::in=
to_raw` and is still valid.
> -        let ptr =3D 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 =3D unsafe { ArcInner::container_of(ptr) };
>=20
>           // SAFETY: By the safety requirements we know that `ptr` came f=
rom `Arc::into_raw`, so the
>           // reference count held then will be owned by the new `Arc` obj=
ect.
> -        unsafe { Self::from_inner(NonNull::new_unchecked(ptr.cast_mut())=
) }
> +        unsafe { Self::from_inner(ptr) }
>       }
>=20
>       /// 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.

--=20
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 =3D unsafe { ArcInner::container_of(ptr) };
> +
> +        // SAFETY: The caller promises that the value remains valid sinc=
e the reference count must
> +        // not hit zero, and no mutable reference will be created since =
that would involve a
> +        // `UniqueArc`.
> +        unsafe { Self::new(ptr) }
> +    }
>   }
>=20
>   impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> {
>=20
> --
> 2.44.0.rc1.240.g4c46232300-goog
>=20