From 099b89d3840c30d7dd962e18668c2e6d39f0c626 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Tue, 12 Feb 2019 19:56:49 -0500 Subject: [PATCH] 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. --- src/thread/pthread_mutex_consistent.c | 10 +++++++--- src/thread/pthread_mutex_timedlock.c | 6 ++++-- src/thread/pthread_mutex_trylock.c | 10 +++++----- src/thread/pthread_mutex_unlock.c | 9 +++++++-- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/thread/pthread_mutex_consistent.c b/src/thread/pthread_mutex_consistent.c index 96b83b52..27c74e5b 100644 --- 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 9867f389..b95af251 100644 --- 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 783ca0c4..3fe59127 100644 --- 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 7dd00d27..ea9f54dd 100644 --- 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(); -- 2.25.1