diff mbox series

[Openvpn-devel,v17] mroute: adapt to new protocol handling and hashing improvements

Message ID 20250124205135.18765-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v17] mroute: adapt to new protocol handling and hashing improvements | expand

Commit Message

Gert Doering Jan. 24, 2025, 8:51 p.m. UTC 
From: Gianmarco De Gregori <gianmarco@mandelbit.com>

Repurposing an unused field and renaming it to 'proto'
instead of introducing a new field. The hashing now
begins at the 'proto' field rather than the 'type'
field. Additionally, the changes ensure that the
correct protocol is consistently used with virtual
addresses ensuring alignment.

Change-Id: Ic66eccb5058fe9c0fae64d8e2ca88728068a92ab
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

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/+/765
This mail reflects revision 17 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Jan. 25, 2025, 9:57 a.m. UTC | #1 
This is also basically only "adding infrastructure" - the mroute hashing
is used to find a peer based on IP address, and with UDP+TCP sockets
in use at the same time, a collision "same remote IP+Port, different
proto" is possible.

Now, the hashing is not strictly needed for incoming packets on a
TCP connection ("there is only one client on that socket"), but the
currently-existing infrastructure is not using the "socket -> mi"
shortcut, at least not everywhere (e.g. multi.c: multi_close_instance()
which is agnostic on UDP/TCP, and crashed nicely on 765 #v16...).

Thus, we need this ;-)

Stared a lot at this code from v1...v17, Gian/Antonio reworked the
patch quite a bit, and the server side testbed found more issues... but
now we're at a point where the code looks good and all the test instances
are well-behaved.  In it goes.

Your patch has been applied to the master branch.

commit dda93f30411eec3457a48e94ef599e3f034c87e5
Author: Gianmarco De Gregori
Date:   Fri Jan 24 21:51:35 2025 +0100

     mroute: adapt to new protocol handling and hashing improvements

     Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20250124205135.18765-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg30579.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index 80e18b7..74923cf 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -454,6 +454,7 @@ 
                 buf_printf(&out, "UNKNOWN");
                 break;
         }
+        buf_printf(&out, "|%d", maddr.proto);
         return BSTR(&out);
     }
     else
diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 8b457d4..2659695 100644
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -74,7 +74,7 @@ 
 
 struct mroute_addr {
     uint8_t len;    /* length of address */
-    uint8_t unused;
+    uint8_t proto;
     uint8_t type;   /* MR_ADDR/MR_WITH flags */
     uint8_t netbits; /* number of bits in network part of address,
                       * valid if MR_WITH_NETBITS is set */
@@ -183,6 +183,15 @@ 
 {
     unsigned int ret = 0;
     verify_align_4(buf);
+
+    /*
+     * Since we don't really need the protocol on vaddresses for internal VPN
+     * payload packets, make sure we have the same value to avoid hashing insert
+     * and search issues.
+     */
+    src->proto = 0;
+    dest->proto = src->proto;
+
     if (tunnel_type == DEV_TYPE_TUN)
     {
         ret = mroute_extract_addr_ip(src, dest, buf);
@@ -201,6 +210,10 @@ 
     {
         return false;
     }
+    if (a1->proto != a2->proto)
+    {
+        return false;
+    }
     if (a1->netbits != a2->netbits)
     {
         return false;
@@ -216,13 +229,13 @@ 
 mroute_addr_hash_ptr(const struct mroute_addr *a)
 {
     /* NOTE: depends on ordering of struct mroute_addr */
-    return (uint8_t *) &a->type;
+    return (uint8_t *) &a->proto;
 }
 
 static inline uint32_t
 mroute_addr_hash_len(const struct mroute_addr *a)
 {
-    return (uint32_t) a->len + 2;
+    return (uint32_t) a->len + 3;
 }
 
 static inline void
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index 6d1d5a0..62ed044 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -56,6 +56,7 @@ 
     mi = multi_create_instance(m, NULL, ls);
     if (mi)
     {
+        mi->real.proto = ls->info.proto;
         struct hash_element *he;
         const uint32_t hv = hash_value(hash, &mi->real);
         struct hash_bucket *bucket = hash_bucket(hash, hv);
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 6137578..4f2bbd7 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -192,6 +192,7 @@ 
     struct mroute_addr real = {0};
     struct multi_instance *mi = NULL;
     struct hash *hash = m->hash;
+    real.proto = ls->info.proto;
 
     if (mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true)
         && m->top.c2.buf.len > 0)
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 3f55dd7..9c8c014 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -794,6 +794,7 @@ 
         {
             goto err;
         }
+        mi->real.proto = ls->info.proto;
         generate_prefix(mi);
     }
 
@@ -1243,6 +1244,7 @@ 
     CLEAR(remote_si);
     remote_si.addr.in4.sin_family = AF_INET;
     remote_si.addr.in4.sin_addr.s_addr = htonl(a);
+    addr.proto = 0;
     ASSERT(mroute_extract_openvpn_sockaddr(&addr, &remote_si, false));
 
     if (netbits >= 0)
@@ -3548,7 +3550,6 @@ 
         const int dev_type = TUNNEL_TYPE(m->top.c1.tuntap);
         int16_t vid = 0;
 
-
 #ifdef MULTI_DEBUG_EVENT_LOOP
         printf("TUN -> TCP/UDP [%d]\n", BLEN(&m->top.c2.buf));
 #endif