conntrack: Handle already natted packets.

When a packet gets dnatted and then recirculated, it could be possible
that it matches another rule that performs another nat action.
The kernel datapath handles this situation turning to a no-op the
second nat action, so natting only once the packet.  In the userspace
datapath instead, when the ct action gets executed, an initial lookup
of the translated packet fails to retrieve the connection related to
the packet, leading to the creation of a new entry in ct for the src
nat action with a subsequent failure of the connection establishment.

with the following flows:

table=0,priority=30,in_port=1,ip,nw_dst=192.168.2.100,actions=ct(commit,nat(dst=10.1.1.2:80),table=1)
table=0,priority=20,in_port=2,ip,actions=ct(nat,table=1)
table=0,priority=10,ip,actions=resubmit(,2)
table=0,priority=10,arp,actions=NORMAL
table=0,priority=0,actions=drop
table=1,priority=5,ip,actions=ct(commit,nat(src=10.1.1.240),table=2)
table=2,in_port=ovs-l0,actions=2
table=2,in_port=ovs-r0,actions=1

Establishing a connection from 10.1.1.1 to 192.168.2.100 the outcome is:

  tcp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=4000,dport=80),
     reply=(src=10.1.1.2,dst=10.1.1.240,sport=80,dport=4000),
     protoinfo=(state=ESTABLISHED)
  tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),
     reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),
     protoinfo=(state=ESTABLISHED)

With this patch applied the outcome is:

  tcp,orig=(src=10.1.1.1,dst=192.168.2.100,sport=4000,dport=80),
     reply=(src=10.1.1.2,dst=10.1.1.1,sport=80,dport=4000),
     protoinfo=(state=ESTABLISHED)

The patch performs, for already natted packets, a lookup of the
reverse key in order to retrieve the related entry, it also adds a
test case that besides testing the scenario ensures that the other ct
actions are executed.

Reported-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

2 files changed, 66 insertions, 1 deletions

diff --git a/lib/conntrack.c b/lib/conntrack.c
index bb98395cd..99ea29d94 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -44,6 +44,7 @@ VLOG_DEFINE_THIS_MODULE(conntrack);
 
 COVERAGE_DEFINE(conntrack_full);
 COVERAGE_DEFINE(conntrack_long_cleanup);
+COVERAGE_DEFINE(conntrack_lookup_natted_miss);
 
 struct conn_lookup_ctx {
     struct conn_key key;
@@ -1270,6 +1271,34 @@ process_one_fast(uint16_t zone, const uint32_t *setmark,
 }
 
 static void
+initial_conn_lookup(struct conntrack *ct, struct conn_lookup_ctx *ctx,
+                    long long now, bool natted)
+{
+    if (natted) {
+        /* If the packet has been already natted (e.g. a previous
+         * action took place), retrieve it performing a lookup of its
+         * reverse key. */
+        conn_key_reverse(&ctx->key);
+    }
+
+    conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply);
+
+    if (natted) {
+        if (OVS_LIKELY(ctx->conn)) {
+            ctx->reply = !ctx->reply;
+            ctx->key = ctx->reply ? ctx->conn->rev_key : ctx->conn->key;
+            ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
+        } else {
+            /* A lookup failure does not necessarily imply that an
+             * error occurred, it may simply indicate that a conn got
+             * removed during the recirculation. */
+            COVERAGE_INC(conntrack_lookup_natted_miss);
+            conn_key_reverse(&ctx->key);
+        }
+    }
+}
+
+static void
 process_one(struct conntrack *ct, struct dp_packet *pkt,
             struct conn_lookup_ctx *ctx, uint16_t zone,
             bool force, bool commit, long long now, const uint32_t *setmark,
@@ -1283,7 +1312,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
     }
 
     bool create_new_conn = false;
-    conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply);
+    initial_conn_lookup(ct, ctx, now, !!(pkt->md.ct_state &
+                                         (CS_SRC_NAT | CS_DST_NAT)));
     struct conn *conn = ctx->conn;
 
     /* Delete found entry if in wrong direction. 'force' implies commit. */
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 3bcd65e84..5cfb7d72d 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -4498,6 +4498,41 @@ 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 - DNAT with additional SNAT])
+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")
+NS_CHECK_EXEC([at_ns0], [ip route add 172.1.1.0/24 via 10.1.1.2])
+
+OVS_START_L7([at_ns1], [http])
+
+AT_DATA([flows.txt], [dnl
+table=0,priority=30,in_port=1,ip,nw_dst=172.1.1.2,actions=ct(commit,nat(dst=10.1.1.2:80),table=1)
+table=0,priority=20,in_port=2,ip,actions=ct(nat),1
+table=0,priority=10,arp,actions=NORMAL
+table=0,priority=1,actions=drop
+dnl Be sure all ct() actions but src nat are executed
+table=1,ip,actions=ct(commit,nat(src=10.1.1.240),exec(set_field:0xac->ct_mark,set_field:0xac->ct_label),table=2)
+table=2,in_port=1,ip,ct_mark=0xac,ct_label=0xac,actions=2
+])
+AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt])
+
+NS_CHECK_EXEC([at_ns0], [wget http://172.1.1.2:8080 -t 5 -T 1 --retry-connrefused -v -o wget0.log])
+
+dnl - make sure only dst nat has been performed
+AT_CHECK([ovs-appctl dpctl/dump-conntrack |  FORMAT_CT(10.1.1.240)], [0], [dnl
+])
+
+AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.1)], [0], [dnl
+tcp,orig=(src=10.1.1.1,dst=172.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),mark=172,labels=0xac,protoinfo=(state=<cleared>)
+])
+
+OVS_TRAFFIC_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([conntrack - more complex DNAT])
 CHECK_CONNTRACK()
 CHECK_CONNTRACK_NAT()