[Openvpn-devel,v20] Split multi_get_create_instance_udp into data and control parts

Message ID 20260813184950.6709-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v20] Split multi_get_create_instance_udp into data and control parts |

Commit Message

Gert Doering Aug. 13, 2026, 6:49 p.m. UTC
  From: Arne Schwabe <arne@rfc2549.org>

This currently leads to a bit of code duplication but this refactoring
will make the follow up patches cleaner and better to understand when
the control channel lookup will be changed to use session ids instead of
IP addresses.

Change-Id: I8e9923b51b77f184c7d49d697004cb02d2b5cfc3
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1726
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1726
This mail reflects revision 20 of this Change.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <frank@lichtenheld.com>
  

Patch

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 08d79c9..632b064 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -218,8 +218,7 @@ 
 static struct multi_instance *
 handle_connection_attempt(struct multi_context *m,
                           struct link_socket *sock,
-                          struct mroute_addr *real,
-                          struct hash_bucket *bucket)
+                          struct mroute_addr *real)
 {
     struct hash *hash = m->hash;
     struct tls_pre_decrypt_state state = { 0 };
@@ -250,7 +249,9 @@ 
             if (mi)
             {
                 const uint64_t hv = hash_value(hash, real);
+                struct hash_bucket *bucket = hash_bucket(hash, hv);
                 hash_add_fast(hash, bucket, &mi->real, hv, mi);
+
                 mi->did_real_hash = true;
                 multi_assign_peer_id(m, mi);
 
@@ -299,6 +300,19 @@ 
     return NULL;
 }
 
+struct multi_instance *
+multi_get_instance_udp_control(struct multi_context *m, struct link_socket *sock)
+{
+    struct mroute_addr real = { 0 };
+    real.proto = sock->info.proto;
+
+    if (mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true) && m->top.c2.buf.len > 0)
+    {
+        return multi_get_instance_udp_real(m, &real);
+    }
+
+    return NULL;
+}
 
 /**
  * Get a client instance based on real address.  If
@@ -306,72 +320,101 @@ 
  * maintaining real address hash table atomicity.
  */
 struct multi_instance *
-multi_get_create_instance_udp(struct multi_context *m, bool *floated, struct link_socket *sock)
+multi_get_instance_udp_data(struct multi_context *m, bool *floated, struct mroute_addr *real, struct link_socket *sock)
 {
-    struct gc_arena gc = gc_new();
-    struct mroute_addr real = { 0 };
     struct multi_instance *mi = NULL;
-    struct hash *hash = m->hash;
-    real.proto = sock->info.proto;
 
-    if (mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true) && m->top.c2.buf.len > 0)
+    uint8_t *ptr = BPTR(&m->top.c2.buf);
+    uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+    bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
+    bool peer_id_disabled = false;
+
+    /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
+    if (v2)
     {
-        const uint64_t hv = hash_value(hash, &real);
-        struct hash_bucket *bucket = hash_bucket(hash, hv);
-        uint8_t *ptr = BPTR(&m->top.c2.buf);
-        uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
-        bool v2 = (op == P_DATA_V2) && (m->top.c2.buf.len >= (1 + 3));
-        bool peer_id_disabled = false;
+        uint32_t peer_id = ((uint32_t)ptr[1] << 16) | ((uint32_t)ptr[2] << 8) | ((uint32_t)ptr[3]);
+        peer_id_disabled = (peer_id == MAX_PEER_ID);
 
-        /* make sure buffer has enough length to read opcode (1 byte) and peer-id (3 bytes) */
-        if (v2)
+        if (!peer_id_disabled && (peer_id < m->max_clients) && (m->instances[peer_id]))
         {
-            uint32_t peer_id = ((uint32_t)ptr[1] << 16) | ((uint32_t)ptr[2] << 8) | ((uint32_t)ptr[3]);
-            peer_id_disabled = (peer_id == MAX_PEER_ID);
-
-            if (!peer_id_disabled && (peer_id < m->max_clients) && m->instances[peer_id])
+            /* Floating on TCP will never be possible, so ensure we only process
+             * UDP clients */
+            if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto
+                == sock->info.proto)
             {
-                /* Floating on TCP will never be possible, so ensure we only process
-                 * UDP clients */
-                if (m->instances[peer_id]->context.c2.link_sockets[0]->info.proto
-                    == sock->info.proto)
-                {
-                    mi = m->instances[peer_id];
-                    *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
+                mi = m->instances[peer_id];
+                *floated = !link_socket_actual_match(&mi->context.c2.from, &m->top.c2.from);
 
-                    if (*floated)
-                    {
-                        /* reset prefix, since here we are not sure peer is the one it claims to be
-                         */
-                        ungenerate_prefix(mi);
-                        msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
-                            mroute_addr_print(&real, &gc));
-                    }
+                if (*floated)
+                {
+                    /* reset prefix, since here we are not sure peer is the one it claims to be
+                     */
+                    ungenerate_prefix(mi);
+                    struct gc_arena gc = gc_new();
+                    msg(D_MULTI_MEDIUM, "Float requested for peer %" PRIu32 " to %s", peer_id,
+                        mroute_addr_print(real, &gc));
+                    gc_free(&gc);
                 }
+                return mi;
             }
         }
