diff mbox series

[ovs-dev,branch-2.17,2/2] conntrack: Remove nat_conn introducing key directionality.

Message ID 20230926122034.418115-2-aconole@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,branch-2.17,1/2] conntrack: simplify cleanup path | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation fail test: fail

Commit Message

Aaron Conole Sept. 26, 2023, 12:20 p.m. UTC
From: Peng He <hepeng.0320@bytedance.com>

The patch avoids the extra allocation for nat_conn.
Currently, when doing NAT, the userspace conntrack will use an extra
conn for the two directions in a flow. However, each conn has actually
the two keys for both orig and rev directions. This patch introduces a
key_node[CT_DIRS] member as per Aaron's suggestion in the conn which
consists of a key, direction, and a cmap_node for hash lookup so
addressing the feedback received by the original patch [0].

With this adjustment, we also remove the assertion that connections in
the table are DEFAULT while updating connection state and/or removing
connections.

[0] https://patchwork.ozlabs.org/project/openvswitch/patch/20201129033255.64647-2-hepeng.0320@bytedance.com/

[Aaron resolved numerous conflicts due to lack of multiple commits]

Reported-by: Michael Plato <michael.plato@tu-berlin.de>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2022-September/052065.html
Signed-off-by: Peng He <hepeng.0320@bytedance.com>
Co-authored-by: Paolo Valerio <pvalerio@redhat.com>
Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
Tested-by: Frode Nordahl <frode.nordahl@canonical.com>
Acked-by: Ilya Maximets <i.maximets@ovn.org>
Acked-by: Aaron Conole <aconole@redhat.com>
Signed-off-by: Aaron Conole <aconole@redhat.com>
---
 lib/conntrack-private.h |  19 +-
 lib/conntrack-tp.c      |   6 +-
 lib/conntrack.c         | 379 +++++++++++++++++-----------------------
 3 files changed, 180 insertions(+), 224 deletions(-)

Comments

