[ovs-dev] [patch v2 5/5] conntrack: Tighten handling of alg reverse conns.

[Darrell Ball]

Close a theoretical race delete/create corner case for alg
reverse conns and add debugging around this that may point to
an intentional exploit, unintentional problem or just a rare
condition.

Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
Signed-off-by: Darrell Ball <dlu998 at gmail.com>
---
 lib/conntrack.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 8deeec9..302a85c 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -67,8 +67,6 @@ enum ct_alg_mode {
     CT_TFTP_MODE,
 };
 
-void print_conn_info(struct conn *c, char *log_msg);
-
 static bool conn_key_extract(struct conntrack *, struct dp_packet *,
                              ovs_be16 dl_type, struct conn_lookup_ctx *,
                              uint16_t zone);
@@ -225,7 +223,7 @@ conn_key_cmp(const struct conn_key *key1, const struct conn_key *key2)
     return 1;
 }
 
-void
+static void
 print_conn_info(struct conn *c, char *log_msg)
 {
     VLOG_INFO("%s", log_msg);
@@ -830,6 +828,17 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
                     nc->nat_info->nat_action = NAT_ACTION_DST;
                 }
                 *conn_for_un_nat_copy = *nc;
+                ct_rwlock_wrlock(&ct->resources_lock);
+                bool new_insert = nat_conn_keys_insert(&ct->nat_conn_keys,
+                                                       conn_for_un_nat_copy,
+                                                       ct->hash_basis);
+                ct_rwlock_unlock(&ct->resources_lock);
+                if (!new_insert) {
+                    char *log_msg = xasprintf("Pre-existing alg "
+                                              "nat_conn_key");
+                    print_conn_info(conn_for_un_nat_copy, log_msg);
+                    free(log_msg);
+                }
             } else {
                 *conn_for_un_nat_copy = *nc;
                 ct_rwlock_wrlock(&ct->resources_lock);
@@ -931,8 +940,16 @@ create_un_nat_conn(struct conntrack *ct, struct conn *conn_for_un_nat_copy,
     struct conn *rev_conn = conn_lookup(ct, &nc->key, now);
 
     if (alg_un_nat) {
-        hmap_insert(&ct->buckets[un_nat_conn_bucket].connections,
-                    &nc->node, un_nat_hash);
+        if (!rev_conn) {
+            hmap_insert(&ct->buckets[un_nat_conn_bucket].connections,
+                        &nc->node, un_nat_hash);
+        } else {
+            char *log_msg = xasprintf("Unusual condition for un_nat conn "
+                                      "create for alg: rev_conn %p", rev_conn);
+            print_conn_info(nc, log_msg);
+            free(log_msg);
+            free(nc);
+        }
     } else {
         ct_rwlock_rdlock(&ct->resources_lock);
 
@@ -944,6 +961,11 @@ create_un_nat_conn(struct conntrack *ct, struct conn *conn_for_un_nat_copy,
             hmap_insert(&ct->buckets[un_nat_conn_bucket].connections,
                         &nc->node, un_nat_hash);
         } else {
+            char *log_msg = xasprintf("Unusual condition for un_nat conn "
+                                      "create: nat_conn_key_node/rev_conn "
+                                      "%p/%p", nat_conn_key_node, rev_conn);
+            print_conn_info(nc, log_msg);
+            free(log_msg);
             free(nc);
         }
         ct_rwlock_unlock(&ct->resources_lock);
-- 
1.9.1