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()) }
+    }
+
     /// Returns an [`ArcBorrow`] from the given [`Arc`].
     ///
     /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
-- 
2.46.0

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

(of course you would need to change the safety requirements of
`clone_from_raw` to point to `from_raw`)

---
Cheers,
Benno

> +    }
> +
>      /// Returns an [`ArcBorrow`] from the given [`Arc`].
>      ///
>      /// This is useful when the argument of a function call is an [`ArcBorrow`] (e.g., in a method
> --
> 2.46.0
> 
> 

"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

Andreas Hindborg <a.hindborg@kernel.org> writes:

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

Well, disregard that. This is why one should not reply to emails before
coffee in the morning.

Of course, a precondition for calling this function is that the arc
containing the data pointed to by `ptr` is live for the duration. So
what you wrote would work. But I still do not like having two `Arc`s in
existence with the wrong refcount. 

BR Andreas

On Thu, Sep 19, 2024 at 8:19 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
> > "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.
>
> Well, disregard that. This is why one should not reply to emails before
> coffee in the morning.
>
> Of course, a precondition for calling this function is that the arc
> containing the data pointed to by `ptr` is live for the duration. So
> what you wrote would work. But I still do not like having two `Arc`s in
> existence with the wrong refcount.

Doing it this way has been pretty standard with std for a long time,
until the Arc::increment_strong_count and similar methods were added.
I think it's fine, though I would have used ManuallyDrop instead of
mem::forget.

Of course, in this particular case, using `ArcBorrow` is even better.

Alice

On Wed, 18 Sep 2024 18:19:20 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

> 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
> 
> (of course you would need to change the safety requirements of
> `clone_from_raw` to point to `from_raw`)

Wouldn't this function simply be

	// SAFETY: ...
	let borrow = unsafe { ArcBorrow::from_raw(ptr) }
  	borrow.into()

?

Maybe this function doesn't even need to exist...

Best,
Gary

"Gary Guo" <gary@garyguo.net> writes:

> On Wed, 18 Sep 2024 18:19:20 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
>
>> On 18.09.24 00:27, Andreas Hindborg wrote:
[...]
>> > +    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
>>
>> (of course you would need to change the safety requirements of
>> `clone_from_raw` to point to `from_raw`)
>
> Wouldn't this function simply be
>
> 	// SAFETY: ...
> 	let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>   	borrow.into()
>
> ?
>
> Maybe this function doesn't even need to exist...

Maybe that could work. But my use case does not satisfy the safety
requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
called. Perhaps we can update the requirements for that function?

Best regards,
Andreas

On 19.09.24 08:00, Andreas Hindborg wrote:
> "Gary Guo" <gary@garyguo.net> writes:
> 
>> On Wed, 18 Sep 2024 18:19:20 +0000
>> Benno Lossin <benno.lossin@proton.me> wrote:
>>
>>> On 18.09.24 00:27, Andreas Hindborg wrote:
> [...]
>>>> +    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
>>>
>>> (of course you would need to change the safety requirements of
>>> `clone_from_raw` to point to `from_raw`)
>>
>> Wouldn't this function simply be
>>
>> 	// SAFETY: ...
>> 	let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>>   	borrow.into()
>>
>> ?
>>
>> Maybe this function doesn't even need to exist...
> 
> Maybe that could work. But my use case does not satisfy the safety
> requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
> called. Perhaps we can update the requirements for that function?

If I understood the code correctly, you are essentially doing this:

    let arc = Arc::<T>::new(..);
    let ptr = Arc::as_ptr(&arc);
    let ptr = T::raw_get_timer(ptr);
    let ptr = Timer::raw_get(ptr);

    // ptr is now used by the timer subsystem to fire the timer

    let ptr = ptr.cast::<Timer>();
    let ptr = T::timer_container_of(ptr);
    let borrow = ArcBorrow::from_raw(ptr);
    let arc = borrow.into();

The only thing that we would have to change would be adding
`Arc::as_ptr` as a source in the `ArcBorrow::from_raw` safety
requirements.

---
Cheers,
Benno

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

> On 19.09.24 08:00, Andreas Hindborg wrote:
>> "Gary Guo" <gary@garyguo.net> writes:
>>
>>> On Wed, 18 Sep 2024 18:19:20 +0000
>>> Benno Lossin <benno.lossin@proton.me> wrote:
>>>
>>>> On 18.09.24 00:27, Andreas Hindborg wrote:
>> [...]
>>>>> +    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
>>>>
>>>> (of course you would need to change the safety requirements of
>>>> `clone_from_raw` to point to `from_raw`)
>>>
>>> Wouldn't this function simply be
>>>
>>> 	// SAFETY: ...
>>> 	let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>>>   	borrow.into()
>>>
>>> ?
>>>
>>> Maybe this function doesn't even need to exist...
>>
>> Maybe that could work. But my use case does not satisfy the safety
>> requirements on `ArcBorrow::from_raw`. The`Arc::into_raw` was not
>> called. Perhaps we can update the requirements for that function?
>
> If I understood the code correctly, you are essentially doing this:
>
>     let arc = Arc::<T>::new(..);
>     let ptr = Arc::as_ptr(&arc);
>     let ptr = T::raw_get_timer(ptr);
>     let ptr = Timer::raw_get(ptr);
>
>     // ptr is now used by the timer subsystem to fire the timer
>
>     let ptr = ptr.cast::<Timer>();
>     let ptr = T::timer_container_of(ptr);
>     let borrow = ArcBorrow::from_raw(ptr);
>     let arc = borrow.into();
>
> The only thing that we would have to change would be adding
> `Arc::as_ptr` as a source in the `ArcBorrow::from_raw` safety
> requirements.

Yes, I think so. I agree that `ArcBorrow` is the right way forward here.
Patch 11 of this series extends `TimerCallback` to be able to have a
different type for the implementer of `RawCallback` and the type passed
to `run`. This is to be able to specify `Pin<Box<_>` as the pointer but
`& _` as the parameter. Those names should probably be changed to
`CallbackTarget` and `CallbackTargetBorrow`, but they could cover this
use of `ArcBorrow` as well.

This use is very similar to `ForeignOwnable`, but a slightly different
use case, since I want to implement for pinned references too. Is there
a way of aligning this and sharing some trait here?

Best regards,
Andreas

On 18.09.24 22:12, Gary Guo wrote:
> On Wed, 18 Sep 2024 18:19:20 +0000
> Benno Lossin <benno.lossin@proton.me> wrote:
> 
>> 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
>>
>> (of course you would need to change the safety requirements of
>> `clone_from_raw` to point to `from_raw`)
> 
> Wouldn't this function simply be
> 
> 	// SAFETY: ...
> 	let borrow = unsafe { ArcBorrow::from_raw(ptr) }
>   	borrow.into()
> 
> ?

Oh yeah we do have ArcBorrow...

> Maybe this function doesn't even need to exist...

Depends on how often the above has to be written, but I think yeah, it
might not be necessary.

---
Cheers,
Benno