From: Peter Zijlstra <peterz@infradead.org>
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, peterz@infradead.org
Subject: [PATCH -v3 6/8] locking,netfilter: Fix nf_conntrack_lock()
Date: Tue, 31 May 2016 11:41:40 +0200 [thread overview]
Message-ID: <20160531094844.222530154@infradead.org> (raw)
In-Reply-To: 20160531094134.606249808@infradead.org
[-- Attachment #1: peterz-locking-netfilter.patch --]
[-- Type: text/plain, Size: 1723 bytes --]
Even with spin_unlock_wait() fixed, 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) <peterz@infradead.org>
---
net/netfilter/nf_conntrack_core.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -83,6 +83,12 @@ 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);
spin_lock(lock);
}
@@ -128,6 +134,12 @@ 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]);
}
@@ -135,7 +147,11 @@ static void nf_conntrack_all_lock(void)
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);
}
next prev parent reply other threads:[~2016-05-31 9:53 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-31 9:41 [PATCH -v3 0/8] spin_unlock_wait borkage and assorted bits Peter Zijlstra
2016-05-31 9:41 ` [PATCH -v3 1/8] locking: Replace smp_cond_acquire with smp_cond_load_acquire Peter Zijlstra
2016-05-31 9:41 ` [PATCH -v3 2/8] locking: Introduce cmpwait() Peter Zijlstra
2016-05-31 9:41 ` [PATCH -v3 3/8] locking: Introduce smp_acquire__after_ctrl_dep Peter Zijlstra
2016-06-01 13:52 ` Boqun Feng
2016-06-01 16:22 ` Peter Zijlstra
2016-06-01 23:19 ` Boqun Feng
2016-05-31 9:41 ` [PATCH -v3 4/8] locking, arch: Update spin_unlock_wait() Peter Zijlstra
2016-06-01 11:24 ` Will Deacon
2016-06-01 11:37 ` Peter Zijlstra
2016-05-31 9:41 ` [PATCH -v3 5/8] locking: Update spin_unlock_wait users Peter Zijlstra
2016-05-31 9:41 ` Peter Zijlstra [this message]
2016-05-31 9:41 ` [PATCH -v3 7/8] locking: Move smp_cond_load_acquire() and friends into asm-generic/barrier.h Peter Zijlstra
2016-05-31 20:01 ` Waiman Long
2016-06-01 9:31 ` Peter Zijlstra
2016-06-01 12:00 ` Will Deacon
2016-06-01 12:06 ` Peter Zijlstra
2016-06-01 12:13 ` Will Deacon
2016-06-01 12:45 ` Peter Zijlstra
2016-06-01 14:07 ` Will Deacon
2016-06-01 17:13 ` Peter Zijlstra
2016-06-01 16:53 ` Waiman Long
2016-05-31 9:41 ` [PATCH -v3 8/8] locking, tile: Provide TILE specific smp_acquire__after_ctrl_dep Peter Zijlstra
2016-05-31 15:32 ` Chris Metcalf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160531094844.222530154@infradead.org \
--to=peterz@infradead.org \
--cc=Waiman.Long@hpe.com \
--cc=boqun.feng@gmail.com \
--cc=dave@stgolabs.net \
--cc=davem@davemloft.net \
--cc=hofrat@osadl.org \
--cc=kaber@trash.net \
--cc=linux-kernel@vger.kernel.org \
--cc=manfred@colorfullife.com \
--cc=netfilter-devel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=pablo@netfilter.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=sasha.levin@oracle.com \
--cc=tj@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).