[SRU C, D/Unstable][PATCH 0/1] netfilter: nf_conncount: fix for LP#1811094

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU C, D/Unstable][PATCH 0/1] netfilter: nf_conncount: fix for LP#1811094

Mauricio Faria de Oliveira-3
BugLink: https://bugs.launchpad.net/bugs/1811094

[Note to Cosmic/Disco/Unstable]

Cosmic and later are not severely affected, as the main fix
commit is applied.  However, there's this particular commit
that fixes that fix which the netfilter maintainer required,
and it's part of the Xenial/Bionic patch series, so it must
be applied to later releases as well for consistency.

[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 conncount/connlimit (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 (1):
  netfilter: nf_conncount: don't skip eviction when age is negative

 net/netfilter/nf_conncount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU C, D/Unstable][PATCH 1/1] netfilter: nf_conncount: don't skip eviction when age is negative

Mauricio Faria de Oliveira-3
From: Florian Westphal <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1811094

age is signed integer, so result can be negative when the timestamps
have a large delta.  In this case we want to discard the entry.

Instead of using age >= 2 || age < 0, just make it unsigned.

Fixes: b36e4523d4d56 ("netfilter: nf_conncount: fix garbage collection confirm race")
Reviewed-by: Shawn Bohrer <[hidden email]>
Signed-off-by: Florian Westphal <[hidden email]>
Signed-off-by: Pablo Neira Ayuso <[hidden email]>
(cherry picked from commit 4cd273bb91b3001f623f516ec726c49754571b1a)
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 net/netfilter/nf_conncount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
index 510039862aa9..79d1e17a39d8 100644
--- a/net/netfilter/nf_conncount.c
+++ b/net/netfilter/nf_conncount.c
@@ -106,7 +106,7 @@ find_or_evict(struct net *net, struct nf_conncount_tuple *conn)
  const struct nf_conntrack_tuple_hash *found;
  unsigned long a, b;
  int cpu = raw_smp_processor_id();
- __s32 age;
+ u32 age;
 
  found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
  if (found)
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU C, D/Unstable][PATCH 1/1] netfilter: nf_conncount: don't skip eviction when age is negative

Stefan Bader-2
On 10.01.19 04:45, Mauricio Faria de Oliveira wrote:

> From: Florian Westphal <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> age is signed integer, so result can be negative when the timestamps
> have a large delta.  In this case we want to discard the entry.
>
> Instead of using age >= 2 || age < 0, just make it unsigned.
>
> Fixes: b36e4523d4d56 ("netfilter: nf_conncount: fix garbage collection confirm race")
> Reviewed-by: Shawn Bohrer <[hidden email]>
> Signed-off-by: Florian Westphal <[hidden email]>
> Signed-off-by: Pablo Neira Ayuso <[hidden email]>
> (cherry picked from commit 4cd273bb91b3001f623f516ec726c49754571b1a)
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>

> ---
>  net/netfilter/nf_conncount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
> index 510039862aa9..79d1e17a39d8 100644
> --- a/net/netfilter/nf_conncount.c
> +++ b/net/netfilter/nf_conncount.c
> @@ -106,7 +106,7 @@ find_or_evict(struct net *net, struct nf_conncount_tuple *conn)
>   const struct nf_conntrack_tuple_hash *found;
>   unsigned long a, b;
>   int cpu = raw_smp_processor_id();
> - __s32 age;
> + u32 age;
>  
>   found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
>   if (found)
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU C, D/Unstable][PATCH 1/1] netfilter: nf_conncount: don't skip eviction when age is negative

Kleber Souza
In reply to this post by Mauricio Faria de Oliveira-3
On 1/10/19 4:45 AM, Mauricio Faria de Oliveira wrote:

> From: Florian Westphal <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> age is signed integer, so result can be negative when the timestamps
> have a large delta.  In this case we want to discard the entry.
>
> Instead of using age >= 2 || age < 0, just make it unsigned.
>
> Fixes: b36e4523d4d56 ("netfilter: nf_conncount: fix garbage collection confirm race")
> Reviewed-by: Shawn Bohrer <[hidden email]>
> Signed-off-by: Florian Westphal <[hidden email]>
> Signed-off-by: Pablo Neira Ayuso <[hidden email]>
> (cherry picked from commit 4cd273bb91b3001f623f516ec726c49754571b1a)
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>


> ---
>  net/netfilter/nf_conncount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/netfilter/nf_conncount.c b/net/netfilter/nf_conncount.c
> index 510039862aa9..79d1e17a39d8 100644
> --- a/net/netfilter/nf_conncount.c
> +++ b/net/netfilter/nf_conncount.c
> @@ -106,7 +106,7 @@ find_or_evict(struct net *net, struct nf_conncount_tuple *conn)
>   const struct nf_conntrack_tuple_hash *found;
>   unsigned long a, b;
>   int cpu = raw_smp_processor_id();
> - __s32 age;
> + u32 age;
>  
>   found = nf_conntrack_find_get(net, &conn->zone, &conn->tuple);
>   if (found)



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[C]: [SRU C, D/Unstable][PATCH 0/1] netfilter: nf_conncount: fix for LP#1811094

Kleber Souza
In reply to this post by Mauricio Faria de Oliveira-3
On 1/10/19 4:45 AM, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> [Note to Cosmic/Disco/Unstable]
>
> Cosmic and later are not severely affected, as the main fix
> commit is applied.  However, there's this particular commit
> that fixes that fix which the netfilter maintainer required,
> and it's part of the Xenial/Bionic patch series, so it must
> be applied to later releases as well for consistency.
>
> [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 conncount/connlimit (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 (1):
>   netfilter: nf_conncount: don't skip eviction when age is negative
>
>  net/netfilter/nf_conncount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Applied to cosmic/master-next branch.

Thanks,
Kleber


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[D/Unstable]: [SRU C, D/Unstable][PATCH 0/1] netfilter: nf_conncount: fix for LP#1811094

Seth Forshee
In reply to this post by Mauricio Faria de Oliveira-3
On Thu, Jan 10, 2019 at 01:45:38AM -0200, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1811094
>
> [Note to Cosmic/Disco/Unstable]
>
> Cosmic and later are not severely affected, as the main fix
> commit is applied.  However, there's this particular commit
> that fixes that fix which the netfilter maintainer required,
> and it's part of the Xenial/Bionic patch series, so it must
> be applied to later releases as well for consistency.
>
> [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 conncount/connlimit (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

Applied to disco/master-next and unstable/master, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team