redesign robust mutex states to eliminate data races on type field

in order to implement ENOTRECOVERABLE, the implementation has
traditionally used a bit of the mutex type field to indicate that it's
recovered after EOWNERDEAD and will go into ENOTRECOVERABLE state if
pthread_mutex_consistent is not called before unlocking. while it's
only the thread that holds the lock that needs access to this
information (except possibly for the sake of pthread_mutex_consistent
choosing between EINVAL and EPERM for erroneous calls), the change to
the type field is formally a data race with all other threads that
perform any operation on the mutex. no individual bits race, and no
write races are possible, so things are "okay" in some sense, but it's
still not good.

this patch moves the recovery/consistency state to the mutex
owner/lock field which is rightfully mutable. bit 30, the same bit the
kernel uses with a zero owner to indicate that the previous owner died
holding the lock, is now used with a nonzero owner to indicate that
the mutex is held but has not yet been marked consistent. note that
the kernel ABI also reserves bit 29 not to appear in any tid, so the
sentinel value we use for ENOTRECOVERABLE, 0x7fffffff, does not clash
with any tid plus bit 30.

diff --git a/src/thread/pthread_mutex_consistent.c b/src/thread/pthread_mutex_consistent.c
index 96b83b52851b202aed9f817447129792833d1da1..27c74e5b6a0100ed18d826eeff3b994917160617 100644 (file)
--- a/src/thread/pthread_mutex_consistent.c
+++ b/src/thread/pthread_mutex_consistent.c
@@ -1,10 +1,14 @@
 #include "pthread_impl.h"
+#include "atomic.h"
 
 int pthread_mutex_consistent(pthread_mutex_t *m)
 {
-       if (!(m->_m_type & 8)) return EINVAL;
-       if ((m->_m_lock & 0x7fffffff) != __pthread_self()->tid)
+       int old = m->_m_lock;
+       int own = old & 0x3fffffff;
+       if (!(m->_m_type & 4) || !own || !(old & 0x40000000))
+               return EINVAL;
+       if (own != __pthread_self()->tid)
                return EPERM;
-       m->_m_type &= ~8U;
+       a_and(&m->_m_lock, ~0x40000000);
        return 0;
 }

diff --git a/src/thread/pthread_mutex_timedlock.c b/src/thread/pthread_mutex_timedlock.c
index 9867f3895f65962fe70281510e37b1eaa0b5d1f1..b95af2512e1c7d58765613df52bdf3699bf27e8a 100644 (file)
--- a/src/thread/pthread_mutex_timedlock.c
+++ b/src/thread/pthread_mutex_timedlock.c
@@ -16,10 +16,12 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec
        while (spins-- && m->_m_lock && !m->_m_waiters) a_spin();
 
        while ((r=__pthread_mutex_trylock(m)) == EBUSY) {
-               if (!(r=m->_m_lock) || ((r&0x40000000) && (type&4)))
+               r = m->_m_lock;
+               int own = r & 0x3fffffff;
+               if (!own && (!r || (type&4)))
                        continue;
                if ((type&3) == PTHREAD_MUTEX_ERRORCHECK
-                && (r&0x7fffffff) == __pthread_self()->tid)
+                   && own == __pthread_self()->tid)
                        return EDEADLK;
 
                a_inc(&m->_m_waiters);

diff --git a/src/thread/pthread_mutex_trylock.c b/src/thread/pthread_mutex_trylock.c
index 783ca0c40d745053aedc8f92bb16f8e85049125b..3fe591276f0dfd8403b4e46358f61c02f6df30a6 100644 (file)
--- a/src/thread/pthread_mutex_trylock.c
+++ b/src/thread/pthread_mutex_trylock.c
@@ -8,14 +8,14 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
        int tid = self->tid;
 
        old = m->_m_lock;
-       own = old & 0x7fffffff;
+       own = old & 0x3fffffff;
        if (own == tid && (type&3) == PTHREAD_MUTEX_RECURSIVE) {
                if ((unsigned)m->_m_count >= INT_MAX) return EAGAIN;
                m->_m_count++;
                return 0;
        }
-       if (own == 0x7fffffff) return ENOTRECOVERABLE;
-       if (own && (!(own & 0x40000000) || !(type & 4))) return EBUSY;
+       if (own == 0x3fffffff) return ENOTRECOVERABLE;
+       if (own || (old && !(type & 4))) return EBUSY;
 
        if (m->_m_type & 128) {
                if (!self->robust_list.off) {
@@ -25,6 +25,7 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
                if (m->_m_waiters) tid |= 0x80000000;
                self->robust_list.pending = &m->_m_next;
        }
+       tid |= old & 0x40000000;
 
        if (a_cas(&m->_m_lock, old, tid) != old) {
                self->robust_list.pending = 0;
@@ -39,9 +40,8 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m)
        self->robust_list.head = &m->_m_next;
        self->robust_list.pending = 0;
 
-       if (own) {
+       if (old) {
                m->_m_count = 0;
-               m->_m_type |= 8;
                return EOWNERDEAD;
        }
 

diff --git a/src/thread/pthread_mutex_unlock.c b/src/thread/pthread_mutex_unlock.c
index 7dd00d275b6dd9075d5964405ccb3928da1126a1..ea9f54dd032166225a7fc8fea140edb37fcfabba 100644 (file)
--- a/src/thread/pthread_mutex_unlock.c
+++ b/src/thread/pthread_mutex_unlock.c
@@ -7,13 +7,18 @@ int __pthread_mutex_unlock(pthread_mutex_t *m)
        int cont;
        int type = m->_m_type & 15;
        int priv = (m->_m_type & 128) ^ 128;
+       int new = 0;
 
        if (type != PTHREAD_MUTEX_NORMAL) {
                self = __pthread_self();
-               if ((m->_m_lock&0x7fffffff) != self->tid)
+               int old = m->_m_lock;
+               int own = old & 0x3fffffff;
+               if (own != self->tid)
                        return EPERM;
                if ((type&3) == PTHREAD_MUTEX_RECURSIVE && m->_m_count)
                        return m->_m_count--, 0;
+               if ((type&4) && (old&0x40000000))
+                       new = 0x7fffffff;
                if (!priv) {
                        self->robust_list.pending = &m->_m_next;
                        __vm_lock();
@@ -24,7 +29,7 @@ int __pthread_mutex_unlock(pthread_mutex_t *m)
                if (next != &self->robust_list.head) *(volatile void *volatile *)
                        ((char *)next - sizeof(void *)) = prev;
        }
-       cont = a_swap(&m->_m_lock, (type & 8) ? 0x7fffffff : 0);
+       cont = a_swap(&m->_m_lock, new);
        if (type != PTHREAD_MUTEX_NORMAL && !priv) {
                self->robust_list.pending = 0;
                __vm_unlock();