diff mbox series

[1/1] netfilter: Fix handling simultaneous open in TCP conntrack

Message ID 1524311028-22603-2-git-send-email-kadlec@blackhole.kfki.hu
State Accepted
Delegated to: Pablo Neira
Headers show
Series TCP conntrack patch to handle simultaneous open | expand

Commit Message

Jozsef Kadlecsik April 21, 2018, 11:43 a.m. UTC
Dominique Martinet reported a TCP hang problem when simultaneous open was used.
The problem is that the tcp_conntracks state table is not smart enough
to handle the case. The state table could be fixed by introducing a new state,
but that would require more lines of code compared to this patch, due to the
required backward compatibility with ctnetlink.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
Reported-by: Dominique Martinet <asmadeus@codewreck.org>
Tested-by: Dominique Martinet <asmadeus@codewreck.org>
---
 include/uapi/linux/netfilter/nf_conntrack_tcp.h |  3 +++
 net/netfilter/nf_conntrack_proto_tcp.c          | 11 +++++++++++
 2 files changed, 14 insertions(+)

Comments

Pablo Neira Ayuso April 26, 2018, 10:38 p.m. UTC | #1
On Sat, Apr 21, 2018 at 01:43:48PM +0200, Jozsef Kadlecsik wrote:
> Dominique Martinet reported a TCP hang problem when simultaneous open was used.
> The problem is that the tcp_conntracks state table is not smart enough
> to handle the case. The state table could be fixed by introducing a new state,
> but that would require more lines of code compared to this patch, due to the
> required backward compatibility with ctnetlink.

Applied, thanks Jozsef.

BTW, what is exactly the problem in ctnetlink. I think probably there
is a way to do some mapping to avoid this. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jozsef Kadlecsik May 4, 2018, 11:54 a.m. UTC | #2
Hi Pablo,

[Sorry for the delay.]

On Fri, 27 Apr 2018, Pablo Neira Ayuso wrote:

> On Sat, Apr 21, 2018 at 01:43:48PM +0200, Jozsef Kadlecsik wrote:
> > Dominique Martinet reported a TCP hang problem when simultaneous open 
> > was used. The problem is that the tcp_conntracks state table is not 
> > smart enough to handle the case. The state table could be fixed by 
> > introducing a new state, but that would require more lines of code 
> > compared to this patch, due to the required backward compatibility 
> > with ctnetlink.
> 
> BTW, what is exactly the problem in ctnetlink. I think probably there is 
> a way to do some mapping to avoid this. Thanks!

There's nothing wrong with ctnetlink, I was too terse.

If a new state is introduced, then there'd be a hole in several internal 
tables (tcp_conntrack_names, tcp_timeouts, tcp_conntracks state table) and 
that'd be ugly. However if the states are renumbered in order to get rid 
of the holes, then that'd broke the backward compatibility in ctnetlink - 
and userspace anyway, because the constants are exposed through 
uapi/linux/netfilter/nf_conntrack_tcp.h. Or some mapping could be used as 
you suggest but that seems to be overkill compared to the few lines of 
code in the patch.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
          H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/include/uapi/linux/netfilter/nf_conntrack_tcp.h b/include/uapi/linux/netfilter/nf_conntrack_tcp.h
index 74b9115..bcba72d 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_tcp.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_tcp.h
@@ -46,6 +46,9 @@  enum tcp_conntrack {
 /* Marks possibility for expected RFC5961 challenge ACK */
 #define IP_CT_EXP_CHALLENGE_ACK 		0x40
 
+/* Simultaneous open initialized */
+#define IP_CT_TCP_SIMULTANEOUS_OPEN		0x80
+
 struct nf_ct_tcp_flags {
 	__u8 flags;
 	__u8 mask;
diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index e97cdc1..8e67910 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -981,6 +981,17 @@  static int tcp_packet(struct nf_conn *ct,
 			return NF_ACCEPT; /* Don't change state */
 		}
 		break;
+	case TCP_CONNTRACK_SYN_SENT2:
+		/* tcp_conntracks table is not smart enough to handle
+		 * simultaneous open.
+		 */
+		ct->proto.tcp.last_flags |= IP_CT_TCP_SIMULTANEOUS_OPEN;
+		break;
+	case TCP_CONNTRACK_SYN_RECV:
+		if (dir == IP_CT_DIR_REPLY && index == TCP_ACK_SET &&
+		    ct->proto.tcp.last_flags & IP_CT_TCP_SIMULTANEOUS_OPEN)
+			new_state = TCP_CONNTRACK_ESTABLISHED;
+		break;
 	case TCP_CONNTRACK_CLOSE:
 		if (index == TCP_RST_SET
 		    && (ct->proto.tcp.seen[!dir].flags & IP_CT_TCP_FLAG_MAXACK_SET)