From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sasha Levin Subject: Re: [PATCH v2 4.14 0/5] netfilter: xt_connlimit: backport upstream fixes for race in connection counting Date: Sun, 6 Jan 2019 12:56:10 -0500 Message-ID: <20190106175610.GE166797@sasha-vm> References: <20190102204204.12389-1-mfo@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Cc: stable@vger.kernel.org, netdev@vger.kernel.org, Florian Westphal , Alakesh Haloi , nivedita.singhvi@canonical.com, Pablo Neira Ayuso , Jozsef Kadlecsik , "David S. Miller" , Yi-Hung Wei To: Mauricio Faria de Oliveira Return-path: Content-Disposition: inline In-Reply-To: <20190102204204.12389-1-mfo@canonical.com> Sender: stable-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Wed, Jan 02, 2019 at 06:41:59PM -0200, Mauricio Faria de Oliveira wrote: >From: Ubuntu > >[changelog] > - v2: include patch 5/5 (a very recent fix to patch 4/5) which is > not yet in Linus's tree but it's in nf.git + linux-next.git, > thus should make it shortly. Test results still consistent. > Thanks Florian Westphal for reviewing and pointing that out. > >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. > >(v2 update: see Florian's reply to v1 thread [1]; these different results > are probably explained by very recent fixes still missing back then.) > >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 (needs fix in PATCH 5), and PATCHes 1-3 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. Queued for 4.14, thank you.