Received-SPF: pass (google.com: domain of linux-kernel+bounces-130136-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 03 Apr 2024 15:51:42 +0000
To: Alice Ryhl <aliceryhl@google.com>, Miguel Ojeda <ojeda@kernel.org>, Andrew Morton <akpm@linux-foundation.org>
From: Benno Lossin <benno.lossin@proton.me>
Cc: 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>, Marco Elver <elver@google.com>, Kees Cook <keescook@chromium.org>, Coly Li <colyli@suse.de>, Paolo Abeni <pabeni@redhat.com>, Pierre Gondois <pierre.gondois@arm.com>, Ingo Molnar <mingo@kernel.org>, Jakub Kicinski <kuba@kernel.org>, Wei Yang <richard.weiyang@gmail.com>, Matthew Wilcox <willy@infradead.org>, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org
Subject: Re: [PATCH 1/9] rust: list: add ListArc
Message-ID: <2f25f21e-fad8-48bb-aa2b-d61bf8909a41@proton.me>
In-Reply-To: <20240402-linked-list-v1-1-b1c59ba7ae3b@google.com>
References: <20240402-linked-list-v1-0-b1c59ba7ae3b@google.com> <20240402-linked-list-v1-1-b1c59ba7ae3b@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 02.04.24 14:16, Alice Ryhl wrote:
> +impl<T: ListArcSafe<ID>, const ID: u64> ListArc<T, ID> {
> +    /// Constructs a new reference counted instance of `T`.
> +    pub fn try_new(contents: T) -> Result<Self, AllocError> {
> +        Ok(Self::from_unique(UniqueArc::try_new(contents)?))
> +    }
> +
> +    /// Use the given initializer to in-place initialize a `T`.
> +    ///
> +    /// If `T: !Unpin` it will not be able to move afterwards.
> +    pub fn pin_init<E>(init: impl PinInit<T, E>) -> error::Result<Self>
> +    where
> +        Error: From<E>,
> +    {
> +        Ok(Self::from_pin_unique(UniqueArc::pin_init(init)?))
> +    }

pin-init has a general trait for this: InPlaceInit. I don't know if the
other functions that it provides would help you.

> +}
> +
> +impl<T, const ID: u64> ListArc<T, ID>
> +where
> +    T: ListArcSafe<ID> + ?Sized,
> +{
> +    /// Convert a [`UniqueArc`] into a [`ListArc`].
> +    pub fn from_unique(mut unique: UniqueArc<T>) -> Self {
> +        // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.
> +        unsafe { T::on_create_list_arc_from_unique(&mut unique) };
> +        let arc =3D Arc::from(unique);
> +        // SAFETY: We just called `on_create_list_arc_from_unique` on an=
 arc without a `ListArc`,
> +        // so we can create a `ListArc`.
> +        unsafe { Self::transmute_from_arc(arc) }
> +    }
> +
> +    /// Convert a pinned [`UniqueArc`] into a [`ListArc`].
> +    pub fn from_pin_unique(unique: Pin<UniqueArc<T>>) -> Self {
> +        // SAFETY: We continue to treat this pointer as pinned after thi=
s call, since `ListArc`
> +        // implicitly pins its value.

This is not sufficient, since you also rely on `Self::from_unique` to
handle the parameter as if it were pinned, which it does not. Since it
calls `T::on_create_list_arc_from_unique` which just gets a `&mut self`
as a parameter and it could move stuff out.

> +        Self::from_unique(unsafe { Pin::into_inner_unchecked(unique) })
> +    }
> +
> +    /// Like [`from_unique`], but creates two `ListArcs`.
> +    ///
> +    /// The two ids must be different.
> +    ///
> +    /// [`from_unique`]: ListArc::from_unique
> +    pub fn pair_from_unique<const ID2: u64>(mut unique: UniqueArc<T>) ->=
 (Self, ListArc<T, ID2>)
> +    where
> +        T: ListArcSafe<ID2>,
> +    {
> +        assert_ne!(ID, ID2);

Can this be a `build_assert!`?

> +
> +        // SAFETY: We have a `UniqueArc`, so we can call this method.

I liked the comment from above better:

     // SAFETY: We have a `UniqueArc`, so there is no `ListArc`.

Maybe use the variation "so there are no `ListArc`s for any ID.".

> +        unsafe { <T as ListArcSafe<ID>>::on_create_list_arc_from_unique(=
&mut unique) };
> +        // SAFETY: We have a `UniqueArc`, so we can call this method. Th=
e two ids are not equal.
> +        unsafe { <T as ListArcSafe<ID2>>::on_create_list_arc_from_unique=
(&mut unique) };
> +
> +        let arc1 =3D Arc::from(unique);
> +        let arc2 =3D Arc::clone(&arc1);
> +
> +        // SAFETY: We just called `on_create_list_arc_from_unique` on an=
 arc without a `ListArc`,
> +        // so we can create a `ListArc`.

I would mention the two different IDs again.

> +        unsafe {
> +            (
> +                Self::transmute_from_arc(arc1),
> +                ListArc::transmute_from_arc(arc2),
> +            )
> +        }
> +    }
> +
> +    /// Like [`pair_from_unique`], but uses a pinned arc.
> +    ///
> +    /// The two ids must be different.
> +    ///
> +    /// [`pair_from_unique`]: ListArc::pair_from_unique
> +    pub fn pair_from_pin_unique<const ID2: u64>(
> +        unique: Pin<UniqueArc<T>>,
> +    ) -> (Self, ListArc<T, ID2>)
> +    where
> +        T: ListArcSafe<ID2>,
> +    {
> +        // SAFETY: We continue to treat this pointer as pinned after thi=
s call, since `ListArc`
> +        // implicitly pins its value.
> +        Self::pair_from_unique(unsafe { Pin::into_inner_unchecked(unique=
) })
> +    }
> +
> +    /// Transmutes an [`Arc`] into a `ListArc` without updating the trac=
king inside `T`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * The value must not already have a `ListArc` reference.
> +    /// * The tracking inside `T` must think that there is a `ListArc` r=
eference.
> +    #[inline]
> +    unsafe fn transmute_from_arc(me: Arc<T>) -> Self {
> +        // INVARIANT: By the safety requirements, the invariants on `Lis=
tArc` are satisfied.
> +        // SAFETY: ListArc is repr(transparent).
> +        unsafe { core::mem::transmute(me) }

Why do you need a transmute here? Can't you just construct the struct?

     Self { arc: me }

--=20
Cheers,
Benno

> +    }
> +