This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: BZ 20822 :powerpc: race condition in __lll_unlock_elision
- From: Torvald Riegel <triegel at redhat dot com>
- To: munroesj at linux dot vnet dot ibm dot com
- Cc: Rajalakshmi Srinivasaraghavan <raji at linux dot vnet dot ibm dot com>, libc-alpha at sourceware dot org, aaron Sawdey <acsawdey at linux dot vnet dot ibm dot com>, Ulrich Weigand <Ulrich dot Weigand at de dot ibm dot com>, Steve Munroe <sjmunroe at us dot ibm dot com>, carlos at redhat dot com, adhemerval dot zanella at linaro dot org, adconrad at ubuntu dot com, wschmidt at linux dot vnet dot ibm dot com
- Date: Wed, 23 Nov 2016 19:34:53 +0100
- Subject: Re: BZ 20822 :powerpc: race condition in __lll_unlock_elision
- Authentication-results: sourceware.org; auth=none
- References: <f777eebe-4a7b-87d2-1281-ab605c7fce8b@linux.vnet.ibm.com> <1479394010.7146.1154.camel@localhost.localdomain> <1479771736.9880.42.camel@oc7878010663> <1479804279.7146.1280.camel@localhost.localdomain> <1479837812.8455.18.camel@oc7878010663> <1479900303.7146.1412.camel@localhost.localdomain> <1479920508.10848.7.camel@oc7878010663>
On Wed, 2016-11-23 at 11:01 -0600, Steven Munroe wrote:
> On Wed, 2016-11-23 at 12:25 +0100, Torvald Riegel wrote:
> > 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.
> >
> I disagree, your definition of consistency is in this case misleading
> for my platform.
>
> > > 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.
> >
> So a comment to the effect that explicit atomics are not required for
> this case because it is within the critical region of the lll_lock /
> lll_unlock.
To be clear:
I want to see relaxed MO atomics being used.
I would appreciate comments with the note about why using
read-modify-write would not be helpful (ie, what you wrote above), and
why using atomic loads and stores would not be strictly necessary for
those particular accesses from *purely* an atomicity perspective, but
that we still use them for consistency with other accesses outside of
transactions to adapt_count.