[PATCH 4.14 0/4] netfilter: xt_connlimit: backport upstream fixes for race in connection counting

* [PATCH 4.14 0/4] netfilter: xt_connlimit: backport upstream fixes for race in connection counting
@ 2019-01-02 17:00 Mauricio Faria de Oliveira
  2019-01-02 17:00 ` [PATCH 4.14 1/4] netfilter: xt_connlimit: don't store address in the conn nodes Mauricio Faria de Oliveira
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Mauricio Faria de Oliveira @ 2019-01-02 17:00 UTC (permalink / raw)
  To: stable, netdev, Florian Westphal
  Cc: Alakesh Haloi, nivedita.singhvi, Pablo Neira Ayuso,
	Jozsef Kadlecsik, David S. Miller, Yi-Hung Wei

Recently, Alakesh Haloi reported the following issue [1] with stable/4.14:

  """
  An iptable rule like the following on a multicore systems will result in
  accepting more connections than set in the rule.

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

And proposed a fix that is not in Linus's tree. The discussion went on to
confirm whether the issue was still reproducible with mainline/nf.git tip,
and to either identify the upstream fix or re-submit the non-upstream fix.

Alakesh eventually was able to test with upstream, and reported that issue
was still reproducible [2].
On that, our findinds diverge, at least in my test environment:

First, I verified that the suggested mainline fix for the issue [3] indeed
fixes it, by testing with it applied and reverted on v4.18, a clean revert.
(The issue is reproducible with the commit reverted).

Then, with a consistent reproducer, I moved to nf.git, with HEAD on commit
a007232 ("netfilter: nf_conncount: fix argument order to find_next_bit"),
and the issues was not reproducible (even with 20+ threads on client side,
the number Alakesh reported to achieve 2150+ connections [4], and I tried
spreading the network interface IRQ affinity over more and more CPUs too.)

Either way, the suggested mainline fix does actually fix the issue in 4.14
for at least one environment. So, it might well be the case that Alakesh's
test environment has differences/subtleties that leads to more connections
accepted, and more commits are needed for that particular environment type.

But for now, with one bare-metal environment (24-core server, 4-core client)
verified, I thought of submitting the patches for review/comments/testing,
then looking for additional fixes for that environment separately.

The fix is PATCH 4/4, and PATCHes 1-3/4 are helpers for a cleaner backport.
All backports are simple, and essentially consist of refresh context lines
and use older struct/file names.

Reviews from netfilter maintainers are very appreciated, as I've no previous
experience in this area, and although the backports look simple and build/run
correctly, there's usually stuff that only more experienced people may notice.

Thanks,
Mauricio

Links:
=====

  [1] https://www.spinics.net/lists/stable/msg270040.html
  [2] https://www.spinics.net/lists/stable/msg273669.html
  [3] https://www.spinics.net/lists/stable/msg271300.html
  [4] https://www.spinics.net/lists/stable/msg273669.html

Test-case:
=========

 - v4.14.91 (original): client achieves 2000+ connections (6000 target)
                        with 3 threads.

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

    server # iptables -L
    Chain INPUT (policy ACCEPT)
    target     prot opt source               destination         
    DROP       tcp  --  anywhere             anywhere             tcp dpt:7777 flags:FIN,SYN,RST,ACK/SYN #conn src/0 > 2000

    Chain FORWARD (policy ACCEPT)
    target     prot opt source               destination         

    Chain OUTPUT (policy ACCEPT)
    target     prot opt source               destination         

    server # ulimit -SHn 65000
    server # ruby server.rb
    <... listening ...>

    client # ulimit -SHn 65000
    client # ruby client.rb 10.230.56.100 7777 6000 3
    Connecting to ["10.230.56.100"]:7777 6000 times with 3
    1
    2
    3
    <...>
    2000
    <...>
    6000
    Target reached. Thread finishing
    6001
    Target reached. Thread finishing
    6002
    Target reached. Thread finishing
    Threads done. 6002 connections
    press enter to exit

 - v4.14.91 + patches: client only achieved 2000 connections.

    server #  (same procedure)

    client #  (same procedure)

    Connecting to ["10.230.56.100"]:7777 6000 times with 3
    1
    2
    3
    <...>
    2000
    <... blocked for a while...>
    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

Florian Westphal (2):
  netfilter: xt_connlimit: don't store address in the conn nodes
  netfilter: nf_conncount: fix garbage collection confirm race

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.7.4

^ permalink raw reply	[flat|nested] 9+ messages in thread