Re: [PATCH] powerpc: Fix race condition in lock elision

[Torvald Riegel <triegel at redhat dot com>]

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.