0-day Robot Sept. 26, 2023, 12:39 p.m. UTC | #1
Bleep bloop.  Greetings Aaron Conole, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Aaron Conole <aconole@redhat.com>
Lines checked: 947, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index dfdf4e676b..581f517ad1 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -48,6 +48,12 @@  struct ct_endpoint {
  * hashing in ct_endpoint_hash_add(). */
 BUILD_ASSERT_DECL(sizeof(struct ct_endpoint) == sizeof(union ct_addr) + 4);
 
+enum key_dir {
+    CT_DIR_FWD = 0,
+    CT_DIR_REV,
+    CT_DIRS,
+};
+
 /* Changes to this structure need to be reflected in conn_key_hash()
  * and conn_key_cmp(). */
 struct conn_key {
@@ -86,21 +92,19 @@  struct alg_exp_node {
     bool nat_rpl_dst;
 };
 
-enum OVS_PACKED_ENUM ct_conn_type {
-    CT_CONN_TYPE_DEFAULT,
-    CT_CONN_TYPE_UN_NAT,
+struct conn_key_node {
+    enum key_dir dir;
+    struct conn_key key;
+    struct cmap_node cm_node;
 };
 
 struct conn {
     /* Immutable data. */
-    struct conn_key key;
-    struct conn_key rev_key;
+    struct conn_key_node key_node[CT_DIRS];
     struct conn_key parent_key; /* Only used for orig_tuple support. */
     struct ovs_list exp_node;
-    struct cmap_node cm_node;
     uint16_t nat_action;
     char *alg;
-    struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
 
     /* Mutable data. */
     struct ovs_mutex lock; /* Guards all mutable fields. */
@@ -120,7 +124,6 @@  struct conn {
 
     /* Immutable data. */
     bool alg_related; /* True if alg data connection. */
-    enum ct_conn_type conn_type;
 
     uint32_t tp_id; /* Timeout policy ID. */
 };
diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
index a586d3a8d3..2bdda67110 100644
--- a/lib/conntrack-tp.c
+++ b/lib/conntrack-tp.c
@@ -276,7 +276,8 @@  conn_update_expiration(struct conntrack *ct, struct conn *conn,
     ovs_mutex_lock(&conn->lock);
     VLOG_DBG_RL(&rl, "Update timeout %s zone=%u with policy id=%d "
                 "val=%u sec.",
-                ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+                ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+                conn->tp_id, val);
 
     conn_update_expiration__(ct, conn, tm, now, val);
 }
@@ -307,7 +308,8 @@  conn_init_expiration(struct conntrack *ct, struct conn *conn,
     }
 
     VLOG_DBG_RL(&rl, "Init timeout %s zone=%u with policy id=%d val=%u sec.",
-                ct_timeout_str[tm], conn->key.zone, conn->tp_id, val);
+                ct_timeout_str[tm], conn->key_node[CT_DIR_FWD].key.zone,
+                conn->tp_id, val);
 
     conn_init_expiration__(ct, conn, tm, now, val);
 }
diff --git a/lib/conntrack.c b/lib/conntrack.c
index e5bf27a321..f7b870a3ef 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -96,12 +96,11 @@  static struct conn *new_conn(struct conntrack *ct, struct dp_packet *pkt,
                              uint32_t tp_id);
 static void delete_conn__(struct conn *);
 static void delete_conn(struct conn *);
-static void delete_conn_one(struct conn *conn);
 static enum ct_update_res conn_update(struct conntrack *ct, struct conn *conn,
                                       struct dp_packet *pkt,
                                       struct conn_lookup_ctx *ctx,
                                       long long now);
-static bool conn_expired(struct conn *, long long now);
+static bool conn_expired(const struct conn *, long long now);
 static void set_mark(struct dp_packet *, struct conn *,
                      uint32_t val, uint32_t mask);
 static void set_label(struct dp_packet *, struct conn *,
@@ -110,8 +109,7 @@  static void set_label(struct dp_packet *, struct conn *,
 static void *clean_thread_main(void *f_);
 
 static bool
-nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
-                     struct conn *nat_conn,
+nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
                      const struct nat_action_info_t *nat_info);
 
 static uint8_t
@@ -205,7 +203,7 @@  static alg_helper alg_helpers[] = {
 #define ALG_WC_SRC_PORT 0
 
 /* If the total number of connections goes above this value, no new connections
- * are accepted; this is for CT_CONN_TYPE_DEFAULT connections. */
+ * are accepted. */
 #define DEFAULT_N_CONN_LIMIT 3000000
 
 /* Does a member by member comparison of two conn_keys; this
@@ -231,61 +229,6 @@  conn_key_cmp(const struct conn_key *key1, const struct conn_key *key2)
     return 1;
 }
 
-static void
-ct_print_conn_info(const struct conn *c, const char *log_msg,
-                   enum vlog_level vll, bool force, bool rl_on)
-{
-#define CT_VLOG(RL_ON, LEVEL, ...)                                          \
-    do {                                                                    \
-        if (RL_ON) {                                                        \
-            static struct vlog_rate_limit rl_ = VLOG_RATE_LIMIT_INIT(5, 5); \
-            vlog_rate_limit(&this_module, LEVEL, &rl_, __VA_ARGS__);        \
-        } else {                                                            \
-            vlog(&this_module, LEVEL, __VA_ARGS__);                         \
-        }                                                                   \
-    } while (0)
-
-    if (OVS_UNLIKELY(force || vlog_is_enabled(&this_module, vll))) {
-        if (c->key.dl_type == htons(ETH_TYPE_IP)) {
-            CT_VLOG(rl_on, vll, "%s: src ip "IP_FMT" dst ip "IP_FMT" rev src "
-                    "ip "IP_FMT" rev dst ip "IP_FMT" src/dst ports "
-                    "%"PRIu16"/%"PRIu16" rev src/dst ports "
-                    "%"PRIu16"/%"PRIu16" zone/rev zone "
-                    "%"PRIu16"/%"PRIu16" nw_proto/rev nw_proto "
-                    "%"PRIu8"/%"PRIu8, log_msg,
-                    IP_ARGS(c->key.src.addr.ipv4),
-                    IP_ARGS(c->key.dst.addr.ipv4),
-                    IP_ARGS(c->rev_key.src.addr.ipv4),
-                    IP_ARGS(c->rev_key.dst.addr.ipv4),
-                    ntohs(c->key.src.port), ntohs(c->key.dst.port),
-                    ntohs(c->rev_key.src.port), ntohs(c->rev_key.dst.port),
-                    c->key.zone, c->rev_key.zone, c->key.nw_proto,
-                    c->rev_key.nw_proto);
-        } else {
-            char ip6_s[INET6_ADDRSTRLEN];
-            inet_ntop(AF_INET6, &c->key.src.addr.ipv6, ip6_s, sizeof ip6_s);
-            char ip6_d[INET6_ADDRSTRLEN];
-            inet_ntop(AF_INET6, &c->key.dst.addr.ipv6, ip6_d, sizeof ip6_d);
-            char ip6_rs[INET6_ADDRSTRLEN];
-            inet_ntop(AF_INET6, &c->rev_key.src.addr.ipv6, ip6_rs,
-                      sizeof ip6_rs);
-            char ip6_rd[INET6_ADDRSTRLEN];
-            inet_ntop(AF_INET6, &c->rev_key.dst.addr.ipv6, ip6_rd,
-                      sizeof ip6_rd);
-
-            CT_VLOG(rl_on, vll, "%s: src ip %s dst ip %s rev src ip %s"
-                    " rev dst ip %s src/dst ports %"PRIu16"/%"PRIu16
-                    " rev src/dst ports %"PRIu16"/%"PRIu16" zone/rev zone "
-                    "%"PRIu16"/%"PRIu16" nw_proto/rev nw_proto "
-                    "%"PRIu8"/%"PRIu8, log_msg, ip6_s, ip6_d, ip6_rs,
-                    ip6_rd, ntohs(c->key.src.port), ntohs(c->key.dst.port),
-                    ntohs(c->rev_key.src.port), ntohs(c->rev_key.dst.port),
-                    c->key.zone, c->rev_key.zone, c->key.nw_proto,
-                    c->rev_key.nw_proto);
-        }
-    }
-}
-
 /* Initializes the connection tracker 'ct'.  The caller is responsible for
  * calling 'conntrack_destroy()', when the instance is not needed anymore */
 struct conntrack *
@@ -447,23 +390,24 @@  static void
 conn_clean(struct conntrack *ct, struct conn *conn)
     OVS_REQUIRES(ct->ct_lock)
 {
-    ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT);
+    uint32_t hash;
 
     if (conn->alg) {
-        expectation_clean(ct, &conn->key);
+        expectation_clean(ct, &conn->key_node[CT_DIR_FWD].key);
     }
 
-    uint32_t hash = conn_key_hash(&conn->key, ct->hash_basis);
-    cmap_remove(&ct->conns, &conn->cm_node, hash);
+    hash = conn_key_hash(&conn->key_node[CT_DIR_FWD].key, ct->hash_basis);
+    cmap_remove(&ct->conns, &conn->key_node[CT_DIR_FWD].cm_node, hash);
 
     struct zone_limit *zl = zone_limit_lookup(ct, conn->admit_zone);
     if (zl && zl->czl.zone_limit_seq == conn->zone_limit_seq) {
         zl->czl.count--;
     }
 
-    if (conn->nat_conn) {
-        hash = conn_key_hash(&conn->nat_conn->key, ct->hash_basis);
-        cmap_remove(&ct->conns, &conn->nat_conn->cm_node, hash);
+    if (conn->nat_action) {
+        hash = conn_key_hash(&conn->key_node[CT_DIR_REV].key,
+                             ct->hash_basis);
+        cmap_remove(&ct->conns, &conn->key_node[CT_DIR_REV].cm_node, hash);
     }
     ovs_list_remove(&conn->exp_node);
     conn->cleaned = true;
@@ -477,15 +421,18 @@  conn_clean(struct conntrack *ct, struct conn *conn)
 void
 conntrack_destroy(struct conntrack *ct)
 {
+    struct conn_key_node *keyn;
     struct conn *conn;
     latch_set(&ct->clean_thread_exit);
     pthread_join(ct->clean_thread, NULL);
     latch_destroy(&ct->clean_thread_exit);
 
     ovs_mutex_lock(&ct->ct_lock);
-    CMAP_FOR_EACH (conn, cm_node, &ct->conns) {
-        if (conn->conn_type != CT_CONN_TYPE_DEFAULT) {
-            continue;
+    CMAP_FOR_EACH (keyn, cm_node, &ct->conns) {
+        if (keyn->dir == CT_DIR_FWD) {
+            conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
+        } else {
+            conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_REV]);
         }
 
         conn_clean(ct, conn);
@@ -527,31 +474,39 @@  conn_key_lookup(struct conntrack *ct, const struct conn_key *key,
                 uint32_t hash, long long now, struct conn **conn_out,
                 bool *reply)
 {
-    struct conn *conn;
+    struct conn_key_node *keyn;
+    struct conn *conn = NULL;
     bool found = false;
 
-    CMAP_FOR_EACH_WITH_HASH (conn, cm_node, hash, &ct->conns) {
-        if (!conn_key_cmp(&conn->key, key) && !conn_expired(conn, now)) {
-            found = true;
-            if (reply) {
-                *reply = false;
-            }
-            break;
+    CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, &ct->conns) {
+        if (keyn->dir == CT_DIR_FWD) {
+            conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
+        } else {
+            conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_REV]);
+        }
+
+        if (conn_expired(conn, now)) {
+            continue;
         }
-        if (!conn_key_cmp(&conn->rev_key, key) && !conn_expired(conn, now)) {
-            found = true;
-            if (reply) {
-                *reply = true;
+
+        for (int i = CT_DIR_FWD; i < CT_DIRS; i++) {
+            if (!conn_key_cmp(&conn->key_node[i].key, key)) {
+                found = true;
+                if (reply) {
+                    *reply = (i == CT_DIR_REV);
+                }
+                goto out_found;
             }
-            break;
         }
     }
 
+out_found:
     if (found && conn_out) {
         *conn_out = conn;
     } else if (conn_out) {
         *conn_out = NULL;
     }
+
     return found;
 }
 
@@ -585,7 +540,7 @@  write_ct_md(struct dp_packet *pkt, uint16_t zone, const struct conn *conn,
         if (conn->alg_related) {
             key = &conn->parent_key;
         } else {
-            key = &conn->key;
+            key = &conn->key_node[CT_DIR_FWD].key;
         }
     } else if (alg_exp) {
         pkt->md.ct_mark = alg_exp->parent_mark;
@@ -814,7 +769,8 @@  nat_inner_packet(struct dp_packet *pkt, struct conn_key *key,
 static void
 nat_packet(struct dp_packet *pkt, struct conn *conn, bool reply, bool related)
 {
-    struct conn_key *key = reply ? &conn->key : &conn->rev_key;
+    enum key_dir dir = reply ? CT_DIR_FWD : CT_DIR_REV;
+    struct conn_key *key = &conn->key_node[dir].key;
     uint16_t nat_action = reply ? nat_action_reverse(conn->nat_action)
                                 : conn->nat_action;
 
@@ -849,7 +805,7 @@  conn_seq_skew_set(struct conntrack *ct, const struct conn *conn_in,
 {
     struct conn *conn;
     ovs_mutex_unlock(&conn_in->lock);
-    conn_lookup(ct, &conn_in->key, now, &conn, NULL);
+    conn_lookup(ct, &conn_in->key_node[CT_DIR_FWD].key, now, &conn, NULL);
     ovs_mutex_lock(&conn_in->lock);
 
     if (conn && seq_skew) {
@@ -887,7 +843,6 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
     OVS_REQUIRES(ct->ct_lock)
 {
     struct conn *nc = NULL;
-    struct conn *nat_conn = NULL;
 
     if (!valid_new(pkt, &ctx->key)) {
         pkt->md.ct_state = CS_INVALID;
@@ -901,6 +856,7 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
     }
 
     if (commit) {
+        struct conn_key_node *fwd_key_node, *rev_key_node;
         struct zone_limit *zl = zone_limit_lookup_or_default(ct,
                                                              ctx->key.zone);
         if (zl && zl->czl.count >= zl->czl.limit) {
@@ -915,9 +871,12 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
         }
 
         nc = new_conn(ct, pkt, &ctx->key, now, tp_id);
-        memcpy(&nc->key, &ctx->key, sizeof nc->key);
-        memcpy(&nc->rev_key, &nc->key, sizeof nc->rev_key);
-        conn_key_reverse(&nc->rev_key);
+        fwd_key_node = &nc->key_node[CT_DIR_FWD];
+        rev_key_node = &nc->key_node[CT_DIR_REV];
+        memcpy(&fwd_key_node->key, &ctx->key, sizeof fwd_key_node->key);
+        memcpy(&rev_key_node->key, &fwd_key_node->key,
+               sizeof rev_key_node->key);
+        conn_key_reverse(&rev_key_node->key);
 
         if (ct_verify_helper(helper, ct_alg_ctl)) {
             nc->alg = nullable_xstrdup(helper);
@@ -932,45 +891,33 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
 
         if (nat_action_info) {
             nc->nat_action = nat_action_info->nat_action;
-            nat_conn = xzalloc(sizeof *nat_conn);
 
             if (alg_exp) {
                 if (alg_exp->nat_rpl_dst) {
-                    nc->rev_key.dst.addr = alg_exp->alg_nat_repl_addr;
+                    rev_key_node->key.dst.addr = alg_exp->alg_nat_repl_addr;
                     nc->nat_action = NAT_ACTION_SRC;
                 } else {
-                    nc->rev_key.src.addr = alg_exp->alg_nat_repl_addr;
+                    rev_key_node->key.src.addr = alg_exp->alg_nat_repl_addr;
                     nc->nat_action = NAT_ACTION_DST;
                 }
             } else {
-                memcpy(nat_conn, nc, sizeof *nat_conn);
-                bool nat_res = nat_get_unique_tuple(ct, nc, nat_conn,
-                                                    nat_action_info);
+                bool nat_res = nat_get_unique_tuple(ct, nc, nat_action_info);
 
                 if (!nat_res) {
                     goto nat_res_exhaustion;
                 }
-
-                /* Update nc with nat adjustments made to nat_conn by
-                 * nat_get_unique_tuple(). */
-                memcpy(nc, nat_conn, sizeof *nc);
             }
 
             nat_packet(pkt, nc, false, ctx->icmp_related);
-            memcpy(&nat_conn->key, &nc->rev_key, sizeof nat_conn->key);
-            memcpy(&nat_conn->rev_key, &nc->key, sizeof nat_conn->rev_key);
-            nat_conn->conn_type = CT_CONN_TYPE_UN_NAT;
-            nat_conn->nat_action = 0;
-            nat_conn->alg = NULL;
-            nat_conn->nat_conn = NULL;
-            uint32_t nat_hash = conn_key_hash(&nat_conn->key, ct->hash_basis);
-            cmap_insert(&ct->conns, &nat_conn->cm_node, nat_hash);
+            uint32_t rev_hash = conn_key_hash(&rev_key_node->key,
+                                              ct->hash_basis);
+            cmap_insert(&ct->conns, &rev_key_node->cm_node, rev_hash);
         }
 
-        nc->nat_conn = nat_conn;
         ovs_mutex_init_adaptive(&nc->lock);
-        nc->conn_type = CT_CONN_TYPE_DEFAULT;
-        cmap_insert(&ct->conns, &nc->cm_node, ctx->hash);
+        fwd_key_node->dir = CT_DIR_FWD;
+        rev_key_node->dir = CT_DIR_REV;
+        cmap_insert(&ct->conns, &fwd_key_node->cm_node, ctx->hash);
         atomic_count_inc(&ct->n_conn);
         ctx->conn = nc; /* For completeness. */
         if (zl) {
@@ -990,7 +937,6 @@  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
      * firewall rules or a separate firewall.  Also using zone partitioning
      * can limit DoS impact. */
 nat_res_exhaustion:
-    free(nat_conn);
     ovs_list_remove(&nc->exp_node);
     delete_conn__(nc);
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
@@ -1004,7 +950,6 @@  conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
                   struct conn_lookup_ctx *ctx, struct conn *conn,
                   long long now)
 {
-    ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT);
     bool create_new_conn = false;
 
     if (ctx->icmp_related) {
@@ -1032,7 +977,8 @@  conn_update_state(struct conntrack *ct, struct dp_packet *pkt,
             break;
         case CT_UPDATE_NEW:
             ovs_mutex_lock(&ct->ct_lock);
-            if (conn_lookup(ct, &conn->key, now, NULL, NULL)) {
+            if (conn_lookup(ct, &conn->key_node[CT_DIR_FWD].key,
+                            now, NULL, NULL)) {
                 conn_clean(ct, conn);
             }
             ovs_mutex_unlock(&ct->ct_lock);
@@ -1209,8 +1155,10 @@  initial_conn_lookup(struct conntrack *ct, struct conn_lookup_ctx *ctx,
 
     if (natted) {
         if (OVS_LIKELY(ctx->conn)) {
+            enum key_dir dir;
             ctx->reply = !ctx->reply;
-            ctx->key = ctx->reply ? ctx->conn->rev_key : ctx->conn->key;
+            dir = ctx->reply ? CT_DIR_REV : CT_DIR_FWD;
+            ctx->key = ctx->conn->key_node[dir].key;
             ctx->hash = conn_key_hash(&ctx->key, ct->hash_basis);
         } else {
             /* A lookup failure does not necessarily imply that an
@@ -1244,32 +1192,14 @@  process_one(struct conntrack *ct, struct dp_packet *pkt,
     /* Delete found entry if in wrong direction. 'force' implies commit. */
     if (OVS_UNLIKELY(force && ctx->reply && conn)) {
         ovs_mutex_lock(&ct->ct_lock);
-        if (conn_lookup(ct, &conn->key, now, NULL, NULL)) {
+        if (conn_lookup(ct, &conn->key_node[CT_DIR_FWD].key,
+                        now, NULL, NULL)) {
             conn_clean(ct, conn);
         }
         ovs_mutex_unlock(&ct->ct_lock);
         conn = NULL;
     }
 
-    if (OVS_LIKELY(conn)) {
-        if (conn->conn_type == CT_CONN_TYPE_UN_NAT) {
-
-            ctx->reply = true;
-            struct conn *rev_conn = conn;  /* Save for debugging. */
-            uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis);
-            conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply);
-
-            if (!conn) {
-                pkt->md.ct_state |= CS_INVALID;
-                write_ct_md(pkt, zone, NULL, NULL, NULL);
-                char *log_msg = xasprintf("Missing parent conn %p", rev_conn);
-                ct_print_conn_info(rev_conn, log_msg, VLL_INFO, true, true);
-                free(log_msg);
-                return;
-            }
-        }
-    }
-
     enum ct_alg_ctl_type ct_alg_ctl = get_alg_ctl_type(pkt, tp_src, tp_dst,
                                                        helper);
 
@@ -1362,8 +1292,9 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
         struct conn *conn = packet->md.conn;
         if (OVS_UNLIKELY(packet->md.ct_state == CS_INVALID)) {
             write_ct_md(packet, zone, NULL, NULL, NULL);
-        } else if (conn && conn->key.zone == zone && !force
-                   && !get_alg_ctl_type(packet, tp_src, tp_dst, helper)) {
+        } else if (conn &&
+                   conn->key_node[CT_DIR_FWD].key.zone == zone && !force &&
+                   !get_alg_ctl_type(packet, tp_src, tp_dst, helper)) {
             process_one_fast(zone, setmark, setlabel, nat_action_info,
                              conn, packet);
         } else if (OVS_UNLIKELY(!conn_key_extract(ct, packet, dl_type, &ctx,
@@ -2143,7 +2074,7 @@  nat_ipv6_addr_increment(struct in6_addr *ipv6, uint32_t increment)
 }
 
 static uint32_t
-nat_range_hash(const struct conn *conn, uint32_t basis,
+nat_range_hash(const struct conn_key *key, uint32_t basis,
                const struct nat_action_info_t *nat_info)
 {
     uint32_t hash = basis;
@@ -2153,11 +2084,11 @@  nat_range_hash(const struct conn *conn, uint32_t basis,
     hash = hash_add(hash,
                     ((uint32_t) nat_info->max_port << 16)
                     | nat_info->min_port);
-    hash = ct_endpoint_hash_add(hash, &conn->key.src);
-    hash = ct_endpoint_hash_add(hash, &conn->key.dst);
-    hash = hash_add(hash, (OVS_FORCE uint32_t) conn->key.dl_type);
-    hash = hash_add(hash, conn->key.nw_proto);
-    hash = hash_add(hash, conn->key.zone);
+    hash = ct_endpoint_hash_add(hash, &key->src);
+    hash = ct_endpoint_hash_add(hash, &key->dst);
+    hash = hash_add(hash, (OVS_FORCE uint32_t) key->dl_type);
+    hash = hash_add(hash, key->nw_proto);
+    hash = hash_add(hash, key->zone);
 
     /* The purpose of the second parameter is to distinguish hashes of data of
      * different length; our data always has the same length so there is no
@@ -2231,7 +2162,7 @@  get_addr_in_range(union ct_addr *min, union ct_addr *max,
 }
 
 static void
-get_initial_addr(const struct conn *conn, union ct_addr *min,
+get_initial_addr(const struct conn_key *key, union ct_addr *min,
                  union ct_addr *max, union ct_addr *curr,
                  uint32_t hash, bool ipv4,
                  const struct nat_action_info_t *nat_info)
@@ -2241,9 +2172,9 @@  get_initial_addr(const struct conn *conn, union ct_addr *min,
     /* All-zero case. */
     if (!memcmp(min, &zero_ip, sizeof *min)) {
         if (nat_info->nat_action & NAT_ACTION_SRC) {
-            *curr = conn->key.src.addr;
+            *curr = key->src.addr;
         } else if (nat_info->nat_action & NAT_ACTION_DST) {
-            *curr = conn->key.dst.addr;
+            *curr = key->dst.addr;
         }
     } else {
         get_addr_in_range(min, max, curr, hash, ipv4);
@@ -2307,7 +2238,7 @@  next_addr_in_range_guarded(union ct_addr *curr, union ct_addr *min,
 }
 
 static bool
-nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
+nat_get_unique_l4(struct conntrack *ct, struct conn_key *rev_key,
                   ovs_be16 *port, uint16_t curr, uint16_t min,
                   uint16_t max)
 {
@@ -2315,8 +2246,7 @@  nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
 
     FOR_EACH_PORT_IN_RANGE (curr, min, max) {
         *port = htons(curr);
-        if (!conn_lookup(ct, &nat_conn->rev_key,
-                         time_msec(), NULL, NULL)) {
+        if (!conn_lookup(ct, rev_key, time_msec(), NULL, NULL)) {
             return true;
         }
     }
@@ -2348,45 +2278,45 @@  nat_get_unique_l4(struct conntrack *ct, struct conn *nat_conn,
  *
  * If none can be found, return exhaustion to the caller. */
 static bool
-nat_get_unique_tuple(struct conntrack *ct, const struct conn *conn,
-                     struct conn *nat_conn,
+nat_get_unique_tuple(struct conntrack *ct, struct conn *conn,
                      const struct nat_action_info_t *nat_info)
 {
+    struct conn_key *fwd_key = &conn->key_node[CT_DIR_FWD].key;
+    struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key;
     union ct_addr min_addr = {0}, max_addr = {0}, curr_addr = {0},
                   guard_addr = {0};
-    uint32_t hash = nat_range_hash(conn, ct->hash_basis, nat_info);
-    bool pat_proto = conn->key.nw_proto == IPPROTO_TCP ||
-                     conn->key.nw_proto == IPPROTO_UDP;
+    bool pat_proto = fwd_key->nw_proto == IPPROTO_TCP ||
+                     fwd_key->nw_proto == IPPROTO_UDP;
     uint16_t min_dport, max_dport, curr_dport;
     uint16_t min_sport, max_sport, curr_sport;
+    uint32_t hash;
 
+    hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
     min_addr = nat_info->min_addr;
     max_addr = nat_info->max_addr;
 
-    get_initial_addr(conn, &min_addr, &max_addr, &curr_addr, hash,
-                     (conn->key.dl_type == htons(ETH_TYPE_IP)), nat_info);
+    get_initial_addr(fwd_key, &min_addr, &max_addr, &curr_addr, hash,
+                     (fwd_key->dl_type == htons(ETH_TYPE_IP)), nat_info);
 
     /* Save the address we started from so that
      * we can stop once we reach it. */
     guard_addr = curr_addr;
 
-    set_sport_range(nat_info, &conn->key, hash, &curr_sport,
+    set_sport_range(nat_info, fwd_key, hash, &curr_sport,
                     &min_sport, &max_sport);
-    set_dport_range(nat_info, &conn->key, hash, &curr_dport,
+    set_dport_range(nat_info, fwd_key, hash, &curr_dport,
                     &min_dport, &max_dport);
 
     if (pat_proto) {
-        nat_conn->rev_key.src.port = htons(curr_dport);
-        nat_conn->rev_key.dst.port = htons(curr_sport);
+        rev_key->src.port = htons(curr_dport);
+        rev_key->dst.port = htons(curr_sport);
     }
 
 another_round:
-    store_addr_to_key(&curr_addr, &nat_conn->rev_key,
-                      nat_info->nat_action);
+    store_addr_to_key(&curr_addr, rev_key, nat_info->nat_action);
 
     if (!pat_proto) {
-        if (!conn_lookup(ct, &nat_conn->rev_key,
-                         time_msec(), NULL, NULL)) {
+        if (!conn_lookup(ct, rev_key, time_msec(), NULL, NULL)) {
             return true;
         }
 
@@ -2395,12 +2325,12 @@  another_round:
 
     bool found = false;
     if (nat_info->nat_action & NAT_ACTION_DST_PORT) {
-        found = nat_get_unique_l4(ct, nat_conn, &nat_conn->rev_key.src.port,
+        found = nat_get_unique_l4(ct, rev_key, &rev_key->src.port,
                                   curr_dport, min_dport, max_dport);
     }
 
     if (!found) {
-        found = nat_get_unique_l4(ct, nat_conn, &nat_conn->rev_key.dst.port,
+        found = nat_get_unique_l4(ct, rev_key, &rev_key->dst.port,
                                   curr_sport, min_sport, max_sport);
     }
 
@@ -2413,7 +2343,7 @@  another_round:
 next_addr:
     if (next_addr_in_range_guarded(&curr_addr, &min_addr,
                                    &max_addr, &guard_addr,
-                                   conn->key.dl_type == htons(ETH_TYPE_IP))) {
+                                   fwd_key->dl_type == htons(ETH_TYPE_IP))) {
         return false;
     }
 
@@ -2425,23 +2355,20 @@  conn_update(struct conntrack *ct, struct conn *conn, struct dp_packet *pkt,
             struct conn_lookup_ctx *ctx, long long now)
 {
     ovs_mutex_lock(&conn->lock);
+    uint8_t nw_proto = conn->key_node[CT_DIR_FWD].key.nw_proto;
     enum ct_update_res update_res =
-        l4_protos[conn->key.nw_proto]->conn_update(ct, conn, pkt, ctx->reply,
-                                                   now);
+        l4_protos[nw_proto]->conn_update(ct, conn, pkt, ctx->reply, now);
     ovs_mutex_unlock(&conn->lock);
     return update_res;
 }
 
 static bool
-conn_expired(struct conn *conn, long long now)
+conn_expired(const struct conn *conn, long long now)
 {
-    if (conn->conn_type == CT_CONN_TYPE_DEFAULT) {
-        ovs_mutex_lock(&conn->lock);
-        bool expired = now >= conn->expiration ? true : false;
-        ovs_mutex_unlock(&conn->lock);
-        return expired;
-    }
-    return false;
+    ovs_mutex_lock(&conn->lock);
+    bool expired = now >= conn->expiration ? true : false;
+    ovs_mutex_unlock(&conn->lock);
+    return expired;
 }
 
 static bool
@@ -2467,9 +2394,7 @@  delete_conn__(struct conn *conn)
 static void
 delete_conn(struct conn *conn)
 {
-    ovs_assert(conn->conn_type == CT_CONN_TYPE_DEFAULT);
     ovs_mutex_destroy(&conn->lock);
-    free(conn->nat_conn);
     delete_conn__(conn);
 }
 
@@ -2561,11 +2486,14 @@  static void
 conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry *entry,
                       long long now)
 {
+    const struct conn_key *rev_key = &conn->key_node[CT_DIR_REV].key;
+    const struct conn_key *key = &conn->key_node[CT_DIR_FWD].key;
+
     memset(entry, 0, sizeof *entry);
-    conn_key_to_tuple(&conn->key, &entry->tuple_orig);
-    conn_key_to_tuple(&conn->rev_key, &entry->tuple_reply);
+    conn_key_to_tuple(key, &entry->tuple_orig);
+    conn_key_to_tuple(rev_key, &entry->tuple_reply);
 
-    entry->zone = conn->key.zone;
+    entry->zone = key->zone;
 
     ovs_mutex_lock(&conn->lock);
     entry->mark = conn->mark;
@@ -2573,7 +2501,7 @@  conn_to_ct_dpif_entry(const struct conn *conn, struct ct_dpif_entry *entry,
 
     long long expiration = conn->expiration - now;
 
-    struct ct_l4_proto *class = l4_protos[conn->key.nw_proto];
+    struct ct_l4_proto *class = l4_protos[key->nw_proto];
     if (class->conn_get_protoinfo) {
         class->conn_get_protoinfo(conn, &entry->protoinfo);
     }
@@ -2621,10 +2549,21 @@  conntrack_dump_next(struct conntrack_dump *dump, struct ct_dpif_entry *entry)
         if (!cm_node) {
             break;
         }
+        struct conn_key_node *keyn;
         struct conn *conn;
-        INIT_CONTAINER(conn, cm_node, cm_node);
-        if ((!dump->filter_zone || conn->key.zone == dump->zone) &&
-            (conn->conn_type != CT_CONN_TYPE_UN_NAT)) {
+
+        INIT_CONTAINER(keyn, cm_node, cm_node);
+
+        if (keyn->dir != CT_DIR_FWD) {
+            continue;
+        }
+
+        conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
+        if (conn_expired(conn, now)) {
+            continue;
+        }
+
+        if ((!dump->filter_zone || keyn->key.zone == dump->zone)) {
             conn_to_ct_dpif_entry(conn, entry, now);
             return 0;
         }
@@ -2642,15 +2581,17 @@  conntrack_dump_done(struct conntrack_dump *dump OVS_UNUSED)
 int
 conntrack_flush(struct conntrack *ct, const uint16_t *zone)
 {
+    struct conn_key_node *keyn;
     struct conn *conn;
 
     ovs_mutex_lock(&ct->ct_lock);
-    CMAP_FOR_EACH (conn, cm_node, &ct->conns) {
-        if (conn->conn_type != CT_CONN_TYPE_DEFAULT) {
+    CMAP_FOR_EACH (keyn, cm_node, &ct->conns) {
+        if (keyn->dir != CT_DIR_FWD) {
             continue;
         }
 
-        if (!zone || *zone == conn->key.zone) {
+        conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
+        if (!zone || *zone == keyn->key.zone) {
             conn_clean(ct, conn);
         }
     }
@@ -2663,19 +2604,19 @@  int
 conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple,
                       uint16_t zone)
 {
-    int error = 0;
     struct conn_key key;
     struct conn *conn;
+    int error = 0;
 
     memset(&key, 0, sizeof(key));
     tuple_to_conn_key(tuple, zone, &key);
     ovs_mutex_lock(&ct->ct_lock);
     conn_lookup(ct, &key, time_msec(), &conn, NULL);
 
-    if (conn && conn->conn_type == CT_CONN_TYPE_DEFAULT) {
+    if (conn) {
         conn_clean(ct, conn);
     } else {
-        VLOG_WARN("Must flush tuple using the original pre-NATed tuple");
+        VLOG_WARN("Tuple not found");
         error = ENOENT;
     }
 
@@ -2819,50 +2760,54 @@  expectation_create(struct conntrack *ct, ovs_be16 dst_port,
                    const struct conn *parent_conn, bool reply, bool src_ip_wc,
                    bool skip_nat)
 {
+    const struct conn_key *pconn_key, *pconn_rev_key;
     union ct_addr src_addr;
     union ct_addr dst_addr;
     union ct_addr alg_nat_repl_addr;
     struct alg_exp_node *alg_exp_node = xzalloc(sizeof *alg_exp_node);
 
+    pconn_key = &parent_conn->key_node[CT_DIR_FWD].key;
+    pconn_rev_key = &parent_conn->key_node[CT_DIR_REV].key;
+
     if (reply) {
-        src_addr = parent_conn->key.src.addr;
-        dst_addr = parent_conn->key.dst.addr;
+        src_addr = pconn_key->src.addr;
+        dst_addr = pconn_key->dst.addr;
         alg_exp_node->nat_rpl_dst = true;
         if (skip_nat) {
             alg_nat_repl_addr = dst_addr;
         } else if (parent_conn->nat_action & NAT_ACTION_DST) {
-            alg_nat_repl_addr = parent_conn->rev_key.src.addr;
+            alg_nat_repl_addr = pconn_rev_key->src.addr;
             alg_exp_node->nat_rpl_dst = false;
         } else {
-            alg_nat_repl_addr = parent_conn->rev_key.dst.addr;
+            alg_nat_repl_addr = pconn_rev_key->dst.addr;
         }
     } else {
-        src_addr = parent_conn->rev_key.src.addr;
-        dst_addr = parent_conn->rev_key.dst.addr;
+        src_addr = pconn_rev_key->src.addr;
+        dst_addr = pconn_rev_key->dst.addr;
         alg_exp_node->nat_rpl_dst = false;
         if (skip_nat) {
             alg_nat_repl_addr = src_addr;
         } else if (parent_conn->nat_action & NAT_ACTION_DST) {
-            alg_nat_repl_addr = parent_conn->key.dst.addr;
+            alg_nat_repl_addr = pconn_key->dst.addr;
             alg_exp_node->nat_rpl_dst = true;
         } else {
-            alg_nat_repl_addr = parent_conn->key.src.addr;
+            alg_nat_repl_addr = pconn_key->src.addr;
         }
     }
     if (src_ip_wc) {
         memset(&src_addr, 0, sizeof src_addr);
     }
 
-    alg_exp_node->key.dl_type = parent_conn->key.dl_type;
-    alg_exp_node->key.nw_proto = parent_conn->key.nw_proto;
-    alg_exp_node->key.zone = parent_conn->key.zone;
+    alg_exp_node->key.dl_type = pconn_key->dl_type;
+    alg_exp_node->key.nw_proto = pconn_key->nw_proto;
+    alg_exp_node->key.zone = pconn_key->zone;
     alg_exp_node->key.src.addr = src_addr;
     alg_exp_node->key.dst.addr = dst_addr;
     alg_exp_node->key.src.port = ALG_WC_SRC_PORT;
     alg_exp_node->key.dst.port = dst_port;
     alg_exp_node->parent_mark = parent_conn->mark;
     alg_exp_node->parent_label = parent_conn->label;
-    memcpy(&alg_exp_node->parent_key, &parent_conn->key,
+    memcpy(&alg_exp_node->parent_key, pconn_key,
            sizeof alg_exp_node->parent_key);
     /* Take the write lock here because it is almost 100%
      * likely that the lookup will fail and
@@ -3114,12 +3059,16 @@  process_ftp_ctl_v4(struct conntrack *ct,
 
     switch (mode) {
     case CT_FTP_MODE_ACTIVE:
-        *v4_addr_rep = conn_for_expectation->rev_key.dst.addr.ipv4;
-        conn_ipv4_addr = conn_for_expectation->key.src.addr.ipv4;
+        *v4_addr_rep =
+            conn_for_expectation->key_node[CT_DIR_REV].key.dst.addr.ipv4;
+        conn_ipv4_addr =
+            conn_for_expectation->key_node[CT_DIR_FWD].key.src.addr.ipv4;
         break;
     case CT_FTP_MODE_PASSIVE:
-        *v4_addr_rep = conn_for_expectation->key.dst.addr.ipv4;
-        conn_ipv4_addr = conn_for_expectation->rev_key.src.addr.ipv4;
+        *v4_addr_rep =
+            conn_for_expectation->key_node[CT_DIR_FWD].key.dst.addr.ipv4;
+        conn_ipv4_addr =
+            conn_for_expectation->key_node[CT_DIR_REV].key.src.addr.ipv4;
         break;
     case CT_TFTP_MODE:
     default:
@@ -3151,7 +3100,7 @@  skip_ipv6_digits(char *str)
 static enum ftp_ctl_pkt
 process_ftp_ctl_v6(struct conntrack *ct,
                    struct dp_packet *pkt,
-                   const struct conn *conn_for_expectation,
+                   const struct conn *conn_for_exp,
                    union ct_addr *v6_addr_rep, char **ftp_data_start,
                    size_t *addr_offset_from_ftp_data_start,
                    size_t *addr_size, enum ct_alg_mode *mode)
@@ -3219,24 +3168,25 @@  process_ftp_ctl_v6(struct conntrack *ct,
 
     switch (*mode) {
     case CT_FTP_MODE_ACTIVE:
-        *v6_addr_rep = conn_for_expectation->rev_key.dst.addr;
+        *v6_addr_rep = conn_for_exp->key_node[CT_DIR_REV].key.dst.addr;
         /* Although most servers will block this exploit, there may be some
          * less well managed. */
         if (memcmp(&ip6_addr, &v6_addr_rep->ipv6, sizeof ip6_addr) &&
-            memcmp(&ip6_addr, &conn_for_expectation->key.src.addr.ipv6,
+            memcmp(&ip6_addr,
+                   &conn_for_exp->key_node[CT_DIR_FWD].key.src.addr.ipv6,
                    sizeof ip6_addr)) {
             return CT_FTP_CTL_INVALID;
         }
         break;
     case CT_FTP_MODE_PASSIVE:
-        *v6_addr_rep = conn_for_expectation->key.dst.addr;
+        *v6_addr_rep = conn_for_exp->key_node[CT_DIR_FWD].key.dst.addr;
         break;
     case CT_TFTP_MODE:
     default:
         OVS_NOT_REACHED();
     }
 
-    expectation_create(ct, port, conn_for_expectation,
+    expectation_create(ct, port, conn_for_exp,
                        !!(pkt->md.ct_state & CS_REPLY_DIR), false, false);
     return CT_FTP_CTL_INTEREST;
 }
@@ -3390,7 +3340,8 @@  handle_tftp_ctl(struct conntrack *ct,
                 long long now OVS_UNUSED, enum ftp_ctl_pkt ftp_ctl OVS_UNUSED,
                 bool nat OVS_UNUSED)
 {
-    expectation_create(ct, conn_for_expectation->key.src.port,
+    expectation_create(ct,
+                       conn_for_expectation->key_node[CT_DIR_FWD].key.src.port,
                        conn_for_expectation,
                        !!(pkt->md.ct_state & CS_REPLY_DIR), false, false);
 }