linux-kernel - Re: [PATCH 3/3] rust: block: convert `block::mq` 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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH5fLghK1dtkF5bRpcRcu2SXZ6vgPoHGLRqW2=r0J3-2T3ALwQ@mail.gmail.com>
Date: Sat, 5 Oct 2024 13:59:44 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Greg KH <gregkh@...uxfoundation.org>, Gary Guo <gary@...yguo.net>, 
	Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Trevor Gross <tmgross@...ch.edu>, Jens Axboe <axboe@...nel.dk>, 
	Will Deacon <will@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Mark Rutland <mark.rutland@....com>, linux-block@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] rust: block: convert `block::mq` to use `Refcount`

On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> Hi Greg,
>
> "Greg KH" <gregkh@...uxfoundation.org> writes:
>
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
>
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].
>
> Best regards,
> Andreas
>
>
> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685

So it sounds like there is one refcount from the C side, and some
number of references from the Rust side. The function checks whether
there's only one Rust reference left, and if so, takes ownership of
the value, correct?

In that case, the CAS should have an acquire ordering to synchronize
with dropping the refcount 3->2 on another thread. Otherwise, you
might have a data race with the operations that happened just before
the 3->2 refcount drop.

Alice

On Sat, Oct 5, 2024 at 11:49 AM Andreas Hindborg <a.hindborg@...nel.org> wrote:
>
> Hi Greg,
>
> "Greg KH" <gregkh@...uxfoundation.org> writes:
>
> > On Fri, Oct 04, 2024 at 04:52:24PM +0100, Gary Guo wrote:
> >> There is an operation needed by `block::mq`, atomically decreasing
> >> refcount from 2 to 0, which is not available through refcount.h, so
> >> I exposed `Refcount::as_atomic` which allows accessing the refcount
> >> directly.
> >
> > That's scary, and of course feels wrong on many levels, but:
> >
> >
> >> @@ -91,13 +95,17 @@ pub(crate) unsafe fn start_unchecked(this: &ARef<Self>) {
> >>      /// C `struct request`. If the operation fails, `this` is returned in the
> >>      /// `Err` variant.
> >>      fn try_set_end(this: ARef<Self>) -> Result<*mut bindings::request, ARef<Self>> {
> >> -        // We can race with `TagSet::tag_to_rq`
> >> -        if let Err(_old) = this.wrapper_ref().refcount().compare_exchange(
> >> -            2,
> >> -            0,
> >> -            Ordering::Relaxed,
> >> -            Ordering::Relaxed,
> >> -        ) {
> >> +        // To hand back the ownership, we need the current refcount to be 2.
> >> +        // Since we can race with `TagSet::tag_to_rq`, this needs to atomically reduce
> >> +        // refcount to 0. `Refcount` does not provide a way to do this, so use the underlying
> >> +        // atomics directly.
> >> +        if this
> >> +            .wrapper_ref()
> >> +            .refcount()
> >> +            .as_atomic()
> >> +            .compare_exchange(2, 0, Ordering::Relaxed, Ordering::Relaxed)
> >> +            .is_err()
> >
> > Why not just call rust_helper_refcount_set()?  Or is the issue that you
> > think you might not be 2 here?  And if you HAVE to be 2, why that magic
> > value (i.e. why not just always be 1 and rely on normal
> > increment/decrement?)
> >
> > I know some refcounts are odd in the kernel, but I don't see where the
> > block layer is caring about 2 as a refcount anywhere, what am I missing?
>
> It is in the documentation, rendered version available here [1]. Let me
> know if it is still unclear, then I guess we need to update the docs.
>
> Also, my session from Recipes has a little bit of discussion regarding
> this refcount and it's use [2].
>
> Best regards,
> Andreas
>
>
> [1] https://rust.docs.kernel.org/kernel/block/mq/struct.Request.html#implementation-details
> [2] https://youtu.be/1LEvgkhU-t4?si=B1XnJhzCCNnUtRsI&t=1685
>