From patchwork Wed Dec 21 18:03:04 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tulio Magno Quites Machado Filho X-Patchwork-Id: 707914 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tkMwQ2Zbsz9t14 for ; Thu, 22 Dec 2016 05:03:50 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b="nNRMcfPS"; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:cc:subject:date:in-reply-to:references :message-id; q=dns; s=default; b=voTE2lKNY/OZWpGm2LZJKJXHWA+UQri Nv8nn0CCq5WjlSQWPd6QJBG3lpzEmO535U3oifT9irjK57VYv/CnLy/AX6RanS4y Bxq6+YFjucETArhMXO/qAEuByZ3MKN2atciaIxOOpxoagKG3E1JfmsU9tuCPg7Hc Nx/o8cUqYoH8= 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:from:to:cc:subject:date:in-reply-to:references :message-id; s=default; bh=fsCcsriUl4Z55/oVxR7hICs00fE=; b=nNRMc fPS9nC6IgVy25fIxU5FS1vaINP40345U+7bfyOIKtn/48K4O+8Mqv2cthFd3f/PC Ok/e22euRRxZhhmI3koPjhpmos961u7G0+G4XcJ2gyZaKZyprwGEF77WxFCKPMX+ h1/pl77RhreD+kYvwLxM65pPth+qAe+YvEiPvQ= Received: (qmail 34687 invoked by alias); 21 Dec 2016 18:03:41 -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 34610 invoked by uid 89); 21 Dec 2016 18:03:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=completing, unlocks, exhausted, munroe X-HELO: mx0a-001b2d01.pphosted.com From: "Tulio Magno Quites Machado Filho" To: libc-alpha@sourceware.org Cc: vapier@gentoo.org, raji@linux.vnet.ibm.com, triegel@redhat.com, munroesj@linux.vnet.ibm.com Subject: [PATCHv3] powerpc: Fix write-after-destroy in lock elision Date: Wed, 21 Dec 2016 16:03:04 -0200 In-Reply-To: <20161212043646.GL10558@vapier.lan> References: <20161212043646.GL10558@vapier.lan> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16122118-0032-0000-0000-000005274898 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16122118-0033-0000-0000-000011A986A5 Message-Id: <1482343384-28430-1-git-send-email-tuliom@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-12-21_13:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1612050000 definitions=main-1612210279 From: Rajalakshmi Srinivasaraghavan Changes since v2: - Fixed coding style. Changes since v1: - Removed references to data race by the actual error: write-after-destroy. - Enclosed adapt_count accesses in C11 atomics. --- 8< --- The update of *adapt_count after the release of the lock causes a race condition when thread A unlocks, thread B continues and destroys the mutex, and thread A writes to *adapt_count. 2016-12-21 Rajalakshmi Srinivasaraghavan Steven Munroe Tulio Magno Quites Machado Filho [BZ #20822] * sysdeps/unix/sysv/linux/powerpc/elision-lock.c (__lll_lock_elision): Access adapt_count via C11 atomics. * sysdeps/unix/sysv/linux/powerpc/elision-trylock.c (__lll_trylock_elision): Likewise. * sysdeps/unix/sysv/linux/powerpc/elision-unlock.c (__lll_unlock_elision): Update adapt_count variable inside the critical section. --- sysdeps/unix/sysv/linux/powerpc/elision-lock.c | 8 +++++--- sysdeps/unix/sysv/linux/powerpc/elision-trylock.c | 7 ++++--- sysdeps/unix/sysv/linux/powerpc/elision-unlock.c | 14 +++++++++----- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c index dd1e4c3..86adbc9 100644 --- a/sysdeps/unix/sysv/linux/powerpc/elision-lock.c +++ b/sysdeps/unix/sysv/linux/powerpc/elision-lock.c @@ -45,7 +45,7 @@ int __lll_lock_elision (int *lock, short *adapt_count, EXTRAARG int pshared) { - if (*adapt_count > 0) + if (atomic_load_relaxed (adapt_count) > 0) { goto use_lock; } @@ -67,7 +67,8 @@ __lll_lock_elision (int *lock, short *adapt_count, EXTRAARG int pshared) if (_TEXASRU_FAILURE_PERSISTENT (__builtin_get_texasru ())) { if (aconf.skip_lock_internal_abort > 0) - *adapt_count = aconf.skip_lock_internal_abort; + atomic_store_relaxed (adapt_count, + aconf.skip_lock_internal_abort); goto use_lock; } } @@ -75,7 +76,8 @@ __lll_lock_elision (int *lock, short *adapt_count, EXTRAARG int pshared) /* Fall back to locks for a bit if retries have been exhausted */ if (aconf.try_tbegin > 0 && aconf.skip_lock_out_of_tbegin_retries > 0) - *adapt_count = aconf.skip_lock_out_of_tbegin_retries; + atomic_store_relaxed (adapt_count, + aconf.skip_lock_out_of_tbegin_retries); use_lock: return LLL_LOCK ((*lock), pshared); diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c b/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c index 0807a6a..6061856 100644 --- a/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c +++ b/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c @@ -34,7 +34,7 @@ __lll_trylock_elision (int *futex, short *adapt_count) __libc_tabort (_ABORT_NESTED_TRYLOCK); /* Only try a transaction if it's worth it. */ - if (*adapt_count > 0) + if (atomic_load_relaxed (adapt_count) > 0) { goto use_lock; } @@ -49,7 +49,7 @@ __lll_trylock_elision (int *futex, short *adapt_count) __libc_tend (0); if (aconf.skip_lock_busy > 0) - *adapt_count = aconf.skip_lock_busy; + atomic_store_relaxed (adapt_count, aconf.skip_lock_busy); } else { @@ -59,7 +59,8 @@ __lll_trylock_elision (int *futex, short *adapt_count) result in another failure. Use normal locking now and for the next couple of calls. */ if (aconf.skip_trylock_internal_abort > 0) - *adapt_count = aconf.skip_trylock_internal_abort; + atomic_store_relaxed (adapt_count, + aconf.skip_trylock_internal_abort); } } diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c index 43c5a67..0e0baf5 100644 --- a/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c +++ b/sysdeps/unix/sysv/linux/powerpc/elision-unlock.c @@ -28,13 +28,17 @@ __lll_unlock_elision (int *lock, short *adapt_count, int pshared) __libc_tend (0); else { - lll_unlock ((*lock), pshared); - - /* Update the adapt count AFTER completing the critical section. - Doing this here prevents unneeded stalling when entering - a critical section. Saving about 8% runtime on P8. */ + /* Update adapt_count in the critical section to prevent a + write-after-destroy error as mentioned in BZ 20822. The + following update of adapt_count is clearly contained within + the critical region of the fall-back lock as it immediately + precedes the unlock. Attempting to use C11 atomics to access + adapt_count would be (at minimum) misleading and (at worse) a + serious error forcing a larx-hit-stcx flush. */ if (*adapt_count > 0) (*adapt_count)--; + + lll_unlock ((*lock), pshared); } return 0; }