[Openvpn-devel,M] Change in openvpn[master]: Refinement to handle simultaneous UDP and TCP connections

Message ID f3ca71b16262c4789d0628c0b2ec109fe10263f8-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: Refinement to handle simultaneous UDP and TCP connections | expand

Commit Message

flichtenheld (Code Review) Sept. 23, 2024, 1:41 p.m. UTC
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/766?usp=email

to review the following change.


Change subject: Refinement to handle simultaneous UDP and TCP connections
......................................................................

Refinement to handle simultaneous UDP and TCP connections

MULTI: properly remove TCP instances by checking the multi_instance
       protocol instead of the global one.

TLS: set the tls_option xmit_hold bool value to true only in case of
     TCP child instance to avoid checking the global protocol
     value.

INIT: initialize the c->c2.event_set in the inherit_context_top()
      by default and not only in case of UDP since we could have
      multiple different sockets.

Change-Id: Ibe0614bf6ced210c136b2d13036048188196f7ef
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
---
M src/openvpn/forward.c
M src/openvpn/init.c
M src/openvpn/mtcp.c
M src/openvpn/mudp.c
M src/openvpn/multi.c
M src/openvpn/multi.h
M src/openvpn/multi_io.c
M src/openvpn/options.c
M src/openvpn/options.h
9 files changed, 143 insertions(+), 54 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/66/766/1

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 1357cad..c37095d 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -2358,7 +2358,7 @@ 
 #if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
     if (socket & EVENT_READ && c->c2.did_open_tun)
     {
-        dco_event_set(&c->c1.tuntap->dco, c->c2.event_set, (void *)&dco_shift);
+        dco_event_set(&c->c1.tuntap->dco, c->c2.event_set, (void *)dco_shift);
     }
 #endif
 
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index c817ce6..51b49f2 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -224,7 +224,7 @@ 
         if (streq(p[1], "HTTP"))
         {
             struct http_proxy_options *ho;
-            if (ce->proto != PROTO_TCP && ce->proto != PROTO_TCP_CLIENT)
+            if (ce->proto != PROTO_TCP && c->mode != CM_CHILD_TCP)
             {
                 msg(M_WARN, "HTTP proxy support only works for TCP based connections");
                 return false;
@@ -601,7 +601,10 @@ 
             ce_defined = false;
         }
 
+        int proto = c->options.ce.proto;
         c->options.ce = *ce;
+        c->options.ce.proto = proto;
+
 #ifdef ENABLE_MANAGEMENT
         if (ce_defined && management && management_query_remote_enabled(management))
         {
@@ -2600,7 +2603,7 @@ 
 
     if (found & OPT_P_EXPLICIT_NOTIFY)
     {
-        if (!proto_is_udp(c->options.ce.proto) && c->options.ce.explicit_exit_notification)
+        if (!proto_is_udp(c->c2.link_sockets[0]->info.proto) && c->options.ce.explicit_exit_notification)
         {
             msg(D_PUSH, "OPTIONS IMPORT: --explicit-exit-notify can only be used with --proto udp");
             c->options.ce.explicit_exit_notification = 0;
@@ -2760,14 +2763,21 @@ 
     int sec = 2;
     int backoff = 0;
 
-    switch (c->options.ce.proto)
+    switch (c->mode)
     {
-        case PROTO_TCP_SERVER:
-            sec = 1;
+        case CM_TOP:
+            if (has_udp_in_local_list(&c->options))
+            {
+                sec = c->options.ce.connect_retry_seconds;
+            }
+            else
+            {
+                sec = 1;
+            }
             break;
 
-        case PROTO_UDP:
-        case PROTO_TCP_CLIENT:
+        case CM_CHILD_UDP:
+        case CM_CHILD_TCP:
             sec = c->options.ce.connect_retry_seconds;
             break;
     }
@@ -2785,7 +2795,7 @@ 
     }
 
     /* Slow down reconnection after 5 retries per remote -- for TCP client or UDP tls-client only */
-    if (c->options.ce.proto == PROTO_TCP_CLIENT
+    if (c->mode == CM_CHILD_TCP
         || (c->options.ce.proto == PROTO_UDP && c->options.tls_client))
     {
         backoff = (c->options.unsuccessful_attempts / c->options.connection_list->len) - 4;
@@ -3265,7 +3275,21 @@ 
     to.server = options->tls_server;
     to.replay_window = options->replay_window;
     to.replay_time = options->replay_time;
-    to.tcp_mode = link_socket_proto_connection_oriented(options->ce.proto);
+
+    if (c->options.ce.local_list->len > 1)
+    {
+        for (int i = 0; i < c->options.ce.local_list->len; i++)
+        {
+            if (proto_is_dgram(c->options.ce.local_list->array[i]->proto))
+            {
+                to.tcp_mode = false;
+            }
+        }
+    }
+    else
+    {
+        to.tcp_mode = link_socket_proto_connection_oriented(c->options.ce.local_list->array[0]->proto);
+    }
     to.config_ciphername = c->options.ciphername;
     to.config_ncp_ciphers = c->options.ncp_ciphers;
     to.transition_window = options->transition_window;
@@ -3310,7 +3334,7 @@ 
 
     /* should we not xmit any packets until we get an initial
      * response from client? */
-    if (to.server && options->ce.proto == PROTO_TCP_SERVER)
+    if (to.server && c->mode == CM_CHILD_TCP)
     {
         to.xmit_hold = true;
     }
@@ -4214,20 +4238,13 @@ 
 #ifdef _WIN32
         msg(M_INFO, "NOTE: --fast-io is disabled since we are running on Windows");
 #else
-        if (!proto_is_udp(c->options.ce.proto))
+        if (c->options.shaper)
         {
-            msg(M_INFO, "NOTE: --fast-io is disabled since we are not using UDP");
+            msg(M_INFO, "NOTE: --fast-io is disabled since we are using --shaper");
         }
         else
         {
-            if (c->options.shaper)
-            {
-                msg(M_INFO, "NOTE: --fast-io is disabled since we are using --shaper");
-            }
-            else
-            {
-                c->c2.fast_io = true;
-            }
+            c->c2.fast_io = true;
         }
 #endif
     }
@@ -4924,6 +4941,7 @@ 
 
     /* options */
     dest->options = src->options;
+    dest->options.ce.proto = ls->info.proto;
     options_detach(&dest->options);
 
     dest->c2.event_set = src->c2.event_set;
@@ -5022,10 +5040,7 @@ 
     dest->c2.es_owned = false;
 
     dest->c2.event_set = NULL;
-    if (proto_is_dgram(src->options.ce.proto))
-    {
-        do_event_set_init(dest, false);
-    }
+    do_event_set_init(dest, false);
 
 #ifdef USE_COMP
     dest->c2.comp_context = NULL;
diff --git a/src/openvpn/mtcp.c b/src/openvpn/mtcp.c
index 1eb28ec..ba705a8 100644
--- a/src/openvpn/mtcp.c
+++ b/src/openvpn/mtcp.c
@@ -153,12 +153,19 @@ 
 {
     if (mi)
     {
-        mi->socket_set_called = true;
-        socket_set(mi->context.c2.link_sockets[0],
-                   m->multi_io->es,
-                   mbuf_defined(mi->tcp_link_out_deferred) ? EVENT_WRITE : EVENT_READ,
-                   &mi->ev_arg,
-                   &mi->tcp_rwflags);
+        if (proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto))
+        {
+            return;
+        }
+        else
+        {
+            mi->socket_set_called = true;
+            socket_set(mi->context.c2.link_sockets[0],
+                       m->multi_io->es,
+                       mbuf_defined(mi->tcp_link_out_deferred) ? EVENT_WRITE : EVENT_READ,
+                       &mi->ev_arg,
+                       &mi->tcp_rwflags);
+        }
     }
 }
 
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index f9efcd5..9edd6ad 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -194,6 +194,7 @@ 
     struct hash *hash = m->hash;
     real.proto = ls->info.proto;
     m->local.proto = real.proto;
+    m->hmac_reply_ls = ls;
 
     if (mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true)
         && m->top.c2.buf.len > 0)
@@ -320,15 +321,8 @@ 
         msg_set_prefix("Connection Attempt");
         m->top.c2.to_link = m->hmac_reply;
         m->top.c2.to_link_addr = m->hmac_reply_dest;
-        for (int i = 0; i < m->top.c1.link_sockets_num; i++)
-        {
-            if (!proto_is_dgram(m->top.c2.link_sockets[i]->info.proto))
-            {
-                continue;
-            }
-
-            process_outgoing_link(&m->top, m->top.c2.link_sockets[i]);
-        }
+        process_outgoing_link(&m->top, m->hmac_reply_ls);
+        m->hmac_reply_ls = NULL;
         m->hmac_reply_dest = NULL;
     }
 }
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 4a6dd52..a6cdb67 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -606,6 +606,7 @@ 
 
     ASSERT(!mi->halt);
     mi->halt = true;
