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 |
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
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 --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)