| Message ID | 1479311642-8285-1-git-send-email-raji@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 2016-11-16 at 21:24 +0530, Rajalakshmi Srinivasaraghavan wrote: > The update of *adapt_count after the release of the lock causes a race > condition when 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. > > 2016-11-16 Rajalakshmi Srinivasaraghavan <raji@linux.vnet.ibm.com> > > [BZ #20822] > * sysdeps/unix/sysv/linux/powerpc/elision-unlock.c > (__lll_unlock_elision): Update adapt_count variable > inside the critical section. > --- > sysdeps/unix/sysv/linux/powerpc/elision-unlock.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c > index 43c5a67..e5e1afd 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c > +++ b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c > @@ -28,13 +28,12 @@ __lll_unlock_elision (int *lock, short *adapt_count, int pshared) > __libc_tend (0); > else > { > - 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. */ I'd rather mention the mutex destruction requirements by POSIX. This is what we're saying in sysdeps/nptl/lowlevellock.h for unlock: Evaluate PRIVATE before releasing the lock so that we do not violate the mutex destruction requirements. Specifically, we need to ensure that another thread can destroy the mutex (and reuse its memory) once it acquires the lock and when there will be no further lock acquisitions; thus, we must not access the lock after releasing it, or those accesses could be concurrent with mutex destruction or reuse of the memory. > if (*adapt_count > 0) > (*adapt_count)--; > + lll_unlock ((*lock), pshared); > + > } > return 0; > } Please make sure that there are no data races on at least adapt_count (ie, use atomic accesses consistently). I'm aware that this requires touching more files, but it makes the whole code more reliable and cleaner.
diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c index 43c5a67..e5e1afd 100644 --- a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c +++ b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c @@ -28,13 +28,12 @@ __lll_unlock_elision (int *lock, short *adapt_count, int pshared) __libc_tend (0); else { - 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); + } return 0; }