Re: [PATCH,nptl] Fix lock elision of pthread_mutex_trylock vs unlock

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

I think this bug is already being tracked by BZ#16657 and
Andreas Schwab already created a fix similar to this one.

https://sourceware.org/bugzilla/show_bug.cgi?id=16657

On 28-08-2014 14:58, Samuel Thibault wrote:
> Hello,
>
> On hardware with RTM, the following crashes:
>
> #include <pthread.h>
> pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER;
> int main(int argc, char *argv[]) {
> 	pthread_mutex_trylock(&m);
> 	pthread_mutex_unlock(&m);
> 	if (pthread_mutex_destroy(&m))
> 		*(int*)0 = 0;
> 	return 0;
> }
>
> The state of the mutex is indeed:
>
> (gdb) p/x m
> $1 = {__data = {__lock = 0x0, __count = 0x0, __owner = 0x0, __nusers = 0xffffffff, __kind = 0x0, 
>     __spins = 0x0, __elision = 0x3, __list = {__prev = 0x0, __next = 0x0}}, __size = {0x0 <repeats 12 times>, 
>     0xff, 0xff, 0xff, 0xff, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x3, 0x0 <repeats 17 times>}, __align = 0x0}
>
> What happens is that pthread_mutex_trylock does elision (DO_ELISION),
> but pthread_mutex_unlock is not aware that trylock did, and doesn't do
> elision, and thus erroneously does __nusers-- and pthread_mutex_destroy
> returns EBUSY.
>
> pthread_mutex_trylock.c mentions early in the file that we "don't force
> elision in trylock, because this can lead to inconsistent lock state if
> the lock was actually busy.". I don't know the details, but if trylock
> should really not do elision, then it shouldn't do it :) The following
> patch does this, and it indeed fixes the issue.
>
> --- a/nptl/pthread_mutex_trylock.c
> +++ b/nptl/pthread_mutex_trylock.c
> @@ -77,9 +77,6 @@ __pthread_mutex_trylock (mutex)
>        return 0;
>
>      case PTHREAD_MUTEX_TIMED_NP:
> -      if (DO_ELISION (mutex))
> -	goto elision;
> -      /*FALL THROUGH*/
>      case PTHREAD_MUTEX_ADAPTIVE_NP:
>      case PTHREAD_MUTEX_ERRORCHECK_NP:
>        if (lll_trylock (mutex->__data.__lock) != 0)
>
> But perhaps this case is actually safe (I haven't investigated the
> details) and thus it's the unlock part which needs fixed, as the
> following patch does:
>
> Andy?
>
> Samuel
>
> diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
> index 95ae933..299877b 100644
> --- a/nptl/pthread_mutex_unlock.c
> +++ b/nptl/pthread_mutex_unlock.c
> @@ -27,6 +27,10 @@
>  #define lll_unlock_elision(a,b) ({ lll_unlock (a,b); 0; })
>  #endif
>
> +#ifndef DO_ELISION
> +#define DO_ELISION(m) 0
> +#endif
> +
>  static int
>  internal_function
>  __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
> @@ -44,7 +48,7 @@ __pthread_mutex_unlock_usercnt (mutex, decr)
>      return __pthread_mutex_unlock_full (mutex, decr);
>
>    if (__builtin_expect (type, PTHREAD_MUTEX_TIMED_NP)
> -      == PTHREAD_MUTEX_TIMED_NP)
> +      == PTHREAD_MUTEX_TIMED_NP && !DO_ELISION(mutex))
>      {
>        /* Always reset the owner field.  */
>      normal:
> @@ -60,7 +64,9 @@ __pthread_mutex_unlock_usercnt (mutex, decr)
>
>        return 0;
>      }
> -  else if (__glibc_likely (type == PTHREAD_MUTEX_TIMED_ELISION_NP))
> +  else if (__glibc_likely (type == PTHREAD_MUTEX_TIMED_ELISION_NP) ||
> +           (__glibc_likely (type == PTHREAD_MUTEX_TIMED_NP) &&
> +		  DO_ELISION(mutex)))
>      {
>        /* Don't reset the owner/users fields for elision.  */
>        return lll_unlock_elision (mutex->__data.__lock,
> diff --git a/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c b/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c
> new file mode 100644
> index 0000000..048dd5d
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c
> @@ -0,0 +1,22 @@
> +/* Elided version of pthread_mutex_trylock.
> +   Copyright (C) 2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <elision-conf.h>
> +#include <force-elision.h>
> +
> +#include <nptl/pthread_mutex_unlock.c>
> diff --git a/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c b/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c
> new file mode 100644
> index 0000000..80b594c
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/pthread_mutex_unlock.c
> @@ -0,0 +1,22 @@
> +/* Elided version of pthread_mutex_trylock.
> +   Copyright (C) 2011-2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <elision-conf.h>
> +#include "force-elision.h"
> +
> +#include "nptl/pthread_mutex_unlock.c"
>