| Message ID | 20190114185522.10533-1-mfo@canonical.com |
|---|---|
| Headers | show |
| Series | netfilter: nf_conncount: fix for LP#1811094 | expand |
On 14.01.19 19:55, Mauricio Faria de Oliveira wrote: > BugLink: https://bugs.launchpad.net/bugs/1811094 > > [Notes for Trusty] > > The backport for Trusty differs in some ways than for Xenial/Bionic. > The delta is large enough that deps for a "cleaner" backport become > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > functions that don't yet exist in Trusty, so aren't needed here. > > Patch 1 helps with that a bit, but it's actually needed to move the > non-NULL checks down, so that IS_ERR() can be used before those. > > Patch 2 is the fix per se, and has the most changes, all described > in the backport/provenance section, but those seem straightforward. > > Patch 3 is a fix for that, trivial change. > > [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 that address the race condition (plus one fix). > > 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 changes are limited to netfilter connlimit/conncount (names > change between older/newer kernel versions). > > Florian Westphal (3): > netfilter: connlimit: improve packet-to-closed-connection logic > netfilter: nf_conncount: fix garbage collection confirm race > netfilter: nf_conncount: don't skip eviction when age is negative > > net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 18 deletions(-) > Somewhat not simple to review but I think the spirit of the changes look ok. And it changes a very specific function which can be verified. Wondering whether the security might be interested in that code to add it to the QRT suite. Acked-by: Stefan Bader <stefan.bader@canonical.com>
On Mon, Jan 14, 2019 at 4:56 PM Mauricio Faria de Oliveira <mfo@canonical.com> wrote: > > BugLink: https://bugs.launchpad.net/bugs/1811094 > > [Notes for Trusty] > > The backport for Trusty differs in some ways than for Xenial/Bionic. > The delta is large enough that deps for a "cleaner" backport become > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > functions that don't yet exist in Trusty, so aren't needed here. > > Patch 1 helps with that a bit, but it's actually needed to move the > non-NULL checks down, so that IS_ERR() can be used before those. > > Patch 2 is the fix per se, and has the most changes, all described > in the backport/provenance section, but those seem straightforward. > > Patch 3 is a fix for that, trivial change. [snip] Hello kernel team, This series has one ACK so far (from Stefan Bader on Jan 15). Any chance of another reviewer to ack/nack for this cycle? Thanks in advance, -- Mauricio Faria de Oliveira
On 2019-01-28 15:50:27 , Mauricio Faria de Oliveira wrote: > On Mon, Jan 14, 2019 at 4:56 PM Mauricio Faria de Oliveira > <mfo@canonical.com> wrote: > > > > BugLink: https://bugs.launchpad.net/bugs/1811094 > > > > [Notes for Trusty] > > > > The backport for Trusty differs in some ways than for Xenial/Bionic. > > The delta is large enough that deps for a "cleaner" backport become > > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > > functions that don't yet exist in Trusty, so aren't needed here. > > > > Patch 1 helps with that a bit, but it's actually needed to move the > > non-NULL checks down, so that IS_ERR() can be used before those. > > > > Patch 2 is the fix per se, and has the most changes, all described > > in the backport/provenance section, but those seem straightforward. > > > > Patch 3 is a fix for that, trivial change. > [snip] > > Hello kernel team, > > This series has one ACK so far (from Stefan Bader on Jan 15). > Any chance of another reviewer to ack/nack for this cycle? > > Thanks in advance, > > Hey, sorry I reviewed the equivalent changes for X and B but forgot this one. I will review it shortly and apply it to Trusty if all is good. Khaled > -- > Mauricio Faria de Oliveira > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Hi Khaled, On Mon, Jan 28, 2019 at 4:41 PM Khaled Elmously <khalid.elmously@canonical.com> wrote: > > On 2019-01-28 15:50:27 , Mauricio Faria de Oliveira wrote: > > On Mon, Jan 14, 2019 at 4:56 PM Mauricio Faria de Oliveira > > <mfo@canonical.com> wrote: > > > > > > BugLink: https://bugs.launchpad.net/bugs/1811094 [snip] > > > > Hello kernel team, > > > > This series has one ACK so far (from Stefan Bader on Jan 15). > > Any chance of another reviewer to ack/nack for this cycle? > > > > Thanks in advance, > > > > > > Hey, sorry I reviewed the equivalent changes for X and B but forgot this one. I will review it shortly and apply it to Trusty if all is good. No worries, thanks a bunch! > Khaled > > > -- > > Mauricio Faria de Oliveira > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On 2019-01-14 16:55:19 , Mauricio Faria de Oliveira wrote: > BugLink: https://bugs.launchpad.net/bugs/1811094 > > [Notes for Trusty] > > The backport for Trusty differs in some ways than for Xenial/Bionic. > The delta is large enough that deps for a "cleaner" backport become > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > functions that don't yet exist in Trusty, so aren't needed here. > > Patch 1 helps with that a bit, but it's actually needed to move the > non-NULL checks down, so that IS_ERR() can be used before those. > > Patch 2 is the fix per se, and has the most changes, all described > in the backport/provenance section, but those seem straightforward. > > Patch 3 is a fix for that, trivial change. > > [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 that address the race condition (plus one fix). > > 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 changes are limited to netfilter connlimit/conncount (names > change between older/newer kernel versions). > > Florian Westphal (3): > netfilter: connlimit: improve packet-to-closed-connection logic > netfilter: nf_conncount: fix garbage collection confirm race > netfilter: nf_conncount: don't skip eviction when age is negative > > net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 18 deletions(-) > Thanks for the detailed 'backport' descriptions Acked-by: Khalid Elmously <khalid.elmously@canonical.com>
Applied to Trusty - but this bug isn't nominated for Trusty On 2019-01-14 16:55:19 , Mauricio Faria de Oliveira wrote: > BugLink: https://bugs.launchpad.net/bugs/1811094 > > [Notes for Trusty] > > The backport for Trusty differs in some ways than for Xenial/Bionic. > The delta is large enough that deps for a "cleaner" backport become > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > functions that don't yet exist in Trusty, so aren't needed here. > > Patch 1 helps with that a bit, but it's actually needed to move the > non-NULL checks down, so that IS_ERR() can be used before those. > > Patch 2 is the fix per se, and has the most changes, all described > in the backport/provenance section, but those seem straightforward. > > Patch 3 is a fix for that, trivial change. > > [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 that address the race condition (plus one fix). > > 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 changes are limited to netfilter connlimit/conncount (names > change between older/newer kernel versions). > > Florian Westphal (3): > netfilter: connlimit: improve packet-to-closed-connection logic > netfilter: nf_conncount: fix garbage collection confirm race > netfilter: nf_conncount: don't skip eviction when age is negative > > net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 18 deletions(-) > > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Tue, Jan 29, 2019 at 2:36 AM Khaled Elmously <khalid.elmously@canonical.com> wrote: > > > On 2019-01-14 16:55:19 , Mauricio Faria de Oliveira wrote: > > BugLink: https://bugs.launchpad.net/bugs/1811094 > > > > [Notes for Trusty] > > > > The backport for Trusty differs in some ways than for Xenial/Bionic. > > The delta is large enough that deps for a "cleaner" backport become > > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > > functions that don't yet exist in Trusty, so aren't needed here. > > > > Patch 1 helps with that a bit, but it's actually needed to move the > > non-NULL checks down, so that IS_ERR() can be used before those. > > > > Patch 2 is the fix per se, and has the most changes, all described > > in the backport/provenance section, but those seem straightforward. > > > > Patch 3 is a fix for that, trivial change. > > > > [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 that address the race condition (plus one fix). > > > > 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 changes are limited to netfilter connlimit/conncount (names > > change between older/newer kernel versions). > > > > Florian Westphal (3): > > netfilter: connlimit: improve packet-to-closed-connection logic > > netfilter: nf_conncount: fix garbage collection confirm race > > netfilter: nf_conncount: don't skip eviction when age is negative > > > > net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- > > 1 file changed, 44 insertions(+), 18 deletions(-) > > > > Thanks for the detailed 'backport' descriptions Glad it's actually been useful. :-) > Acked-by: Khalid Elmously <khalid.elmously@canonical.com> Thanks! -- Mauricio Faria de Oliveira
On Tue, Jan 29, 2019 at 4:40 AM Khaled Elmously <khalid.elmously@canonical.com> wrote: > > Applied to Trusty - but this bug isn't nominated for Trusty Indeed, Trusty was requested later on. It's fixed/nominated now. > > On 2019-01-14 16:55:19 , Mauricio Faria de Oliveira wrote: > > BugLink: https://bugs.launchpad.net/bugs/1811094 > > > > [Notes for Trusty] > > > > The backport for Trusty differs in some ways than for Xenial/Bionic. > > The delta is large enough that deps for a "cleaner" backport become > > a lot, and intrusive. That cleaning in Xenial/Bionic happened into > > functions that don't yet exist in Trusty, so aren't needed here. > > > > Patch 1 helps with that a bit, but it's actually needed to move the > > non-NULL checks down, so that IS_ERR() can be used before those. > > > > Patch 2 is the fix per se, and has the most changes, all described > > in the backport/provenance section, but those seem straightforward. > > > > Patch 3 is a fix for that, trivial change. > > > > [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 that address the race condition (plus one fix). > > > > 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 changes are limited to netfilter connlimit/conncount (names > > change between older/newer kernel versions). > > > > Florian Westphal (3): > > netfilter: connlimit: improve packet-to-closed-connection logic > > netfilter: nf_conncount: fix garbage collection confirm race > > netfilter: nf_conncount: don't skip eviction when age is negative > > > > net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- > > 1 file changed, 44 insertions(+), 18 deletions(-) > > > > -- > > 2.17.1 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
BugLink: https://bugs.launchpad.net/bugs/1811094 [Notes for Trusty] The backport for Trusty differs in some ways than for Xenial/Bionic. The delta is large enough that deps for a "cleaner" backport become a lot, and intrusive. That cleaning in Xenial/Bionic happened into functions that don't yet exist in Trusty, so aren't needed here. Patch 1 helps with that a bit, but it's actually needed to move the non-NULL checks down, so that IS_ERR() can be used before those. Patch 2 is the fix per se, and has the most changes, all described in the backport/provenance section, but those seem straightforward. Patch 3 is a fix for that, trivial change. [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 that address the race condition (plus one fix). 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 changes are limited to netfilter connlimit/conncount (names change between older/newer kernel versions). Florian Westphal (3): netfilter: connlimit: improve packet-to-closed-connection logic netfilter: nf_conncount: fix garbage collection confirm race netfilter: nf_conncount: don't skip eviction when age is negative net/netfilter/xt_connlimit.c | 62 +++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 18 deletions(-)