linux-kernel - Re: [PATCH 12/13] rust: sync: introduce `CondVar`

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: <ZCrWPiqfAxsBNL+W@wedsonaf-dev>
Date:   Mon, 3 Apr 2023 10:35:58 -0300
From:   Wedson Almeida Filho <wedsonaf@...il.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     rust-for-linux@...r.kernel.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>,
        linux-kernel@...r.kernel.org,
        Wedson Almeida Filho <walmeida@...rosoft.com>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Waiman Long <longman@...hat.com>
Subject: Re: [PATCH 12/13] rust: sync: introduce `CondVar`

On Mon, Apr 03, 2023 at 10:59:59AM +0200, Peter Zijlstra wrote:
> On Thu, Mar 30, 2023 at 11:56:33AM -0300, Wedson Almeida Filho wrote:
> > On Thu, Mar 30, 2023 at 02:59:27PM +0200, Peter Zijlstra wrote:
> > > On Thu, Mar 30, 2023 at 01:39:53AM -0300, Wedson Almeida Filho wrote:
> > > 
> > > > +    fn wait_internal<T: ?Sized, B: Backend>(&self, wait_state: u32, guard: &mut Guard<'_, T, B>) {
> > > > +        let wait = Opaque::<bindings::wait_queue_entry>::uninit();
> > > > +
> > > > +        // SAFETY: `wait` points to valid memory.
> > > > +        unsafe { bindings::init_wait(wait.get()) };
> > > > +
> > > > +        // SAFETY: Both `wait` and `wait_list` point to valid memory.
> > > > +        unsafe {
> > > > +            bindings::prepare_to_wait_exclusive(self.wait_list.get(), wait.get(), wait_state as _)
> > > > +        };
> > > 
> > > I can't read this rust gunk, but where is the condition test gone?
> > > 
> > > Also, where is the loop gone to?
> > 
> > They're both at the caller. The usage of condition variables is something like:
> > 
> > while guard.value != v {
> >     condvar.wait_uninterruptible(&mut guard);
> > }
> > 
> > (Note that this is not specific to the kernel or to Rust: this is how condvars
> > work in general. You'll find this in any textbook on the topic.)
> > 
> > In the implementation of wait_internal(), we add the local wait entry to the
> > wait queue _before_ releasing the lock (i.e., before the test result can
> > change), so we guarantee that we don't miss wake up attempts.
> 
> Ah, so you've not yet been exposed to the wonderful 'feature' where
> pthread_cond_timedwait() gets called with .mutex=NULL and people expect
> things to just work :/ (luckily not accepted by the majority of
> implementations)

Rust doesn't have this problem: a `Guard` cannot exist without a lock, and one
cannot call `wait` or `wait_uninterruptible` without a `Guard`.

> Or a little more devious, calling signal and not holding the same mutex.

We don't require that callers hold the lock while singaling. If they signal when
the condition isn't satisfied (with or without the lock held, it doesn't
matter), it will just look like a spurious signal to the waiting thread.

> I just got alarm bells going off because I see prepare_to_wait without an
> obvious loop around it.

Fair enough, we do need a loop.