| Message ID | 20190110033204.31413-1-mfo@canonical.com |
|---|---|
| Headers | show |
| Series | netfilter: nf_conncount: fix for LP#1811094 | expand |
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 > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
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 >
Hi Stefan, Thanks for reviewing. On Thu, Jan 10, 2019 at 8:25 AM Stefan Bader <stefan.bader@canonical.com> 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!
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 <stefan.bader@canonical.com> 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 <stefan.bader@canonical.com> > > Hope this helps. > Thanks! > > > >
On Thu, Jan 10, 2019 at 12:54 PM Stefan Bader <stefan.bader@canonical.com> 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 <stefan.bader@canonical.com> > > > > Hope this helps. > > Thanks! > > > > > > > > > >
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
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