This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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  <>
> 	[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
   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
   thus, we must not access the lock after releasing it, or those
   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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]