[SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Mauricio Faria de Oliveira-3
BugLink: https://bugs.launchpad.net/bugs/1811094

[Impact]

 * The iptables connection count/limit rules can be breached
   with multithreaded network driver/server/client (common)
   due to a race in the conncount/connlimit code.

 * For example:

   # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
     -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
     -j DROP

 * The fix is a backport from an upstream commit that resolves
   the problem (plus dependencies for a cleaner backport) that
   address the race condition:

   commit b36e4523d4d5 ("netfilter: nf_conncount: fix garbage
   collection confirm race").

[Test Case]

 * Server-side: (relevant kernel side)
   (limit TCP port 7777 to only 2000 connections)

   # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
     -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
     -j DROP

   # ulimit -SHn 65000 # increase number of open files
   # ruby server.rb # multi-threaded server

 * Client-side:

   # ulimit -SHn 65000
   # ruby client.rb <server ip> <port> <target # connections> <# threads>
   <test output>

 * Results with Original kernel:
   (client achieves target of 6000 connections > limit of 2000 connections)

   # ruby client.rb 10.230.56.100 7777 6000 3
   1
   2
   3
   <...>
   6000
   Target reached. Thread finishing
   6001
   Target reached. Thread finishing
   6002
   Target reached. Thread finishing
   Threads done. 6002 connections
   press enter to exit

 * Results with Modified kernel:
   (client is limited to 2000 connections, and times out afterward)

   # ruby client.rb 10.230.56.100 7777 6000 3
   1
   2
   3
   <...>
   2000
   <... blocks for a few minutes ...>
   failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
   failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
   failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
   Threads done. 2000 connections
   press enter to exit

 * Test cases possibly available upon request,
   depending on original author's permission.

[Regression Potential]

 * The patchset has been reviewed by a netfilter maintainer [1] in
   stable mailing list, and was considered OK for 4.14, and that's
   essentially the same backport for 4.15 and 4.4.

 * The changes are limited to netfilter connlimit/conncount (names
   change between older/newer kernel versions).

[Other Info]

 * The backport for 4.14 [2] is applied as of 4.14.92.

[1] https://www.spinics.net/lists/stable/msg276883.html
[2] https://www.spinics.net/lists/stable/msg276910.html

Florian Westphal (3):
  netfilter: xt_connlimit: don't store address in the conn nodes
  netfilter: nf_conncount: fix garbage collection confirm race
  netfilter: nf_conncount: don't skip eviction when age is negative

Mauricio Faria de Oliveira (1):
  UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
    add_hlist()

Pablo Neira Ayuso (1):
  netfilter: nf_conncount: expose connection list interface

Yi-Hung Wei (1):
  netfilter: nf_conncount: Fix garbage collection with zones

 include/net/netfilter/nf_conntrack_count.h | 15 ++++
 net/netfilter/xt_connlimit.c               | 99 +++++++++++++++++-----
 2 files changed, 91 insertions(+), 23 deletions(-)
 create mode 100644 include/net/netfilter/nf_conntrack_count.h

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 1/6] netfilter: xt_connlimit: don't store address in the conn nodes

Mauricio Faria de Oliveira-3
From: Florian Westphal <[hidden email]>

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

Only stored, never read.  This is a leftover from commit 7d08487777c8
("netfilter: connlimit: use rbtree for per-host conntrack obj storage"),
which added the rbtree node struct that stores the address instead.

Signed-off-by: Florian Westphal <[hidden email]>
Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(cherry picked from commit ce49480dba8666cba0106e8e31a942c9ce4c438a)
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 net/netfilter/xt_connlimit.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 99bbc829868d..1152a4bb7b9b 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -46,7 +46,6 @@
 struct xt_connlimit_conn {
  struct hlist_node node;
  struct nf_conntrack_tuple tuple;
- union nf_inet_addr addr;
 };
 
 struct xt_connlimit_rb {
@@ -126,7 +125,6 @@ static bool add_hlist(struct hlist_head *head,
  if (conn == NULL)
  return false;
  conn->tuple = *tuple;
- conn->addr = *addr;
  hlist_add_head(&conn->node, head);
  return true;
 }
@@ -274,7 +272,6 @@ count_tree(struct net *net, struct rb_root *root,
  }
 
  conn->tuple = *tuple;
- conn->addr = *addr;
  rbconn->addr = *addr;
 
  INIT_HLIST_HEAD(&rbconn->hhead);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 2/6] UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in add_hlist()

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
BugLink: https://bugs.launchpad.net/bugs/1811094

The previous commit made the 'addr' parameter in add_hlist() unused.  So remove it
with a SAUCE patch, to simplify the backport of the next patches, as it is removed
anyway in upstream later (but before the next patches) through commit 625c556118f3
("netfilter: connlimit: split xt_connlimit into front and backend"), in the rename
from 'xt_connlimit.c' to 'nf_conncount.c', which is a large refactor we don't need.

Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 net/netfilter/xt_connlimit.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 1152a4bb7b9b..79c3156e12b6 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -116,8 +116,7 @@ same_source_net(const union nf_inet_addr *addr,
 }
 
 static bool add_hlist(struct hlist_head *head,
-      const struct nf_conntrack_tuple *tuple,
-      const union nf_inet_addr *addr)
+      const struct nf_conntrack_tuple *tuple)
 {
  struct xt_connlimit_conn *conn;
 
@@ -233,7 +232,7 @@ count_tree(struct net *net, struct rb_root *root,
  if (!addit)
  return count;
 
- if (!add_hlist(&rbconn->hhead, tuple, addr))
+ if (!add_hlist(&rbconn->hhead, tuple))
  return 0; /* hotdrop */
 
  return count + 1;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 3/6] netfilter: nf_conncount: expose connection list interface

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: Pablo Neira Ayuso <[hidden email]>

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

This patch provides an interface to maintain the list of connections and
the lookup function to obtain the number of connections in the list.

Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(backported from commit 5e5cbc7b23eaf13e18652c03efbad5be6995de6a)
[mfo: backport: refresh context lines and use older symbol/file names:
 - nf_conntrack_count.h: new file, add include guards.
 - nf_conncount.c -> xt_connlimit.c.
   - nf_conncount_rb -> xt_connlimit_rb
   - nf_conncount_tuple -> xt_connlimit_conn
   - conncount_rb_cachep -> connlimit_rb_cachep
   - conncount_conn_cachep -> connlimit_conn_cachep]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 include/net/netfilter/nf_conntrack_count.h | 14 +++++++++
 net/netfilter/xt_connlimit.c               | 36 ++++++++++++++--------
 2 files changed, 37 insertions(+), 13 deletions(-)
 create mode 100644 include/net/netfilter/nf_conntrack_count.h

diff --git a/include/net/netfilter/nf_conntrack_count.h b/include/net/netfilter/nf_conntrack_count.h
new file mode 100644
index 000000000000..54e43b8a8da1
--- /dev/null
+++ b/include/net/netfilter/nf_conntrack_count.h
@@ -0,0 +1,14 @@
+#ifndef _NF_CONNTRACK_COUNT_H
+#define _NF_CONNTRACK_COUNT_H
+
+unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
+ const struct nf_conntrack_tuple *tuple,
+ const struct nf_conntrack_zone *zone,
+ bool *addit);
+
+bool nf_conncount_add(struct hlist_head *head,
+      const struct nf_conntrack_tuple *tuple);
+
+void nf_conncount_cache_free(struct hlist_head *hhead);
+
+#endif
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index 79c3156e12b6..f45de1a4cb09 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -115,7 +115,7 @@ same_source_net(const union nf_inet_addr *addr,
  }
 }
 
