linux-kernel - Re: [PATCH] doc: Update wake_up() & co. memory-barrier guarantees

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]

Date:   Tue, 26 Jun 2018 17:30:25 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Andrea Parri <andrea.parri@...rulasolutions.com>
Cc:     linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
        Alan Stern <stern@...land.harvard.edu>,
        Will Deacon <will.deacon@....com>,
        Boqun Feng <boqun.feng@...il.com>,
        Nicholas Piggin <npiggin@...il.com>,
        David Howells <dhowells@...hat.com>,
        Jade Alglave <j.alglave@....ac.uk>,
        Luc Maranget <luc.maranget@...ia.fr>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Akira Yokosawa <akiyks@...il.com>,
        Daniel Lustig <dlustig@...dia.com>,
        Jonathan Corbet <corbet@....net>,
        Ingo Molnar <mingo@...hat.com>,
        Randy Dunlap <rdunlap@...radead.org>
Subject: Re: [PATCH] doc: Update wake_up() & co. memory-barrier guarantees

On Tue, Jun 26, 2018 at 12:09:42PM +0200, Andrea Parri wrote:
> > > Same RFC for the first comment:
> > > 
> > >   /*
> > >    * The above implies an smp_mb(), which matches with the smp_wmb() from
> > >    * woken_wake_function() such that if we observe WQ_FLAG_WOKEN we must
> > >    * also observe all state before the wakeup.
> > >    */
> > > 
> > > What is the corresponding snippet & BUG_ON()?
> > 
> > The comment there suggest:
> > 
> > 	if (condition)
> > 		break;
> > 
> > 	set_current_state(UNINTERRUPTIBLE);		condition = true;
> > 	/* smp_mb() */					smp_wmb();
> > 							wq_entry->flags |= WQ_FLAG_WOKEN;
> > 	if (!wq_entry->flags & WQ_FLAG_WOKEN)
> > 		schedule();
> > 
> > 
> > 	BUG_ON((wq_entry->flags & WQ_FLAG_WOKEN) && !condition);
> > 
> > 
> > But looking at that now, I think that's wrong. Because the the purpose
> > was that, if we don't do the try_to_wake_up(), our task still needs to
> > observe the condition store.
> > 
> > But for that we need a barrier between the wq_entry->flags load and the
> > second condition test, which would (again) be B, not A.
> 
> Agreed.  Now that I stared at the code a bit more, I think that (A) is
> still needed for the synchronization on "->state" and "->flags" (an SB
> pattern seems again to be hidden in the call to try_to_wake_up()):
> 
>   p->state = mode;                           wq_entry->flags |= WQ_FLAG_WOKEN;
>   smp_mb(); // A                             try_to_wake_up():
>   if (!(wq_entry->flags & WQ_FLAG_WOKEN))      <full barrier>
>      schedule()                                if (!(p->state & mode))
>                                                  goto out;
> 
>   BUG_ON(!(wq_entry->flags & WQ_FLAG_WOKEN) && !(p->state & mode))
> 
> So, I think that we should keep (A).

Yes, very much so. Once we actually get to use ttwu() that barrier is
required.

> I am planning to send these changes (smp_mb() in woken_wake_function()
> and fixes to the comments) as a separate patch.

Probably makes sense. Thanks for looking at this, I have vague memories
of being slightly confused when I wrote all that :-)