From a867c010ee9183885ee9d3eb76a0005c075c4d2e Mon Sep 17 00:00:00 2001 From: Yi-Hung Wei Date: Fri, 20 Dec 2019 09:51:08 -0800 Subject: [PATCH] conntrack: Fix conntrack new state In connection tracking system, a connection is established if we see packets from both directions. However, in userspace datapath's conntrack, if we send a connection setup packet in one direction twice, it will make the connection to be in established state. This patch fixes the aforementioned issue, and adds a system traffic test for UDP and TCP traffic to avoid regression. Fixes: a489b16854b59 ("conntrack: New userspace connection tracker.") Signed-off-by: Yi-Hung Wei Signed-off-by: William Tu --- lib/conntrack-other.c | 4 +++- lib/conntrack-private.h | 1 + lib/conntrack-tcp.c | 15 ++++++++++----- lib/conntrack.c | 3 +++ tests/system-traffic.at | 41 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+), 6 deletions(-) diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c index 932f2f4ad..de22ef87c 100644 --- a/lib/conntrack-other.c +++ b/lib/conntrack-other.c @@ -47,16 +47,18 @@ other_conn_update(struct conntrack *ct, struct conn *conn_, struct dp_packet *pkt OVS_UNUSED, bool reply, long long now) { struct conn_other *conn = conn_other_cast(conn_); + enum ct_update_res ret = CT_UPDATE_VALID; if (reply && conn->state != OTHERS_BIDIR) { conn->state = OTHERS_BIDIR; } else if (conn->state == OTHERS_FIRST) { conn->state = OTHERS_MULTIPLE; + ret = CT_UPDATE_VALID_NEW; } conn_update_expiration(ct, &conn->up, other_timeouts[conn->state], now); - return CT_UPDATE_VALID; + return ret; } static bool diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h index b04e4cd77..9a8ca3910 100644 --- a/lib/conntrack-private.h +++ b/lib/conntrack-private.h @@ -124,6 +124,7 @@ enum ct_update_res { CT_UPDATE_INVALID, CT_UPDATE_VALID, CT_UPDATE_NEW, + CT_UPDATE_VALID_NEW, }; /* Timeouts: all the possible timeout states passed to update_expiration() diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c index 47eb8e203..416cb769d 100644 --- a/lib/conntrack-tcp.c +++ b/lib/conntrack-tcp.c @@ -181,11 +181,16 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, return CT_UPDATE_INVALID; } - if (((tcp_flags & (TCP_SYN | TCP_ACK)) == TCP_SYN) - && dst->state >= CT_DPIF_TCPS_FIN_WAIT_2 - && src->state >= CT_DPIF_TCPS_FIN_WAIT_2) { - src->state = dst->state = CT_DPIF_TCPS_CLOSED; - return CT_UPDATE_NEW; + if ((tcp_flags & (TCP_SYN | TCP_ACK)) == TCP_SYN) { + if (dst->state >= CT_DPIF_TCPS_FIN_WAIT_2 + && src->state >= CT_DPIF_TCPS_FIN_WAIT_2) { + src->state = dst->state = CT_DPIF_TCPS_CLOSED; + return CT_UPDATE_NEW; + } else if (src->state <= CT_DPIF_TCPS_SYN_SENT) { + src->state = CT_DPIF_TCPS_SYN_SENT; + conn_update_expiration(ct, &conn->up, CT_TM_TCP_FIRST_PACKET, now); + return CT_UPDATE_NEW; + } } if (src->wscale & CT_WSCALE_FLAG diff --git a/lib/conntrack.c b/lib/conntrack.c index 60222ca53..ff5a89457 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1110,6 +1110,9 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, ovs_mutex_unlock(&ct->ct_lock); create_new_conn = true; break; + case CT_UPDATE_VALID_NEW: + pkt->md.ct_state |= CS_NEW; + break; default: OVS_NOT_REACHED(); } diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 0fb7aacfa..4a39c929c 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2290,6 +2290,47 @@ tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=,dport=),reply=(src= OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([conntrack - new connections]) +CHECK_CONNTRACK() +OVS_TRAFFIC_VSWITCHD_START() + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +AT_DATA([flows1.txt], [dnl +table=0, priority=1,action=drop +table=0, priority=10,arp,action=normal +table=0, priority=100,tcp,action=ct(table=1) +table=0, priority=100,udp,action=ct(table=1) +table=1, priority=100,in_port=1,tcp,ct_state=+trk+new,action=ct(commit) +table=1, priority=100,in_port=1,udp,ct_state=+trk+new,action=ct(commit) +table=1, priority=100,in_port=1,ct_state=+trk+est,action=2 +table=1, priority=100,in_port=2,ct_state=+trk+est,action=1 +]) + +ovs-appctl vlog/set dbg + +AT_CHECK([ovs-ofctl --bundle add-flows br0 flows1.txt]) + +dnl TCP traffic from ns0 to ns1 should fail. +OVS_START_L7([at_ns1], [http]) +NS_CHECK_EXEC([at_ns0], [wget 10.1.1.2 -t 3 -T 1 --retry-connrefused -v -o wget0.log], [4]) + +dnl Send UDP packet on port 1 twice. +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"]) +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"]) + +dnl There should not be any packet that matches the established ct_state. +AT_CHECK([ovs-ofctl dump-flows br0 "table=1 in_port=1,ct_state=+trk+est" | ofctl_strip], [0], [dnl +NXST_FLOW reply: + table=1, priority=100,ct_state=+est+trk,in_port=1 actions=output:2 +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([conntrack - ICMP related]) AT_SKIP_IF([test $HAVE_NC = no]) CHECK_CONNTRACK() -- 2.39.2