Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On 18.09.24 00:27, Andreas Hindborg wrote:
>> Add a method to clone an arc from a pointer to the data managed by the
>> `Arc`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/sync/arc.rs | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
>> index a57ea3e2b44c..2c95712d12a2 100644
>> --- a/rust/kernel/sync/arc.rs
>> +++ b/rust/kernel/sync/arc.rs
>> @@ -282,6 +282,26 @@ pub unsafe fn from_raw(ptr: *const T) -> Self {
>>          unsafe { Self::from_inner(ptr) }
>>      }
>>
>> +    /// Clones an [`Arc`] instance from a pointer to the contained data.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must point to an allocation that is contained within a live [`Arc<T>`].
>> +    pub unsafe fn clone_from_raw(ptr: *const T) -> Self {
>> +        // SAFETY: The caller promises that this pointer points to data
>> +        // contained in an `Arc` that is still valid.
>> +        let inner = unsafe { ArcInner::container_of(ptr).as_ref() };
>> +
>> +        // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot
>> +        // overflow to zero. SAFETY: By the function safety requirement, there
>> +        // is necessarily a reference to the object, so it is safe to increment
>> +        // the refcount.
>> +        unsafe { bindings::refcount_inc(inner.refcount.get()) };
>> +
>> +        // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>> +        unsafe { Self::from_inner(inner.into()) }
>
> The implementation of this function looks a bit strange to me, how about
> this?:
>
>     // SAFETY: this function has the same safety requirements as `from_raw`.
>     let arc = unsafe { Self::from_raw(ptr) };
>     let clone = arc.clone();
>     // Prevent decrementing the refcount.
>     mem::forget(arc);
>     clone
>

We do not own
a refcount on the Arc. For a short duration you will have a wrong
refcount. If you have two Arcs and the refcount is 1, the ArcInner might
be dropped after the first line of this suggestion, before you do clone,
and then this is not sound.

Best regards,
Andreas