From patchwork Tue Jul 14 20:21:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 7676 Received: (qmail 70788 invoked by alias); 14 Jul 2015 20:21:21 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 70772 invoked by uid 89); 14 Jul 2015 20:21:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Message-ID: <1436905273.19451.14.camel@localhost.localdomain> Subject: [PATCH][BZ 13690] Do not violate mutex destruction requirements. From: Torvald Riegel To: GLIBC Devel Cc: "Carlos O'Donell" , David Miller Date: Tue, 14 Jul 2015 22:21:13 +0200 Mime-Version: 1.0 POSIX and C++11 require that a thread can destroy a mutex if no thread owns the mutex, is blocked on the mutex, or will try to acquire it in the future. After destroying the mutex, it can reuse or unmap the underlying memory. Thus, we must not access a mutex' memory after releasing it. Currently, we can load the private flag after releasing the mutex, which is fixed by this patch. See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more background. We need to call futex_wake on the lock after releasing it, however. This is by design, and can lead to spurious wake-ups on unrelated futex words (e.g., when the mutex memory is reused for another mutex). This behavior is documented in the glibc-internal futex API and in recent drafts of the Linux kernel's futex documentation (see the draft_futex branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git). Carlos, do you want to consider this for 2.22, or should this rather target 2.23? The actual change is simple. Not tested. Could someone test this on a non-x86-linux machine (I don't have one handy). Dave, could you test on sparc, please? 2015-07-14 Torvald Riegel [BZ #13690] * sysdeps/nptl/lowlevellock.h (__lll_unlock): Do not access the lock after releasing it. (__lll_robust_unlock): Likewise. * nptl/pthread_mutex_unlock.c (__pthread_mutex_unlock_full): Likewise. * sysdeps/unix/sysv/linux/sparc/lowlevellock.h (lll_unlock): Likewise. (lll_robust_unlock): Likewise. commit abff5f4160ad21dcc71841c5e92dac8db0b81cff Author: Torvald Riegel Date: Tue Jul 14 21:58:34 2015 +0200 Do not violate mutex destruction requirements. POSIX and C++11 require that a thread can destroy a mutex if no other thread owns the mutex, is blocked on the mutex, or will try to acquire it in the future. After destroying the mutex, it can reuse or unmap the underlying memory. Thus, we must not access a mutex' memory after releasing it. Currently, we can load the private flag after releasing the mutex, which is fixed by this patch. See https://sourceware.org/bugzilla/show_bug.cgi?id=13690 for more background. We need to call futex_wake on the lock after releasing it, however. This is by design, and can lead to spurious wake-ups on unrelated futex words (e.g., when the mutex memory is reused for another mutex). This behavior is documented in the glibc-internal futex API and in recent drafts of the Linux kernel's futex documentation (see the draft_futex branch of git://git.kernel.org/pub/scm/docs/man-pages/man-pages.git). diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c index 80939ba..42e4ec9 100644 --- a/nptl/pthread_mutex_unlock.c +++ b/nptl/pthread_mutex_unlock.c @@ -232,16 +232,18 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr) /* One less user. */ --mutex->__data.__nusers; - /* Unlock. */ + /* Unlock. Load all necessary mutex data before releasing the mutex + to not violate the mutex destruction requirements (see + lll_unlock). */ + int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP; + int private = (robust + ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) + : PTHREAD_MUTEX_PSHARED (mutex)); if ((mutex->__data.__lock & FUTEX_WAITERS) != 0 || atomic_compare_and_exchange_bool_rel (&mutex->__data.__lock, 0, THREAD_GETMEM (THREAD_SELF, tid))) { - int robust = mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP; - int private = (robust - ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) - : PTHREAD_MUTEX_PSHARED (mutex)); INTERNAL_SYSCALL_DECL (__err); INTERNAL_SYSCALL (futex, __err, 2, &mutex->__data.__lock, __lll_private_flag (FUTEX_UNLOCK_PI, private)); diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h index 27f4142..d7eb282 100644 --- a/sysdeps/nptl/lowlevellock.h +++ b/sysdeps/nptl/lowlevellock.h @@ -191,14 +191,21 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, that's cast to void. */ /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock. If FUTEX was >1 (acquired, possibly with waiters), then wake any waiters. The waiter - that acquires the lock will set FUTEX to >1. */ + that acquires the lock will set FUTEX to >1. + Evaluate PRIVATE before releasing the lock so that we do not violate the + mutex destruction requirements. Specifically, we need to ensure that + another thread can destroy the mutex (and reuse its memory) once it + acquires the lock and when there will be no further lock acquisitions; + thus, we must not access the lock after releasing it, or those accesses + could be concurrent with mutex destruction or reuse of the memory. */ #define __lll_unlock(futex, private) \ ((void) \ ({ \ int *__futex = (futex); \ + int *__private = (private); \ int __oldval = atomic_exchange_rel (__futex, 0); \ if (__glibc_unlikely (__oldval > 1)) \ - lll_futex_wake (__futex, 1, private); \ + lll_futex_wake (__futex, 1, __private); \ })) #define lll_unlock(futex, private) \ __lll_unlock (&(futex), private) @@ -209,14 +216,17 @@ extern int __lll_robust_timedlock_wait (int *futex, const struct timespec *, that's cast to void. */ /* Unconditionally set FUTEX to 0 (not acquired), releasing the lock. If FUTEX had FUTEX_WAITERS set then wake any waiters. The waiter that acquires the - lock will set FUTEX_WAITERS. */ + lock will set FUTEX_WAITERS. + Evaluate PRIVATE before releasing the lock so that we do not violate the + mutex destruction requirements (see __lll_unlock). */ #define __lll_robust_unlock(futex, private) \ ((void) \ ({ \ int *__futex = (futex); \ + int *__private = (private); \ int __oldval = atomic_exchange_rel (__futex, 0); \ if (__glibc_unlikely (__oldval & FUTEX_WAITERS)) \ - lll_futex_wake (__futex, 1, private); \ + lll_futex_wake (__futex, 1, __private); \ })) #define lll_robust_unlock(futex, private) \ __lll_robust_unlock (&(futex), private) diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h index 7608c01..9fa7337 100644 --- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h +++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h @@ -126,17 +126,19 @@ __lll_robust_timedlock (int *futex, const struct timespec *abstime, #define lll_unlock(lock, private) \ ((void) ({ \ int *__futex = &(lock); \ + int __private = (private); \ int __val = atomic_exchange_24_rel (__futex, 0); \ if (__glibc_unlikely (__val > 1)) \ - lll_futex_wake (__futex, 1, private); \ + lll_futex_wake (__futex, 1, __private); \ })) #define lll_robust_unlock(lock, private) \ ((void) ({ \ int *__futex = &(lock); \ + int __private = (private); \ int __val = atomic_exchange_rel (__futex, 0); \ if (__glibc_unlikely (__val & FUTEX_WAITERS)) \ - lll_futex_wake (__futex, 1, private); \ + lll_futex_wake (__futex, 1, __private); \ })) #define lll_islocked(futex) \