[ovs-dev] [PATCH] conntrack: Fix conntrack new state

[Yi-Hung Wei]

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 <yihung.wei at gmail.com>
---
Travis CI: https://travis-ci.org/YiHungWei/ovs/builds/627518780

---
 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 932f2f4ad9c6..de22ef87cc19 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 b04e4cd77542..9a8ca3910157 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 47eb8e20346f..416cb769d22f 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 b80080e72bf8..eb3790a9aecb 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 0fb7aacfa14e..4a39c929c207 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=<cleared>,dport=<cleared>),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.7.4