diff mbox series

[SRU,T,2/3] netfilter: nf_conncount: fix garbage collection confirm race

Message ID 20190114185522.10533-3-mfo@canonical.com
State New
Headers show
Series netfilter: nf_conncount: fix for LP#1811094 | expand

Commit Message

Mauricio Faria de Oliveira Jan. 14, 2019, 6:55 p.m. UTC 
From: Florian Westphal <fw@strlen.de>

BugLink: https://bugs.launchpad.net/bugs/1811094

Yi-Hung Wei and Justin Pettit found a race in the garbage collection scheme
used by nf_conncount.

When doing list walk, we lookup the tuple in the conntrack table.
If the lookup fails we remove this tuple from our list because
the conntrack entry is gone.

This is the common cause, but turns out its not the only one.
The list entry could have been created just before by another cpu, i.e. the
conntrack entry might not yet have been inserted into the global hash.

The avoid this, we introduce a timestamp and the owning cpu.
If the entry appears to be stale, evict only if:
 1. The current cpu is the one that added the entry, or,
 2. The timestamp is older than two jiffies

The second constraint allows GC to be taken over by other
cpu too (e.g. because a cpu was offlined or napi got moved to another
cpu).

We can't pretend the 'doubtful' entry wasn't in our list.
Instead, when we don't find an entry indicate via IS_ERR
that entry was removed ('did not exist' or withheld
('might-be-unconfirmed').

This most likely also fixes a xt_connlimit imbalance earlier reported by
Dmitry Andrianov.

Cc: Dmitry Andrianov <dmitry.andrianov@alertme.com>
Reported-by: Justin Pettit <jpettit@vmware.com>
Reported-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Yi-Hung Wei <yihung.wei@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
(backported from commit b36e4523d4d56e2595e28f16f6ccf1cd6a9fc452)
[mfo: backport: refresh context lines and use older symbol/file names:
 - nf_conncount.c -> xt_connlimit.c.
   - nf_conncount_rb -> xt_connlimit_rb
   - nf_conncount_tuple -> xt_connlimit_conn
   - conncount_conn_cachep -> connlimit_conn_cachep]
 - hunk 1:
   - refresh context lines; struct xt_connlimit_conn has the
     'addr' field (and it's used), and doesn't have 'zone'.
 - hunk 2, part 1 (assignments) -> hunk 5:
   - not in its own function (add_hlist()/nf_conncount_add()),
     but in count_them(); refresh context lines/indentation.
 - hunk 2, part 2 (find_or_evict()) -> hunk 2:
   - s/&conn->zone/NF_CT_DEFAULT_ZONE/ as in the removed chunk,
     due to lack of commit e59ea3df3fc2 ("netfilter: xt_connlimit:
     honor conntrack zone if available");
   - s/kmem_cache_free()/kfree()/ as in the removed chunk.
 - hunk 3, part 1 (move line) -> hunk 3:
   - refresh context lines.
 - hunk 3, part 2 (remove/add) -> hunk 4:
   - s/head/hash/ in the hlist_for_each_entry_safe() call due to
     lack of commit 15cfd5289575 ("netfilter: connlimit: factor
     hlist search into new function")]
   - s/&conn->zone/NF_CT_DEFAULT_ZONE/
     due to lack of commit e59ea3df3fc2 ("netfilter: xt_connlimit:
     honor conntrack zone if available");
   - s/kmem_cache_free()/kfree()/
   - s/length/matches/ due ot lack of commit 7d08487777c8 ("netfilter:
     connlimit: use rbtree for per-host conntrack obj storage")
   - remove the zone-related addition lines (thus remove the 'tuple'
     check as well), as zones are not used yet (as mentioned above).]
Signed-off-by: Mauricio Faria de Oliveira <mfo@canonical.com>
---
 net/netfilter/xt_connlimit.c | 45 +++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)
diff mbox series

Patch

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 5d18f39ad69b..df33ca3bc42f 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -36,6 +36,8 @@  struct xt_connlimit_conn {
 	struct hlist_node		node;
 	struct nf_conntrack_tuple	tuple;
 	union nf_inet_addr		addr;
+	int				cpu;
+	u32				jiffies32;
 };
 
 struct xt_connlimit_data {
@@ -92,6 +94,35 @@  same_source_net(const union nf_inet_addr *addr,
 	}
 }
 
+static const struct nf_conntrack_tuple_hash *
+find_or_evict(struct net *net, struct xt_connlimit_conn *conn)
+{
+	const struct nf_conntrack_tuple_hash *found;
+	unsigned long a, b;
+	int cpu = raw_smp_processor_id();
+	__s32 age;
+
+	found = nf_conntrack_find_get(net, NF_CT_DEFAULT_ZONE, &conn->tuple);
+	if (found)
+		return found;
+	b = conn->jiffies32;
+	a = (u32)jiffies;
+
+	/* conn might have been added just before by another cpu and
+	 * might still be unconfirmed.  In this case, nf_conntrack_find()
+	 * returns no result.  Thus only evict if this cpu added the
+	 * stale entry or if the entry is older than two jiffies.
+	 */
+	age = a - b;
+	if (conn->cpu == cpu || age >= 2) {
+		hlist_del(&conn->node);
+		kfree(conn);
+		return ERR_PTR(-ENOENT);
+	}
+
+	return ERR_PTR(-EAGAIN);
+}
+
 static int count_them(struct net *net,
 		      struct xt_connlimit_data *data,
 		      const struct nf_conntrack_tuple *tuple,
@@ -101,8 +132,8 @@  static int count_them(struct net *net,
 {
 	const struct nf_conntrack_tuple_hash *found;
 	struct xt_connlimit_conn *conn;
-	struct hlist_node *n;
 	struct nf_conn *found_ct;
+	struct hlist_node *n;
 	struct hlist_head *hash;
 	bool addit = true;
 	int matches = 0;
@@ -116,11 +147,11 @@  static int count_them(struct net *net,
 
 	/* check the saved connections */
 	hlist_for_each_entry_safe(conn, n, hash, node) {
-		found    = nf_conntrack_find_get(net, NF_CT_DEFAULT_ZONE,
-						 &conn->tuple);
-		if (found == NULL) {
-			hlist_del(&conn->node);
-			kfree(conn);
+		found = find_or_evict(net, conn);
+		if (IS_ERR(found)) {
+			/* Not found, but might be about to be confirmed */
+			if (PTR_ERR(found) == -EAGAIN)
+				matches++;
 			continue;
 		}
 
@@ -159,6 +190,8 @@  static int count_them(struct net *net,
 			return -ENOMEM;
 		conn->tuple = *tuple;
 		conn->addr = *addr;
+		conn->cpu = raw_smp_processor_id();
+		conn->jiffies32 = (u32)jiffies;
 		hlist_add_head(&conn->node, hash);
 		++matches;
 	}