Bug#904158: glibc: pthread_cond_wait() is broken in the pshared case
Package: glibc
Version: 2.24-11+deb9u3
Severity: important
The short version is that pthread_cond_wait() is broken in the pshared
case in glibc in Stretch. The version in Sid is not affected because
that part received a large rewrite (that is why I use explicit the
version Stretch for the report).
The full explanation is attached as a patch. I also attached a testcase
to verify. Please note that x86 has handwritten assembly code for the
function which does not have the problem. All other architectures are
using the generic C code and share the problem.
On amdahl.d.o:~bigeasy/glibc you can try the following:
|strace -f ../testcase
|clone(child_stack=0xffff99483b10, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffff994842d0, tls=0xffff994848f0, child_tidptr=0xffff994842d0) = 26581
|[pid 26581] futex(0xaaaada3d40fc, FUTEX_WAIT_REQUEUE_PI, 1, NULL, 0xaaaada3d40b8 <unfinished ...>
|[pid 26579] clone(child_stack=0xffff98c83b10, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffff98c842d0, tls=0xffff98c848f0, child_tidptr=0xffff98c842d0) = 26582
|[pid 26582] futex(0xaaaada3d40fc, FUTEX_WAIT_REQUEUE_PI, 2, NULL, 0xaaaada3d40b8 <unfinished ...>
|[pid 26579] futex(0xaaaada3d40fc, FUTEX_WAKE_OP, 1, 1, 0xaaaada3d40f8, FUTEX_OP_SET<<28|0<<12|FUTEX_OP_CMP_GT<<24|0x1) = -1 EINVAL (Invalid argument)
|[pid 26579] futex(0xaaaada3d40fc, FUTEX_WAKE, 1) = -1 EINVAL (Invalid argument)
As you see the two waiting threads do FUTEX_WAIT_REQUEUE_PI and the
waker does FUTEX_WAKE* which is not valid. The program hangs at this
point.
With the patch attached:
|LD_LIBRARY_PATH=x/lib/aarch64-linux-gnu/ strace -f ../testcase
|child_stack=0xffff8edb6b10, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffff8edb72d0, tls=0xffff8edb78f0, child_tidptr=0xffff8edb72d0) = 26660
|[pid 26660] futex(0xaaaae4e1c0fc, FUTEX_WAIT, 1, NULL <unfinished ...>
|[pid 26659] clone(child_stack=0xffff8e5b6b10, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0xffff8e5b72d0, tls=0xffff8e5b78f0, child_tidptr=0xffff8e5b72d0) = 26661
|[pid 26661] futex(0xaaaae4e1c0fc, FUTEX_WAIT, 2, NULL <unfinished ...>
|[pid 26659] futex(0xaaaae4e1c0fc, FUTEX_WAKE_OP, 1, 1, 0xaaaae4e1c0f8, FUTEX_OP_SET<<28|0<<12|FUTEX_OP_CMP_GT<<24|0x1) = 1
and so on, the program finishes.
Sebastian
From: John Ogness <john.ogness@linutronix.de>
Date: Wed, 16 May 2018 22:34:41 +0200
Subject: [PATCH] condvar: do not use requeue for pshared condvars
With commit e42a990eccb (Update.) condvars were changed to not
store the mutex address when pshared. Instead, ~0l is stored.
This value is checked for in USE_REQUEUE_PI() to determine if
requeue should be used.
pthread_cond_signal() and pthread_cond_broadcast() both use
USE_REQUEUE_PI() with the mutex address stored on the condvar.
However, pthread_cond_wait() and pthread_cond_timedwait() use
USE_REQUEUE_PI() on the mutex address passed in from the caller
(even though that address is *not* stored on the condvar in the
pshared case). The result is that in the pshared case, the
wait functions are using requeue and the wake functions are
not! This is not allowed by the kernel (the waking futex call
returns EINVAL).
Modify the wait functions to use USE_REQUEUE_PI() on the mutex
address stored on the condvar, thus mirroring the behavior of
the wake functions.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
nptl/pthread_cond_timedwait.c | 4 +++-
nptl/pthread_cond_wait.c | 4 +++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/nptl/pthread_cond_timedwait.c b/nptl/pthread_cond_timedwait.c
index 711a51de20..9e6a393a43 100644
--- a/nptl/pthread_cond_timedwait.c
+++ b/nptl/pthread_cond_timedwait.c
@@ -163,6 +163,8 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
to check just the former. */
#if (defined lll_futex_timed_wait_requeue_pi \
&& defined __ASSUME_REQUEUE_PI)
+ pthread_mutex_t *mut = cond->__data.__mutex;
+
/* If pi_flag remained 1 then it means that we had the lock and the mutex
but a spurious waker raced ahead of us. Give back the mutex before
going into wait again. */
@@ -171,7 +173,7 @@ __pthread_cond_timedwait (pthread_cond_t *cond, pthread_mutex_t *mutex,
__pthread_mutex_cond_lock_adjust (mutex);
__pthread_mutex_unlock_usercnt (mutex, 0);
}
- pi_flag = USE_REQUEUE_PI (mutex);
+ pi_flag = USE_REQUEUE_PI (mut);
if (pi_flag)
{
diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
index 3f62acc6bd..7a4313cda6 100644
--- a/nptl/pthread_cond_wait.c
+++ b/nptl/pthread_cond_wait.c
@@ -162,6 +162,8 @@ __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex)
#if (defined lll_futex_wait_requeue_pi \
&& defined __ASSUME_REQUEUE_PI)
+ pthread_mutex_t *mut = cond->__data.__mutex;
+
/* If pi_flag remained 1 then it means that we had the lock and the mutex
but a spurious waker raced ahead of us. Give back the mutex before
going into wait again. */
@@ -170,7 +172,7 @@ __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex)
__pthread_mutex_cond_lock_adjust (mutex);
__pthread_mutex_unlock_usercnt (mutex, 0);
}
- pi_flag = USE_REQUEUE_PI (mutex);
+ pi_flag = USE_REQUEUE_PI (mut);
if (pi_flag)
{
--
2.15.1
#include <stdio.h>
#include <string.h>
#include <pthread.h>
#include <sched.h>
static pthread_t tid[3];
static volatile int tready[3];
static pthread_mutex_t m;
static pthread_cond_t c;
static void setup(void)
{
pthread_mutexattr_t mattr;
pthread_condattr_t cattr;
pthread_mutexattr_init(&mattr);
pthread_mutexattr_setprotocol(&mattr, PTHREAD_PRIO_INHERIT);
pthread_mutexattr_setpshared(&mattr, PTHREAD_PROCESS_SHARED);
pthread_mutex_init(&m, &mattr);
pthread_condattr_init(&cattr);
pthread_condattr_setpshared(&cattr, PTHREAD_PROCESS_SHARED);
pthread_cond_init(&c, &cattr);
}
static void *thread_main(void *arg)
{
unsigned long i = (unsigned long)arg;
pthread_mutex_lock(&m);
tready[i] = 1;
pthread_cond_wait(&c, &m);
tready[i] = 0;
pthread_mutex_unlock(&m);
return NULL;
}
static void wait_for(int count)
{
while (tready[0] + tready[1] + tready[2] != count)
/* spin */;
}
int main(void)
{
setup();
printf("creating thread\n");
pthread_create(&tid[0], NULL, thread_main, (void *)0);
printf("creating thread\n");
pthread_create(&tid[1], NULL, thread_main, (void *)1);
printf("waiting for 2 running threads\n");
wait_for(2);
pthread_mutex_lock(&m);
printf("signaling for a thread to wake and shutdown\n");
pthread_cond_signal(&c);
pthread_mutex_unlock(&m);
printf("waiting for 1 running thread\n");
wait_for(1);
printf("creating thread\n");
pthread_create(&tid[2], NULL, thread_main, (void *)2);
printf("waiting for 2 running threads\n");
wait_for(2);
pthread_mutex_lock(&m);
printf("signaling for a thread to wake and shutdown\n");
pthread_cond_signal(&c);
pthread_mutex_unlock(&m);
printf("waiting for 1 running thread\n");
wait_for(1);
pthread_mutex_lock(&m);
printf("signaling for a thread to wake and shutdown\n");
pthread_cond_signal(&c);
pthread_mutex_unlock(&m);
printf("waiting for 0 running threads\n");
wait_for(0);
printf("success\n");
pthread_join(tid[0], NULL);
pthread_join(tid[1], NULL);
pthread_join(tid[2], NULL);
return 0;
}
Reply to: