linux-kernel - Re: [PATCH v3 2/4] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250221182849.21e9d83a@eugeo>
Date: Fri, 21 Feb 2025 18:28:49 +0000
From: Gary Guo <gary@...yguo.net>
To: Tamir Duberstein <tamird@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas
 Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor
 Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Wedson
 Almeida Filho <walmeida@...rosoft.com>, Alex Mantel
 <alexmantel93@...lbox.org>, Will Deacon <will@...nel.org>, Peter Zijlstra
 <peterz@...radead.org>, Mark Rutland <mark.rutland@....com>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] rust: convert `Arc` to use `Refcount`

On Fri, 21 Feb 2025 12:27:46 -0500
Tamir Duberstein <tamird@...il.com> wrote:

> > > > @@ -412,16 +402,14 @@ 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.
> > > > +        // NOTE: 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. However it is okay
> > > > +        // to have a transient reference to decrement the refcount, see
> > > > +        // https://github.com/rust-lang/rust/issues/55005.
> > > > +        let is_zero = unsafe { self.ptr.as_ref().refcount.dec_and_test() };  
> > >
> > > How come this careful handling is not required in into_unique_or_drop?
> > > At least, the SAFETY comment there is much more mundane.  
> >
> > Because `into_unique_or_drop` doesn't actually remove the allocation
> > (it only decrements refcount for non-zero or turn it into `UniqueArc`).  
> 
> I don't follow. This comment here talks about a race with another CPU
> decrementing to zero; isn't the same race possible between any
> combination of `into_unique_or_drop` and `drop` callers?

Oh you're right. It's indeed the same situation. However I'd want to
avoid repeating justification multiple times, given that this is really
an explaination note to the reader. Do you have any suggestion on how
to organise the comment so I can avoid repeating this multiple times?

Best,
Gary