From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756763AbbLQQFz (ORCPT ); Thu, 17 Dec 2015 11:05:55 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:58678 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755398AbbLQQFy (ORCPT ); Thu, 17 Dec 2015 11:05:54 -0500 Date: Thu, 17 Dec 2015 17:05:49 +0100 From: Peter Zijlstra To: Linus Torvalds Cc: Ingo Molnar , ddaney@caviumnetworks.com, andrew.pinski@caviumnetworks.com, dave@stgolabs.net, will.deacon@arm.com, linux-kernel@vger.kernel.org Subject: [PATCH] locking/osq: Fix ordering of node initialisation in osq_lock Message-ID: <20151217160549.GH6344@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Linus, Please consider this patch for 4.4. --- Subject: locking/osq: Fix ordering of node initialisation in osq_lock From: Will Deacon Date: Fri, 11 Dec 2015 17:46:41 +0000 The Cavium guys reported a soft lockup on their arm64 machine, caused by c55a6ffa6285 ("locking/osq: Relax atomic semantics"): [ 68.909948] [] mutex_optimistic_spin+0x9c/0x1d0 [ 68.909951] [] __mutex_lock_slowpath+0x44/0x158 [ 68.909953] [] mutex_lock+0x54/0x58 [ 68.909956] [] kernfs_iop_permission+0x38/0x70 [ 68.909959] [] __inode_permission+0x88/0xd8 [ 68.909961] [] inode_permission+0x30/0x6c [ 68.909964] [] link_path_walk+0x68/0x4d4 [ 68.909966] [] path_openat+0xb4/0x2bc [ 68.909968] [] do_filp_open+0x74/0xd0 [ 68.909971] [] do_sys_open+0x14c/0x228 [ 68.909973] [] SyS_openat+0x3c/0x48 [ 68.909976] [] el0_svc_naked+0x24/0x28 This is because in osq_lock we initialise the node for the current CPU: node->locked = 0; node->next = NULL; node->cpu = curr; and then publish the current CPU in the lock tail: old = atomic_xchg_acquire(&lock->tail, curr); Once the update to lock->tail is visible to another CPU, the node is then live and can be both read and updated by concurrent lockers. Unfortunately, the ACQUIRE semantics of the xchg operation mean that there is no guarantee the contents of the node will be visible before lock tail is updated. This can lead to lock corruption when, for example, a concurrent locker races to set the next field. Fixes: c55a6ffa6285 ("locking/osq: Relax atomic semantics"): Reported-by: David Daney Reported-by: Andrew Pinski Tested-by: Andrew Pinski Acked-by: Davidlohr Bueso Signed-off-by: Will Deacon Signed-off-by: Peter Zijlstra (Intel) Link: http://lkml.kernel.org/r/1449856001-21177-1-git-send-email-will.deacon@arm.com --- kernel/locking/osq_lock.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) --- a/kernel/locking/osq_lock.c +++ b/kernel/locking/osq_lock.c @@ -93,10 +93,12 @@ bool osq_lock(struct optimistic_spin_que node->cpu = curr; /* - * ACQUIRE semantics, pairs with corresponding RELEASE - * in unlock() uncontended, or fastpath. + * We need both ACQUIRE (pairs with corresponding RELEASE in + * unlock() uncontended, or fastpath) and RELEASE (to publish + * the node fields we just initialised) semantics when updating + * the lock tail. */ - old = atomic_xchg_acquire(&lock->tail, curr); + old = atomic_xchg(&lock->tail, curr); if (old == OSQ_UNLOCKED_VAL) return true;