ACK: [SRU T][PATCH 0/3] netfilter: nf_conncount: fix for LP#1811094

[Mauricio Faria de Oliveira]

On Tue, Jan 29, 2019 at 2:36 AM Khaled Elmously
<khalid.elmously at 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 at canonical.com>

Thanks!



--
Mauricio Faria de Oliveira