From patchwork Tue May 24 14:27:26 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Zijlstra X-Patchwork-Id: 625688 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3rDdNC54DZz9s1h for ; Wed, 25 May 2016 00:39:39 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964856AbcEXOiT (ORCPT ); Tue, 24 May 2016 10:38:19 -0400 Received: from merlin.infradead.org ([205.233.59.134]:43108 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932744AbcEXOiQ (ORCPT ); Tue, 24 May 2016 10:38:16 -0400 Received: from j217100.upc-j.chello.nl ([24.132.217.100] helo=twins) by merlin.infradead.org with esmtpsa (Exim 4.85_2 #1 (Red Hat Linux)) id 1b5DSn-0000rk-55; Tue, 24 May 2016 14:37:41 +0000 Received: by twins (Postfix, from userid 0) id A96F21253182E; Tue, 24 May 2016 16:37:39 +0200 (CEST) Message-Id: <20160524143649.673861121@infradead.org> User-Agent: quilt/0.61-1 Date: Tue, 24 May 2016 16:27:26 +0200 From: Peter Zijlstra To: linux-kernel@vger.kernel.org, torvalds@linux-foundation.org, manfred@colorfullife.com, dave@stgolabs.net, paulmck@linux.vnet.ibm.com, will.deacon@arm.com Cc: boqun.feng@gmail.com, Waiman.Long@hpe.com, tj@kernel.org, pablo@netfilter.org, kaber@trash.net, davem@davemloft.net, oleg@redhat.com, netfilter-devel@vger.kernel.org, sasha.levin@oracle.com, hofrat@osadl.org, "Peter Zijlstra (Intel)" Subject: [RFC][PATCH 3/3] locking,netfilter: Fix nf_conntrack_lock() References: <20160524142723.178148277@infradead.org> MIME-Version: 1.0 Content-Disposition: inline; filename=peterz-locking-netfilter.patch Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org nf_conntrack_lock{,_all}() is borken as it misses a bunch of memory barriers to order the whole global vs local locks scheme. Even x86 (and other TSO archs) are affected. Signed-off-by: Peter Zijlstra (Intel) --- net/netfilter/nf_conntrack_core.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -74,7 +74,18 @@ void nf_conntrack_lock(spinlock_t *lock) spin_lock(lock); while (unlikely(nf_conntrack_locks_all)) { spin_unlock(lock); + /* + * Order the nf_contrack_locks_all load vs the spin_unlock_wait() + * loads below, to ensure locks_all is indeed held. + */ + smp_rmb(); /* spin_lock(locks_all) */ spin_unlock_wait(&nf_conntrack_locks_all_lock); + /* + * The control dependency's LOAD->STORE order is enough to + * guarantee the spin_lock() is ordered after the above + * unlock_wait(). And the ACQUIRE of the lock ensures we are + * fully ordered against the spin_unlock() of locks_all. + */ spin_lock(lock); } } @@ -119,14 +130,31 @@ static void nf_conntrack_all_lock(void) spin_lock(&nf_conntrack_locks_all_lock); nf_conntrack_locks_all = true; + /* + * Order the above store against the spin_unlock_wait() loads + * below, such that if nf_conntrack_lock() observes lock_all + * we must observe lock[] held. + */ + smp_mb(); /* spin_lock(locks_all) */ + for (i = 0; i < CONNTRACK_LOCKS; i++) { spin_unlock_wait(&nf_conntrack_locks[i]); } + /* + * Ensure we observe all state prior to the spin_unlock()s + * observed above. + */ + smp_acquire__after_ctrl_dep(); } static void nf_conntrack_all_unlock(void) { - nf_conntrack_locks_all = false; + /* + * All prior stores must be complete before we clear locks_all. + * Otherwise nf_conntrack_lock() might observe the false but not + * the entire critical section. + */ + smp_store_release(&nf_conntrack_locks_all, false); spin_unlock(&nf_conntrack_locks_all_lock); }