-static bool add_hlist(struct hlist_head *head,
+bool nf_conncount_add(struct hlist_head *head,
       const struct nf_conntrack_tuple *tuple)
 {
  struct xt_connlimit_conn *conn;
@@ -127,12 +127,12 @@ static bool add_hlist(struct hlist_head *head,
  hlist_add_head(&conn->node, head);
  return true;
 }
+EXPORT_SYMBOL_GPL(nf_conncount_add);
 
-static unsigned int check_hlist(struct net *net,
- struct hlist_head *head,
- const struct nf_conntrack_tuple *tuple,
- const struct nf_conntrack_zone *zone,
- bool *addit)
+unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
+ const struct nf_conntrack_tuple *tuple,
+ const struct nf_conntrack_zone *zone,
+ bool *addit)
 {
  const struct nf_conntrack_tuple_hash *found;
  struct xt_connlimit_conn *conn;
@@ -180,6 +180,7 @@ static unsigned int check_hlist(struct net *net,
 
  return length;
 }
+EXPORT_SYMBOL_GPL(nf_conncount_lookup);
 
 static void tree_nodes_free(struct rb_root *root,
     struct xt_connlimit_rb *gc_nodes[],
@@ -226,13 +227,15 @@ count_tree(struct net *net, struct rb_root *root,
  } else {
  /* same source network -> be counted! */
  unsigned int count;
- count = check_hlist(net, &rbconn->hhead, tuple, zone, &addit);
+
+ count = nf_conncount_lookup(net, &rbconn->hhead, tuple,
+    zone, &addit);
 
  tree_nodes_free(root, gc_nodes, gc_count);
  if (!addit)
  return count;
 
- if (!add_hlist(&rbconn->hhead, tuple))
+ if (!nf_conncount_add(&rbconn->hhead, tuple))
  return 0; /* hotdrop */
 
  return count + 1;
@@ -242,7 +245,7 @@ count_tree(struct net *net, struct rb_root *root,
  continue;
 
  /* only used for GC on hhead, retval and 'addit' ignored */
- check_hlist(net, &rbconn->hhead, tuple, zone, &addit);
+ nf_conncount_lookup(net, &rbconn->hhead, tuple, zone, &addit);
  if (hlist_empty(&rbconn->hhead))
  gc_nodes[gc_count++] = rbconn;
  }
@@ -392,11 +395,19 @@ static int connlimit_mt_check(const struct xt_mtchk_param *par)
  return 0;
 }
 
-static void destroy_tree(struct rb_root *r)
+void nf_conncount_cache_free(struct hlist_head *hhead)
 {
  struct xt_connlimit_conn *conn;
- struct xt_connlimit_rb *rbconn;
  struct hlist_node *n;
+
+ hlist_for_each_entry_safe(conn, n, hhead, node)
+ kmem_cache_free(connlimit_conn_cachep, conn);
+}
+EXPORT_SYMBOL_GPL(nf_conncount_cache_free);
+
+static void destroy_tree(struct rb_root *r)
+{
+ struct xt_connlimit_rb *rbconn;
  struct rb_node *node;
 
  while ((node = rb_first(r)) != NULL) {
@@ -404,8 +415,7 @@ static void destroy_tree(struct rb_root *r)
 
  rb_erase(node, r);
 
- hlist_for_each_entry_safe(conn, n, &rbconn->hhead, node)
- kmem_cache_free(connlimit_conn_cachep, conn);
+ nf_conncount_cache_free(&rbconn->hhead);
 
  kmem_cache_free(connlimit_rb_cachep, rbconn);
  }
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 4/6] netfilter: nf_conncount: Fix garbage collection with zones

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: Yi-Hung Wei <[hidden email]>

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

Currently, we use check_hlist() for garbage colleciton. However, we
use the ‘zone’ from the counted entry to query the existence of
existing entries in the hlist. This could be wrong when they are in
different zones, and this patch fixes this issue.

Fixes: e59ea3df3fc2 ("netfilter: xt_connlimit: honor conntrack zone if available")
Signed-off-by: Yi-Hung Wei <[hidden email]>
Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(backported from commit 21ba8847f857028dc83a0f341e16ecc616e34740)
[mfo: backport: refresh context lines and use older symbol/file names, note hunk 5:
 - nf_conncount.c -> xt_connlimit.c
   - nf_conncount_rb -> xt_connlimit_rb
   - nf_conncount_tuple -> xt_connlimit_conn
   - hunk 5: remove check for non-NULL 'tuple', that isn't required as it's introduced
     by upstream commit 35d8deb80 ("netfilter: conncount: Support count only use case")
     which addresses nf_conncount_count() that does not exist yet -- it's introduced by
     upstream commit 625c556118f3 ("netfilter: connlimit: split xt_connlimit into front
     and backend"), a refactor change.
 - nft_connlimit.c -> removed, not used/doesn't exist yet.]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 include/net/netfilter/nf_conntrack_count.h |  3 ++-
 net/netfilter/xt_connlimit.c               | 13 +++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack_count.h b/include/net/netfilter/nf_conntrack_count.h
index 54e43b8a8da1..4b71a2f4c351 100644
--- a/include/net/netfilter/nf_conntrack_count.h
+++ b/include/net/netfilter/nf_conntrack_count.h
@@ -7,7 +7,8 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
  bool *addit);
 
 bool nf_conncount_add(struct hlist_head *head,
-      const struct nf_conntrack_tuple *tuple);
+      const struct nf_conntrack_tuple *tuple,
+      const struct nf_conntrack_zone *zone);
 
 void nf_conncount_cache_free(struct hlist_head *hhead);
 
diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index f45de1a4cb09..eb353c0b33a8 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -46,6 +46,7 @@
 struct xt_connlimit_conn {
  struct hlist_node node;
  struct nf_conntrack_tuple tuple;
+ struct nf_conntrack_zone zone;
 };
 
 struct xt_connlimit_rb {
@@ -116,7 +117,8 @@ same_source_net(const union nf_inet_addr *addr,
 }
 
 bool nf_conncount_add(struct hlist_head *head,
-      const struct nf_conntrack_tuple *tuple)
+      const struct nf_conntrack_tuple *tuple,
+      const struct nf_conntrack_zone *zone)
 {
  struct xt_connlimit_conn *conn;
 
@@ -124,6 +126,7 @@ bool nf_conncount_add(struct hlist_head *head,
  if (conn == NULL)
  return false;
  conn->tuple = *tuple;
+ conn->zone = *zone;
  hlist_add_head(&conn->node, head);
  return true;
 }
@@ -145,7 +148,7 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
 
  /* check the saved connections */
  hlist_for_each_entry_safe(conn, n, head, node) {
- found = nf_conntrack_find_get(net, zone, &conn->tuple);
+ found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
  if (found == NULL) {
  hlist_del(&conn->node);
  kmem_cache_free(connlimit_conn_cachep, conn);
@@ -154,7 +157,8 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
 
  found_ct = nf_ct_tuplehash_to_ctrack(found);
 
- if (nf_ct_tuple_equal(&conn->tuple, tuple)) {
+ if (nf_ct_tuple_equal(&conn->tuple, tuple) &&
+    nf_ct_zone_equal(found_ct, zone, zone->dir)) {
  /*
  * Just to be sure we have it only once in the list.
  * We should not see tuples twice unless someone hooks
@@ -235,7 +239,7 @@ count_tree(struct net *net, struct rb_root *root,
  if (!addit)
  return count;
 
- if (!nf_conncount_add(&rbconn->hhead, tuple))
+ if (!nf_conncount_add(&rbconn->hhead, tuple, zone))
  return 0; /* hotdrop */
 
  return count + 1;
@@ -274,6 +278,7 @@ count_tree(struct net *net, struct rb_root *root,
  }
 
  conn->tuple = *tuple;
+ conn->zone = *zone;
  rbconn->addr = *addr;
 
  INIT_HLIST_HEAD(&rbconn->hhead);
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 5/6] netfilter: nf_conncount: fix garbage collection confirm race

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: Florian Westphal <[hidden email]>

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 <[hidden email]>
Reported-by: Justin Pettit <[hidden email]>
Reported-by: Yi-Hung Wei <[hidden email]>
Signed-off-by: Florian Westphal <[hidden email]>
Acked-by: Yi-Hung Wei <[hidden email]>
Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(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]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 net/netfilter/xt_connlimit.c | 52 ++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index eb353c0b33a8..b64186de28e5 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -47,6 +47,8 @@ struct xt_connlimit_conn {
  struct hlist_node node;
  struct nf_conntrack_tuple tuple;
  struct nf_conntrack_zone zone;
+ int cpu;
+ u32 jiffies32;
 };
 
 struct xt_connlimit_rb {
@@ -127,11 +129,42 @@ bool nf_conncount_add(struct hlist_head *head,
  return false;
  conn->tuple = *tuple;
  conn->zone = *zone;
+ conn->cpu = raw_smp_processor_id();
+ conn->jiffies32 = (u32)jiffies;
  hlist_add_head(&conn->node, head);
  return true;
 }
 EXPORT_SYMBOL_GPL(nf_conncount_add);
 
+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, &conn->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);
+ kmem_cache_free(connlimit_conn_cachep, conn);
+ return ERR_PTR(-ENOENT);
+ }
+
+ return ERR_PTR(-EAGAIN);
+}
+
 unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
  const struct nf_conntrack_tuple *tuple,
  const struct nf_conntrack_zone *zone,
@@ -139,8 +172,8 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
 {
  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;
  unsigned int length = 0;
 
  *addit = true;
@@ -148,10 +181,19 @@ unsigned int nf_conncount_lookup(struct net *net, struct hlist_head *head,
 
  /* check the saved connections */
  hlist_for_each_entry_safe(conn, n, head, node) {
- found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
- if (found == NULL) {
- hlist_del(&conn->node);
- kmem_cache_free(connlimit_conn_cachep, 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) {
+ length++;
+ if (!tuple)
+ continue;
+
+ if (nf_ct_tuple_equal(&conn->tuple, tuple) &&
+    nf_ct_zone_id(&conn->zone, conn->zone.dir) ==
+    nf_ct_zone_id(zone, zone->dir))
+ *addit = false;
+ }
  continue;
  }
 
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 6/6] netfilter: nf_conncount: don't skip eviction when age is negative

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: Florian Westphal <[hidden email]>

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

age is signed integer, so result can be negative when the timestamps
have a large delta.  In this case we want to discard the entry.

Instead of using age >= 2 || age < 0, just make it unsigned.

Fixes: b36e4523d4d56 ("netfilter: nf_conncount: fix garbage collection confirm race")
Reviewed-by: Shawn Bohrer <[hidden email]>
Signed-off-by: Florian Westphal <[hidden email]>
Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(backported from commit 4cd273bb91b3001f623f516ec726c49754571b1a)
[mfo: backport: use older file name, nf_conncount.c -> xt_connlimit.c]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 net/netfilter/xt_connlimit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/xt_connlimit.c b/net/netfilter/xt_connlimit.c
index b64186de28e5..8106bd643aed 100644
--- a/net/netfilter/xt_connlimit.c
+++ b/net/netfilter/xt_connlimit.c
@@ -142,7 +142,7 @@ 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;
+ u32 age;
 
  found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
  if (found)
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Khaled Elmously
In reply to this post by Mauricio Faria de Oliveira-3
On 2019-01-10 01:31:58 , Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> [Impact]
>
>  * The iptables connection count/limit rules can be breached
>    with multithreaded network driver/server/client (common)
>    due to a race in the conncount/connlimit code.
>
>  * For example:
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>  * The fix is a backport from an upstream commit that resolves
>    the problem (plus dependencies for a cleaner backport) that
>    address the race condition:
>
>    commit b36e4523d4d5 ("netfilter: nf_conncount: fix garbage
>    collection confirm race").
>
> [Test Case]
>
>  * Server-side: (relevant kernel side)
>    (limit TCP port 7777 to only 2000 connections)
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>    # ulimit -SHn 65000 # increase number of open files
>    # ruby server.rb # multi-threaded server
>
>  * Client-side:
>
>    # ulimit -SHn 65000
>    # ruby client.rb <server ip> <port> <target # connections> <# threads>
>    <test output>
>
>  * Results with Original kernel:
>    (client achieves target of 6000 connections > limit of 2000 connections)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    6000
>    Target reached. Thread finishing
>    6001
>    Target reached. Thread finishing
>    6002
>    Target reached. Thread finishing
>    Threads done. 6002 connections
>    press enter to exit
>
>  * Results with Modified kernel:
>    (client is limited to 2000 connections, and times out afterward)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    2000
>    <... blocks for a few minutes ...>
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    Threads done. 2000 connections
>    press enter to exit
>
>  * Test cases possibly available upon request,
>    depending on original author's permission.
>
> [Regression Potential]
>
>  * The patchset has been reviewed by a netfilter maintainer [1] in
>    stable mailing list, and was considered OK for 4.14, and that's
>    essentially the same backport for 4.15 and 4.4.
>
>  * The changes are limited to netfilter connlimit/conncount (names
>    change between older/newer kernel versions).
>
> [Other Info]
>
>  * The backport for 4.14 [2] is applied as of 4.14.92.
>
> [1] https://www.spinics.net/lists/stable/msg276883.html
> [2] https://www.spinics.net/lists/stable/msg276910.html
>
> Florian Westphal (3):
>   netfilter: xt_connlimit: don't store address in the conn nodes
>   netfilter: nf_conncount: fix garbage collection confirm race
>   netfilter: nf_conncount: don't skip eviction when age is negative
>
> Mauricio Faria de Oliveira (1):
>   UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
>     add_hlist()
>
> Pablo Neira Ayuso (1):
>   netfilter: nf_conncount: expose connection list interface
>
> Yi-Hung Wei (1):
>   netfilter: nf_conncount: Fix garbage collection with zones
>
>  include/net/netfilter/nf_conntrack_count.h | 15 ++++
>  net/netfilter/xt_connlimit.c               | 99 +++++++++++++++++-----
>  2 files changed, 91 insertions(+), 23 deletions(-)
>  create mode 100644 include/net/netfilter/nf_conntrack_count.h
>
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Stefan Bader-2
In reply to this post by Mauricio Faria de Oliveira-3
On 10.01.19 04:31, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> [Impact]
>
>  * The iptables connection count/limit rules can be breached
>    with multithreaded network driver/server/client (common)
>    due to a race in the conncount/connlimit code.
>
>  * For example:
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>  * The fix is a backport from an upstream commit that resolves
>    the problem (plus dependencies for a cleaner backport) that
>    address the race condition:
>
>    commit b36e4523d4d5 ("netfilter: nf_conncount: fix garbage
>    collection confirm race").
>
> [Test Case]
>
>  * Server-side: (relevant kernel side)
>    (limit TCP port 7777 to only 2000 connections)
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>    # ulimit -SHn 65000 # increase number of open files
>    # ruby server.rb # multi-threaded server
>
>  * Client-side:
>
>    # ulimit -SHn 65000
>    # ruby client.rb <server ip> <port> <target # connections> <# threads>
>    <test output>
>
>  * Results with Original kernel:
>    (client achieves target of 6000 connections > limit of 2000 connections)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    6000
>    Target reached. Thread finishing
>    6001
>    Target reached. Thread finishing
>    6002
>    Target reached. Thread finishing
>    Threads done. 6002 connections
>    press enter to exit
>
>  * Results with Modified kernel:
>    (client is limited to 2000 connections, and times out afterward)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    2000
>    <... blocks for a few minutes ...>
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    Threads done. 2000 connections
>    press enter to exit
>
>  * Test cases possibly available upon request,
>    depending on original author's permission.
>
> [Regression Potential]
>
>  * The patchset has been reviewed by a netfilter maintainer [1] in
>    stable mailing list, and was considered OK for 4.14, and that's
>    essentially the same backport for 4.15 and 4.4.
>
>  * The changes are limited to netfilter connlimit/conncount (names
>    change between older/newer kernel versions).
>
> [Other Info]
>
>  * The backport for 4.14 [2] is applied as of 4.14.92.
>
> [1] https://www.spinics.net/lists/stable/msg276883.html
> [2] https://www.spinics.net/lists/stable/msg276910.html
>
> Florian Westphal (3):
>   netfilter: xt_connlimit: don't store address in the conn nodes
>   netfilter: nf_conncount: fix garbage collection confirm race
>   netfilter: nf_conncount: don't skip eviction when age is negative
>
> Mauricio Faria de Oliveira (1):
>   UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
>     add_hlist()
Just double checking since I do not see this set on 4.4.y right now: you are
positive that this does affect 4.4 the same? If yes, think we could just take
the patches from 4.14.y and try to get those into our Xenial tree. Just in case,
this is an acceptable approach and has been done before (I mean instead of
working with the upstream changes, take those which were applied to a closer
upstream stable). I did a quick test with "netfilter: xt_connlimit: don't store
address in the conn nodes" and it looks like that could just become a

(cherry-picked from commit 5e614e212a6359af78b6034ceb12c56f71d5b423 linux-4.14.y)

>
> Pablo Neira Ayuso (1):
>   netfilter: nf_conncount: expose connection list interface
>
> Yi-Hung Wei (1):
>   netfilter: nf_conncount: Fix garbage collection with zones
>
>  include/net/netfilter/nf_conntrack_count.h | 15 ++++
>  net/netfilter/xt_connlimit.c               | 99 +++++++++++++++++-----
>  2 files changed, 91 insertions(+), 23 deletions(-)
>  create mode 100644 include/net/netfilter/nf_conntrack_count.h
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Mauricio Faria de Oliveira-3
Hi Stefan,

Thanks for reviewing.

On Thu, Jan 10, 2019 at 8:25 AM Stefan Bader <[hidden email]> wrote:
>
> On 10.01.19 04:31, Mauricio Faria de Oliveira wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1811094
[snip]
> > Mauricio Faria de Oliveira (1):
> >   UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
> >     add_hlist()
>
> Just double checking since I do not see this set on 4.4.y right now: you are
> positive that this does affect 4.4 the same? [snip]

Yes, it does affect 4.4 the same. It's actually been originally
reported against a
Xenial 4.4-based kernel, and the reported had an interest in 4.14.y, that's why.
(I realize that sending it to 4.4.y would land it in Xenial 4.4
eventually, but for
timing reasons, I done the SRU first.  I can send it to 4.4.y as well,
if required.)

> [snip] If yes, think we could just take
> the patches from 4.14.y and try to get those into our Xenial tree. Just in case,
> this is an acceptable approach and has been done before (I mean instead of
> working with the upstream changes, take those which were applied to a closer
> upstream stable).

I didn't know it, thanks for mentioning!

> [snip] I did a quick test with "netfilter: xt_connlimit: don't store
> address in the conn nodes" and it looks like that could just become a
>
> (cherry-picked from commit 5e614e212a6359af78b6034ceb12c56f71d5b423 linux-4.14.y)

The reason I didn't do it (i.e., squash this SAUCE in the commit as in
4.14.y) is to have a
more equivalent backport between Xenial and Cosmic, because that patch
is already applied
in Cosmic -- so I wanted to avoid the difference of 1) a backport with
this SAUCE squashed
for Xenial, and 2) SAUCE commit only for Bionic -- so I did the same
SAUCE patch for both
(on top of the cherry-pick in Xenial, and the applied commit in Bionic).

Hope this helps.
Thanks!




--
Mauricio Faria de Oliveira

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Stefan Bader-2
On 10.01.19 14:12, Mauricio Faria de Oliveira wrote:

> Hi Stefan,
>
> Thanks for reviewing.
>
> On Thu, Jan 10, 2019 at 8:25 AM Stefan Bader <[hidden email]> wrote:
>>
>> On 10.01.19 04:31, Mauricio Faria de Oliveira wrote:
>>> BugLink: https://bugs.launchpad.net/bugs/1811094
> [snip]
>>> Mauricio Faria de Oliveira (1):
>>>   UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
>>>     add_hlist()
>>
>> Just double checking since I do not see this set on 4.4.y right now: you are
>> positive that this does affect 4.4 the same? [snip]
>
> Yes, it does affect 4.4 the same. It's actually been originally
> reported against a
> Xenial 4.4-based kernel, and the reported had an interest in 4.14.y, that's why.
> (I realize that sending it to 4.4.y would land it in Xenial 4.4
> eventually, but for
> timing reasons, I done the SRU first.  I can send it to 4.4.y as well,
> if required.)
>
>> [snip] If yes, think we could just take
>> the patches from 4.14.y and try to get those into our Xenial tree. Just in case,
>> this is an acceptable approach and has been done before (I mean instead of
>> working with the upstream changes, take those which were applied to a closer
>> upstream stable).
>
> I didn't know it, thanks for mentioning!
>
>> [snip] I did a quick test with "netfilter: xt_connlimit: don't store
>> address in the conn nodes" and it looks like that could just become a
>>
>> (cherry-picked from commit 5e614e212a6359af78b6034ceb12c56f71d5b423 linux-4.14.y)
>
> The reason I didn't do it (i.e., squash this SAUCE in the commit as in
> 4.14.y) is to have a
> more equivalent backport between Xenial and Cosmic, because that patch
> is already applied
Ah ok. Though actually something that I would really not expect to happen. More
likely to have Xenial and Bionic similar because they are closer in versions
than Xenial and Cosmic. :)

> in Cosmic -- so I wanted to avoid the difference of 1) a backport with
> this SAUCE squashed
> for Xenial, and 2) SAUCE commit only for Bionic -- so I did the same
> SAUCE patch for both
> (on top of the cherry-pick in Xenial, and the applied commit in Bionic).

Since there is a backport in some upstream maintained tree (which got the
maintainers blessing) I probably would have rather gone with pulling that back
into Xenial. But since you already prepared differently and it looks like #5
again needs work between 4.14 and 4.4, lets just go with this.

Acked-by: Stefan Bader <[hidden email]>
>
> Hope this helps.
> Thanks!
>
>
>
>



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Mauricio Faria de Oliveira-3
On Thu, Jan 10, 2019 at 12:54 PM Stefan Bader
<[hidden email]> wrote:
>
> On 10.01.19 14:12, Mauricio Faria de Oliveira wrote:
[snip]
> > The reason I didn't do it (i.e., squash this SAUCE in the commit as in
> > 4.14.y) is to have a
> > more equivalent backport between Xenial and Cosmic, because that patch
> > is already applied
>
> Ah ok. Though actually something that I would really not expect to happen. More
> likely to have Xenial and Bionic similar because they are closer in versions
> than Xenial and Cosmic. :)

