[nptl] Fix lock elision of pthread_mutex_trylock vs unlock
Commit Message
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.
Comments
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"
>
@@ -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
@@ -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,
new file mode 100644
@@ -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>
new file mode 100644
@@ -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"