Re: BZ 20822 :powerpc: race condition in __lll_unlock_elision

[Torvald Riegel <triegel at redhat dot com>]

On Tue, 2016-11-22 at 12:03 -0600, Steven Munroe wrote:
> On Tue, 2016-11-22 at 09:44 +0100, Torvald Riegel wrote:
> > On Mon, 2016-11-21 at 17:42 -0600, Steven Munroe wrote:
> > > On Thu, 2016-11-17 at 15:46 +0100, Torvald Riegel wrote:
> > > > On Tue, 2016-11-15 at 21:42 +0530, Rajalakshmi Srinivasaraghavan wrote:
> > > > > The initial problem reported was memory corruption in MongoDB only on 
> > > > > Ubuntu 16.04 ppc64le(not on Ubuntu 15).
> > > > > The problem was not easily recreatable and debugging with many tools and 
> > > > > creative ideas, the problem is narrowed down to lock elision in glibc.
> > > > > Ubuntu 16.04 has glibc 2.23 with lock elision enabled by default which 
> > > > > made the testcase fail only on 16.04.
> > > > > 
> > > > > As stated in BZ 20822, short description is
> > > > > 
> > > > > "The update of *adapt_count after the release of the lock causes a race 
> > > > > condition. Thread A unlocks, thread B continues and returns, destroying 
> > > > > mutex on stack,
> > > > >   then gets into another function, thread A writes to *adapt_count and 
> > > > > corrupts stack.The window is very narrow and requires that the
> > > > > machine be in smt mode, so likely the two threads have to be on the same 
> > > > > core in order to hit this"
> > > > > 
> > > > > Looking back the file changes in 
> > > > > sysdeps/unix/sysv/linux/powerpc/elision-unlock.c, suspecting the commit
> > > > >   https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=fadd2ad9cc36115440d50b0eae9299e65988917d which is introduced to improve
> > > > > the performance.
> > > > > 
> > > > > Thinking of the following fixes.
> > > > > 
> > > > > 1) Update of adapt_count before the unlock. But I have still not 
> > > > > identified if its going to affect the performance.
> > > > 
> > > > Because of the requirements on mutex destruction (and same for rwlock
> > > > etc.), a thread must not access any data associated with the mutex after
> > > > having unlocked it (this includes reads because it's correct to also
> > > > unmap the memory holding the mutex after destroying the mutex). 
> > > > 
> > > > > 2) In elision-lock.c/elision-trylock.c update adapt_count right after 
> > > > > the lock acquire at the end of the function.
> > > > > In either case we have to be careful not to introduce another race 
> > > > > condition.
> > > > 
> > > > Every access to mutex data outside of transactions must use atomic
> > > > operations if there would be data races otherwise (according to the C11
> > > > memory model).  I know that this isn't followed in the current mutex
> > > > code, but it really should be, and your change qualifies as new code so
> > > > please take care of this too.
> > > > 
> > > In this case (with the proposed patch) the adapt_count update is within
> > > the critical region and adding C11 atomics does not add value and would
> > > generally make the logical harder to read. 
> > > 
> > > __atomic_store_n (adapt_count, (__atomic_load_n(adapt_count,
> > > __ATOMIC_RELAXED) - 1), __ATOMIC_RELAXED)
> > > 
> > > Is a lot of syntax without any real value.
> > 
> > First, it would actually be this in glibc:
> > atomic_store_relaxed (&adapt_count,
> >   atomic_load_relaxed(&adapt_count) - 1);
> > 
> This is not a good example for you case. The proposed patch:
> 
> -      lll_unlock ((*lock), pshared);
> -
> -      /* Update the adapt count AFTER completing the critical section.
> -         Doing this here prevents unneeded stalling when entering
> -         a critical section.  Saving about 8% runtime on P8.  */
> +      /* Update the adapt count in the critical section to
> +         prevent race condition as mentioned in BZ 20822.  */
>        if (*adapt_count > 0)
>  	(*adapt_count)--;
> +      lll_unlock ((*lock), pshared);
> 
> makes it clear that this test and update of the adapt_count is contained
> with the acquire/release semantics of the lll_lock and lll_unlock.

However, it does not make it clear that it should be accessed atomically
outside of transactions.

Another reason why we want to use atomic operations consistently is to
at least allow us to really use C11 atomics in the future (or some sort
of similar underlying type), so that we can more easily catch cases
where atomic operations should be used but aren't.  If we don't
specifically use relaxed-MO operations, the default will be SC.  I'm
sure you'd like to avoid that too.

This is a consistency argument wrt other code.  The fact that the atomic
operation is in a transaction too is just another fact.
We don't want people to assume that plain memory accesses are
sufficient, for example because all accesses to adapt_count are within
transactions.

> Also if we add the explicit syntax you propose, someone, who has not
> studied the architecture, might leap to the conclusion that add_fetch
> would be even better.

I disagree.  If you look at the rules we've set up for concurrent code
in general (eg, see "Concurrency" on the wiki, or the concurrent code we
have converted), we require developers to document what the code does;
this means memory order and operations used, and the high-level intent.
If you do this properly, the misunderstandings you worry should not
happen.

> Which would be absolutely wrong. This would generate a larx-add-stcx
> sequence which has two very bad side-effects.
> 
> 1) on the fall back leg, it would a force a larx-hit-stcx flush hazard
> with the following lll_unlock and cause bad performance.
> 2) on the transaction leg, it would be redundant and just slow things
> down.

So why not add this as a comment?  The use of nonatomic accesses does
not imply these points, BTW.

Also, the same concerns for how to access adapt_count apply to other
accesses to it that may not or may be in a transaction (due to nesting).
You must use atomic accesses for these, so you'd need the explaining
comment anyway.

On a positive note, it seems that we agree that we want to clarity of
the source code.  I'm arguing that more comments and a consistent use of
atomic operations (with proper memory orders and comments, of course)
will get us there.  That's why I suggested that the patch should clean
up and document all accesses to adapt_count.