Doh. Yes, of course, I meant Bionic all the way! (that patch is in v4.15 :- )
Sorry for the confusion.   Below I managed to mention Bionic correctly.

> > in Cosmic -- so I wanted to avoid the difference of 1) a backport with
> > this SAUCE squashed
> > for Xenial, and 2) SAUCE commit only for Bionic -- so I did the same
> > SAUCE patch for both
> > (on top of the cherry-pick in Xenial, and the applied commit in Bionic).
>
> Since there is a backport in some upstream maintained tree (which got the
> maintainers blessing) I probably would have rather gone with pulling that back
> into Xenial. But since you already prepared differently and it looks like #5
> again needs work between 4.14 and 4.4, lets just go with this.

Ok, got it. I'll use that tip in the future. Thanks.

>
> Acked-by: Stefan Bader <[hidden email]>
> >
> > Hope this helps.
> > Thanks!
> >
> >
> >
> >
>
>


--
Mauricio Faria de Oliveira

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU X][PATCH 0/6] netfilter: nf_conncount: fix for LP#1811094

Kleber Souza
In reply to this post by Mauricio Faria de Oliveira-3
On 1/10/19 4:31 AM, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> [Impact]
>
>  * The iptables connection count/limit rules can be breached
>    with multithreaded network driver/server/client (common)
>    due to a race in the conncount/connlimit code.
>
>  * For example:
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>  * The fix is a backport from an upstream commit that resolves
>    the problem (plus dependencies for a cleaner backport) that
>    address the race condition:
>
>    commit b36e4523d4d5 ("netfilter: nf_conncount: fix garbage
>    collection confirm race").
>
> [Test Case]
>
>  * Server-side: (relevant kernel side)
>    (limit TCP port 7777 to only 2000 connections)
>
>    # iptables -A INPUT -p tcp -m tcp --syn --dport 7777 \
>      -m connlimit --connlimit-above 2000 --connlimit-mask 0 \
>      -j DROP
>
>    # ulimit -SHn 65000 # increase number of open files
>    # ruby server.rb # multi-threaded server
>
>  * Client-side:
>
>    # ulimit -SHn 65000
>    # ruby client.rb <server ip> <port> <target # connections> <# threads>
>    <test output>
>
>  * Results with Original kernel:
>    (client achieves target of 6000 connections > limit of 2000 connections)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    6000
>    Target reached. Thread finishing
>    6001
>    Target reached. Thread finishing
>    6002
>    Target reached. Thread finishing
>    Threads done. 6002 connections
>    press enter to exit
>
>  * Results with Modified kernel:
>    (client is limited to 2000 connections, and times out afterward)
>
>    # ruby client.rb 10.230.56.100 7777 6000 3
>    1
>    2
>    3
>    <...>
>    2000
>    <... blocks for a few minutes ...>
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    failed to create connection: Connection timed out - connect(2) for "10.230.56.100" port 7777
>    Threads done. 2000 connections
>    press enter to exit
>
>  * Test cases possibly available upon request,
>    depending on original author's permission.
>
> [Regression Potential]
>
>  * The patchset has been reviewed by a netfilter maintainer [1] in
>    stable mailing list, and was considered OK for 4.14, and that's
>    essentially the same backport for 4.15 and 4.4.
>
>  * The changes are limited to netfilter connlimit/conncount (names
>    change between older/newer kernel versions).
>
> [Other Info]
>
>  * The backport for 4.14 [2] is applied as of 4.14.92.
>
> [1] https://www.spinics.net/lists/stable/msg276883.html
> [2] https://www.spinics.net/lists/stable/msg276910.html
>
> Florian Westphal (3):
>   netfilter: xt_connlimit: don't store address in the conn nodes
>   netfilter: nf_conncount: fix garbage collection confirm race
>   netfilter: nf_conncount: don't skip eviction when age is negative
>
> Mauricio Faria de Oliveira (1):
>   UBUNTU: SAUCE: netfilter: xt_connlimit: remove the 'addr' parameter in
>     add_hlist()
>
> Pablo Neira Ayuso (1):
>   netfilter: nf_conncount: expose connection list interface
>
> Yi-Hung Wei (1):
>   netfilter: nf_conncount: Fix garbage collection with zones
>
>  include/net/netfilter/nf_conntrack_count.h | 15 ++++
>  net/netfilter/xt_connlimit.c               | 99 +++++++++++++++++-----
>  2 files changed, 91 insertions(+), 23 deletions(-)
>  create mode 100644 include/net/netfilter/nf_conntrack_count.h
>
Applied to xenial/master-next branch.

Thanks,
Kleber


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team