-        if (!v2 || peer_id_disabled)
-        {
-            mi = multi_get_instance_udp_real(m, &real);
-        }
+    }
+    if (!v2 || peer_id_disabled)
+    {
+        return multi_get_instance_udp_real(m, real);
+    }
+    return NULL;
+}
 
-        /* we have no existing multi instance for this connection */
-        if (!mi)
-        {
-            mi = handle_connection_attempt(m, sock, &real, bucket);
-        }
-
-#ifdef ENABLE_DEBUG
-        if (check_debug_level(D_MULTI_DEBUG))
-        {
-            const char *status = mi ? "[ok]" : "[failed]";
-
-            dmsg(D_MULTI_DEBUG, "GET INST BY REAL: %s %s", mroute_addr_print(&real, &gc), status);
-        }
-#endif
+struct multi_instance *
+multi_get_create_instance_udp(struct multi_context *m, bool *floated, struct link_socket *sock)
+{
+    /* If the buffer is empty, the packet has no op code and can be neither
+     * a (valid) data nor control packet */
+    if (m->top.c2.buf.len <= 0)
+    {
+        return NULL;
     }
 
-    gc_free(&gc);
+    uint8_t *ptr = BPTR(&m->top.c2.buf);
+    uint8_t op = ptr[0] >> P_OPCODE_SHIFT;
+
+    struct mroute_addr real = { 0 };
+    real.proto = sock->info.proto;
+
+    if (!mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true))
+    {
+        return NULL;
+    }
+
+    struct multi_instance *mi = NULL;
+    if (op == P_DATA_V1 || op == P_DATA_V2)
+    {
+        mi = multi_get_instance_udp_data(m, floated, &real, sock);
+    }
+    else
+    {
+        mi = multi_get_instance_udp_control(m, sock);
+
+        /* we have no existing multi instance for this connection, control
+         * packets can create a session. Data packets cannot */
+        if (!mi)
+        {
+            mi = handle_connection_attempt(m, sock, &real);
+        }
+    }
+
+#ifdef ENABLE_DEBUG
+    if (check_debug_level(D_MULTI_DEBUG))
+    {
+        struct gc_arena gc = gc_new();
+        const char *status = mi ? "[ok]" : "[failed]";
+
+        dmsg(D_MULTI_DEBUG, "GET INST BY REAL/SID: %s %s", mroute_addr_print(&real, &gc), status);
+        gc_free(&gc);
+    }
+#endif
+
     ASSERT(!(mi && mi->halt));
     return mi;
 }