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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87msjioax2.fsf@kernel.org>
Date: Sat, 05 Oct 2024 16:56:57 +0200
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Gary Guo" <gary@...yguo.net>
Cc: "Alice Ryhl" <aliceryhl@...gle.com>,  "Greg KH"
 <gregkh@...uxfoundation.org>,  "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`

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

> On Sat, 5 Oct 2024 13:59:44 +0200
> Alice Ryhl <aliceryhl@...gle.com> wrote:
>
>> 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
>
> The code as is is fine since there's no data protected in
> `RequestDataWrapper` yet (in fact it's not even generic yet). I know
> Andreas does want to introduce driver-specific data into that, so in
> the long term the acquire would be necessary.
>
> Andreas, please let me know if you want me to make the change now, or
> you'd rather change the ordering when you introduce data to
> `RequestDataWrapper`.

I guess we will have said data dependencies when we are going to run
drop for fields in the private data area. Thanks for pointing that out.
I will update the ordering when I submit that patch.

As I mentioned before, I would rather we do not apply this patch before
we get a way to inline helpers.


BR Andreas