linux-kernel - Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <874j6cjiip.fsf@kernel.org>
Date: Thu, 19 Sep 2024 07:54:06 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
Cc: "Miguel Ojeda" <ojeda@...nel.org>,  "Alex Gaynor"
 <alex.gaynor@...il.com>,  "Anna-Maria Behnsen" <anna-maria@...utronix.de>,
  "Frederic Weisbecker" <frederic@...nel.org>,  "Thomas Gleixner"
 <tglx@...utronix.de>,  "Boqun Feng" <boqun.feng@...il.com>,  "Gary Guo"
 <gary@...yguo.net>,  Björn Roy Baron
 <bjorn3_gh@...tonmail.com>,  "Alice
 Ryhl" <aliceryhl@...gle.com>,  <rust-for-linux@...r.kernel.org>,
  <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 04/14] rust: sync: add `Arc::clone_from_raw`

"Benno Lossin" <benno.lossin@...ton.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@...nel.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