linux-kernel - Re: [PATCH v4 3/5] rust: convert `Arc` to use `Refcount`

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] [day] [month] [year] [list]

Message-Id: <DATDAOYC60S9.2MEZ4NHCUHNXO@kernel.org>
Date: Sun, 22 Jun 2025 23:08:05 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Gary Guo" <gary@...nel.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex
 Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary
 Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Alice Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>,
 "Danilo Krummrich" <dakr@...nel.org>, "Will Deacon" <will@...nel.org>,
 "Peter Zijlstra" <peterz@...radead.org>, "Mark Rutland"
 <mark.rutland@....com>, "Tamir Duberstein" <tamird@...il.com>, "Xiangfei
 Ding" <dingxiangfei2009@...il.com>, "Alex Mantel"
 <alexmantel93@...lbox.org>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] rust: convert `Arc` to use `Refcount`

On Sun Jun 22, 2025 at 2:57 PM CEST, Gary Guo wrote:
> @@ -428,14 +422,10 @@ fn as_ref(&self) -> &T {
>  
>  impl<T: ?Sized> Clone for Arc<T> {
>      fn clone(&self) -> Self {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
> -        // safe to dereference it.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> -
> -        // INVARIANT: C `refcount_inc` saturates the refcount, so it cannot overflow to zero.
> +        // INVARIANT: `Refcount` saturates the refcount, so it cannot overflow to zero.
>          // SAFETY: By the type invariant, there is necessarily a reference to the object, so it is
>          // safe to increment the refcount.
> -        unsafe { bindings::refcount_inc(refcount) };
> +        unsafe { self.ptr.as_ref().refcount.inc() };

The `.refcount.inc()` can be outside of the `unsafe` block.

>  
>          // SAFETY: We just incremented the refcount. This increment is now owned by the new `Arc`.
>          unsafe { Self::from_inner(self.ptr) }
> @@ -444,16 +434,10 @@ fn clone(&self) -> Self {
>  
>  impl<T: ?Sized> Drop for Arc<T> {
>      fn drop(&mut self) {
> -        // SAFETY: By the type invariant, there is necessarily a reference to the object. We cannot
> -        // touch `refcount` after it's decremented to a non-zero value because another thread/CPU
> -        // may concurrently decrement it to zero and free it. It is ok to have a raw pointer to
> -        // freed/invalid memory as long as it is never dereferenced.
> -        let refcount = unsafe { self.ptr.as_ref() }.refcount.get();
> -
>          // INVARIANT: If the refcount reaches zero, there are no other instances of `Arc`, and
>          // this instance is being dropped, so the broken invariant is not observable.
> -        // SAFETY: Also by the type invariant, we are allowed to decrement the refcount.
> -        let is_zero = unsafe { bindings::refcount_dec_and_test(refcount) };
> +        // SAFETY: By the type invariant, there is necessarily a reference to the object.
> +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };

Ditto.

>          if is_zero {
>              // The count reached zero, we must free the memory.
>              //
> @@ -747,8 +731,7 @@ pub fn new_uninit(flags: Flags) -> Result<UniqueArc<MaybeUninit<T>>, AllocError>
>          // INVARIANT: The refcount is initialised to a non-zero value.
>          let inner = KBox::try_init::<AllocError>(
>              try_init!(ArcInner {
> -                // SAFETY: There are no safety requirements for this FFI call.
> -                refcount: Opaque::new(unsafe { bindings::REFCOUNT_INIT(1) }),
> +                refcount: Refcount::new(1),
>                  data <- pin_init::uninit::<T, AllocError>(),
>              }? AllocError),
>              flags,
> diff --git a/rust/kernel/sync/refcount.rs b/rust/kernel/sync/refcount.rs
> index a0fc22f6d645..8e7b9b0c1979 100644
> --- a/rust/kernel/sync/refcount.rs
> +++ b/rust/kernel/sync/refcount.rs
> @@ -71,6 +71,14 @@ pub fn dec(&self) {
>      /// must come after.
>      ///
>      /// Returns true if the resulting refcount is 0, false otherwise.
> +    ///
> +    /// # Notes
> +    ///
> +    /// A common pattern of using `Refcount` is to free memory when the reference count reaches
> +    /// zero. This means that the reference to `Refcount` could become invalid after calling this
> +    /// function. This is fine as long as the reference to `Refcount` is no longer used when this
> +    /// function returns `false`. It is not necessary to use raw pointers in this scenario, see
> +    /// https://github.com/rust-lang/rust/issues/55005.

This should be in patch 1?

---
Cheers,
Benno

>      #[inline]
>      #[must_use = "use `dec` instead if you do not need to test if it is 0"]
>      pub fn dec_and_test(&self) -> bool {