From patchwork Thu Dec 15 22:29:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 88236 Delivered-To: patch@linaro.org Received: by 10.140.20.101 with SMTP id 92csp1064300qgi; Thu, 15 Dec 2016 14:29:25 -0800 (PST) X-Received: by 10.84.218.207 with SMTP id g15mr6799049plm.78.1481840965550; Thu, 15 Dec 2016 14:29:25 -0800 (PST) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id o5si4313609plh.162.2016.12.15.14.29.25 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 15 Dec 2016 14:29:25 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-75959-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org; spf=pass (google.com: domain of libc-alpha-return-75959-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-75959-patch=linaro.org@sourceware.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:subject:from:to:cc:date:in-reply-to :references:content-type:mime-version; q=dns; s=default; b=Zl7oH NRiVnvwGoHYv16tuY2QjK8lgbj7aMhA+DS2jYUNPNXDLMO9Rf4dBtox0PxJIj9h0 QuqNO33EL7vI6VQzwm3Q9YdltiN+df0d1APaiEQbMCVBALHLnGAHLjCCpB1h5GgZ S5YTm5r4ejBjgP50cJoZ+1FpZBryY6QUfer/+A= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:message-id:subject:from:to:cc:date:in-reply-to :references:content-type:mime-version; s=default; bh=zvZ19pELS+2 hcgOUpKOw2BWAW4I=; b=N+qszW5g5TCCekmwGlgWjM2DV67K2f8UhwmkOSO+Rzu StRoNDb77Zy5u3hFV9x1BhQhVvGp4Nu0Tlzq1crH3+FFqGUpT7fw6s+xCd8qTqZq Q6Y//XJPM54ElF4LuNneA6ULobuiyXeaYnIGE0TBLYP2fvIIG6jLSAAH/EDDbN2I = Received: (qmail 31410 invoked by alias); 15 Dec 2016 22:29:15 -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 31392 invoked by uid 89); 15 Dec 2016 22:29:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.0 required=5.0 tests=BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=sk:pthread, (unknown), blocked, died X-HELO: mx1.redhat.com Message-ID: <1481840946.14990.588.camel@redhat.com> Subject: Re: [PATCH][BZ #20973] Robust mutexes: Fix lost wake-up. From: Torvald Riegel To: GLIBC Devel Cc: vl@samba.org, Michael Adam , Rich Felker Date: Thu, 15 Dec 2016 23:29:06 +0100 In-Reply-To: <1481840825.14990.586.camel@redhat.com> References: <1481840825.14990.586.camel@redhat.com> Mime-Version: 1.0 On Thu, 2016-12-15 at 23:27 +0100, Torvald Riegel wrote: > See patch for a description. > > Tested on x86_64-linux with our tests and the test case from the > original bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1401665 > > OK? Now with an attached patch... commit 74be3b28d962a5a6d2eeff51b93d61ddf91e2d49 Author: Torvald Riegel Date: Thu Dec 15 16:06:28 2016 +0100 Robust mutexes: Fix lost wake-up. Assume that Thread 1 waits to acquire a robust mutex using futexes to block (and thus sets the FUTEX_WAITERS flag), and is unblocked when this mutex is released. If Thread 2 concurrently acquires the lock and is killed, Thread 1 can recover from the died owner but fail to restore the FUTEX_WAITERS flag. This can lead to a Thread 3 that also blocked using futexes at the same time as Thread 1 to not get woken up because FUTEX_WAITERS is not set anymore. The fix for this is to ensure that we continue to preserve the FUTEX_WAITERS flag whenever we may have set it or shared it with another thread. This is the same requirement as in the algorithm for normal mutexes, only that the robust mutexes need additional handling for died owners and thus preserving the FUTEX_WAITERS flag cannot be done just in the futex slowpath code. [BZ #20973] * nptl/pthread_mutex_lock.c (__pthread_mutex_lock_full): Fix lost wake-up in robust mutexes. * nptl/pthread_mutex_timedlock.c (pthread_mutex_timedlock): Likewise. diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c index bdfa529..01ac75e 100644 --- a/nptl/pthread_mutex_lock.c +++ b/nptl/pthread_mutex_lock.c @@ -182,6 +182,11 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) &mutex->__data.__list.__next); oldval = mutex->__data.__lock; + /* This is set to FUTEX_WAITERS iff we might have shared the + FUTEX_WAITERS flag with other threads, and therefore need to keep it + set to avoid lost wake-ups. We have the same requirement in the + simple mutex algorithm. */ + unsigned int assume_other_futex_waiters = 0; do { again: @@ -190,9 +195,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) /* The previous owner died. Try locking the mutex. */ int newval = id; #ifdef NO_INCR - newval |= FUTEX_WAITERS; + newval |= FUTEX_WAITERS | assume_other_futex_waiters; #else - newval |= (oldval & FUTEX_WAITERS); + newval |= (oldval & FUTEX_WAITERS) | assume_other_futex_waiters; #endif newval @@ -253,7 +258,11 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) } } - oldval = LLL_ROBUST_MUTEX_LOCK (mutex, id); + oldval = LLL_ROBUST_MUTEX_LOCK (mutex, + id | assume_other_futex_waiters); + /* See above. We set FUTEX_WAITERS and might have shared this flag + with other threads; thus, we need to preserve it. */ + assume_other_futex_waiters = FUTEX_WAITERS; if (__builtin_expect (mutex->__data.__owner == PTHREAD_MUTEX_NOTRECOVERABLE, 0)) diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index 07f0901..21e9eea 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -142,13 +142,19 @@ pthread_mutex_timedlock (pthread_mutex_t *mutex, &mutex->__data.__list.__next); oldval = mutex->__data.__lock; + /* This is set to FUTEX_WAITERS iff we might have shared the + FUTEX_WAITERS flag with other threads, and therefore need to keep it + set to avoid lost wake-ups. We have the same requirement in the + simple mutex algorithm. */ + unsigned int assume_other_futex_waiters = 0; do { again: if ((oldval & FUTEX_OWNER_DIED) != 0) { /* The previous owner died. Try locking the mutex. */ - int newval = id | (oldval & FUTEX_WAITERS); + int newval = id | (oldval & FUTEX_WAITERS) + | assume_other_futex_waiters; newval = atomic_compare_and_exchange_val_acq (&mutex->__data.__lock, @@ -203,8 +209,12 @@ pthread_mutex_timedlock (pthread_mutex_t *mutex, } } - result = lll_robust_timedlock (mutex->__data.__lock, abstime, id, + result = lll_robust_timedlock (mutex->__data.__lock, abstime, + id | assume_other_futex_waiters, PTHREAD_ROBUST_MUTEX_PSHARED (mutex)); + /* See above. We set FUTEX_WAITERS and might have shared this flag + with other threads; thus, we need to preserve it. */ + assume_other_futex_waiters = FUTEX_WAITERS; if (__builtin_expect (mutex->__data.__owner == PTHREAD_MUTEX_NOTRECOVERABLE, 0))