+    bool is_dgram = proto_is_dgram(mi->context.c2.link_sockets[0]->info.proto);
 
     dmsg(D_MULTI_DEBUG, "MULTI: multi_close_instance called");
 
@@ -664,7 +665,7 @@ 
             mi->did_iroutes = false;
         }
 
-        if (m->multi_io && !proto_is_dgram(m->top.options.ce.proto))
+        if (!is_dgram)
         {
             multi_tcp_dereference_instance(m->multi_io, mi);
         }
@@ -3400,7 +3401,7 @@ 
             /* decrypt in instance context */
 
             perf_push(PERF_PROC_IN_LINK);
-            lsi = get_link_socket_info(c);
+            lsi = &ls->info;
             orig_buf = c->c2.buf.data;
             if (process_incoming_link_part1(c, lsi, floated))
             {
@@ -3851,7 +3852,7 @@ 
     while ((he = hash_iterator_next(&hi)))
     {
         struct multi_instance *mi = (struct multi_instance *) he->value;
-        if (!mi->halt)
+        if (!mi->halt && proto_is_dgram(mi->context.options.ce.proto))
         {
             send_control_channel_string(&mi->context, next_server ? "RESTART,[N]" : "RESTART", D_PUSH);
             multi_schedule_context_wakeup(m, mi);
@@ -3889,13 +3890,15 @@ 
         status_close(so);
         return false;
     }
-    else if (proto_is_dgram(m->top.options.ce.proto)
-             && is_exit_restart(m->top.sig->signal_received)
-             && (m->deferred_shutdown_signal.signal_received == 0)
-             && m->top.options.ce.explicit_exit_notification != 0)
+    else if (has_udp_in_local_list(&m->top.options))
     {
-        multi_push_restart_schedule_exit(m, m->top.options.ce.explicit_exit_notification == 2);
-        return false;
+        if (is_exit_restart(m->top.sig->signal_received)
+            && (m->deferred_shutdown_signal.signal_received == 0)
+            && m->top.options.ce.explicit_exit_notification != 0)
+        {
+            multi_push_restart_schedule_exit(m, m->top.options.ce.explicit_exit_notification == 2);
+            return false;
+        }
     }
     return true;
 }
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 65fd320..670826e 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -200,6 +200,7 @@ 
 
     struct buffer hmac_reply;
     struct link_socket_actual *hmac_reply_dest;
+    struct link_socket *hmac_reply_ls;
 
     /*
      * Timer object for stale route check
diff --git a/src/openvpn/multi_io.c b/src/openvpn/multi_io.c
index 57ed10e..269b045 100644
--- a/src/openvpn/multi_io.c
+++ b/src/openvpn/multi_io.c
@@ -142,6 +142,11 @@ 
                                      &m->top.c2.link_sockets[i]->ev_arg);
     }
 
+    if (has_udp_in_local_list(&m->top.options))
+    {
+        get_io_flags_udp(&m->top, m->multi_io, p2mp_iow_flags(m));
+    }
+
 #ifdef _WIN32
     if (tuntap_is_wintun(m->top.c1.tuntap))
     {
@@ -396,6 +401,10 @@ 
 multi_protocol_process_io(struct multi_context *m)
 {
     struct multi_protocol *multi_io = m->multi_io;
+    const unsigned int udp_status = multi_io->udp_flags;
+    const unsigned int mpp_flags = m->top.c2.fast_io
+                                   ? (MPP_CONDITIONAL_PRE_SELECT | MPP_CLOSE_ON_SIGNAL)
+                                   : (MPP_PRE_SELECT | MPP_CLOSE_ON_SIGNAL);
     int i;
 
     for (i = 0; i < multi_io->n_esr; ++i)
@@ -447,6 +456,39 @@ 
                         }
                         break;
                     }
+                    else
+                    {
+                        if (e->arg >= MULTI_N)
+                        {
+                            struct event_arg *ev_arg = (struct event_arg *)e->arg;
+                            if (ev_arg->type != EVENT_ARG_LINK_SOCKET)
+                            {
+                                multi_io->udp_flags = ES_ERROR;
+                                msg(D_LINK_ERRORS,
+                                    "MULTI PROTOCOL: io_work: non socket event delivered");
+                                break;
+                            }
+                        }
+                        else
+                        {
+                            ev_arg->pending = true;
+                        }
+
+                        if (udp_status & SOCKET_READ)
+                        {
+                            read_incoming_link(&m->top, ev_arg->u.ls);
+                            if (!IS_SIG(&m->top))
+                            {
+                                multi_process_incoming_link(m, NULL, mpp_flags,
+                                                            ev_arg->u.ls);
+                            }
+                        }
+                        else
+                        {
+                            multi_process_io_udp(m);
+                        }
+                        break;
+                    }
             }
         }
         else
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 42210da..a06813e 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -993,10 +993,13 @@ 
                         const struct connection_entry *e,
                         const int i)
 {
-    setenv_str_i(es, "proto", proto2ascii(e->proto, e->af, false), i);
-    /* expected to befor single socket contexts only */
-    setenv_str_i(es, "local", e->local_list->array[0]->local, i);
-    setenv_str_i(es, "local_port", e->local_list->array[0]->port, i);
+    for (int j = 0; j < e->local_list->len; j++)
+    {
+        setenv_str_i(es, "proto", proto2ascii(e->local_list->array[j]->proto, e->af, false), i);
+        /* expected to befor single socket contexts only */
+        setenv_str_i(es, "local", e->local_list->array[j]->local, i);
+        setenv_str_i(es, "local_port", e->local_list->array[j]->port, i);
+    }
     setenv_str_i(es, "remote", e->remote, i);
     setenv_str_i(es, "remote_port", e->remote_port, i);
 
@@ -2466,7 +2469,7 @@ 
     {
         struct local_entry *le = ce->local_list->array[i];
 
-        if (proto_is_net(ce->proto)
+        if (proto_is_net(le->proto)
             && string_defined_equal(le->local, ce->remote)
             && string_defined_equal(le->port, ce->remote_port))
         {
@@ -3828,6 +3831,12 @@ 
         options_postprocess_mutate_ce(o, o->connection_list->array[i]);
     }
 
+    if (!has_udp_in_local_list(o))
+    {
+        o->fast_io = false;
+        msg(M_INFO, "NOTE: --fast-io is disabled while using multi-socket");
+    }
+
     if (o->ce.local_list)
     {
         for (i = 0; i < o->ce.local_list->len; i++)
@@ -9722,3 +9731,19 @@ 
 err:
     gc_free(&gc);
 }
+
+bool
+has_udp_in_local_list(const struct options *options)
+{
+    if (options->ce.local_list)
+    {
+        for (int i = 0; i < options->ce.local_list->len; i++)
+        {
+            if (proto_is_dgram(options->ce.local_list->array[i]->proto))
+            {
+                return true;
+            }
+        }
+    }
+    return false;
+}
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index f3cf7e5..30bd5aa 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -917,6 +917,8 @@ 
 
 bool key_is_external(const struct options *options);
 
+bool has_udp_in_local_list(const struct options *options);
+
 /**
  * Returns whether the current configuration has dco enabled.
  */