Refactor management bytecount tracking

There are few issues with it:

 - when using DCO, the server part doesn't output BYTECOUNT_CLI since
   process_incoming_link_part1/process_outgoing_link are not called

 - when using DCO, the server part applies bytecount timer to the each
   connection, unneccessary making too many calls to the kernel and also
   uses incorrect BYTECOUNT output.

 - client part outputs counters using timer, server part utilizes
   traffic activity -> inconsistency

Following changes have been made:

 - Use timer to output counters in client and server mode. Code which
   deals with bytecount on traffic activity has been removed. This unifies
   DCO and non-DCO, as well as client and server mode

 - In server mode, peers stats are fetched with the single ioctl call

 - Per-packet stats are not persisted anymore in the client mode during
   traffic activity. Instead cumulative stats (including DCO stats) are
   persisted when the session closes.

GitHub: closes OpenVPN/openvpn#820

Change-Id: I43a93f0d84f01fd808a64115e1b8c3b806706491
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20250902103606.22181-1-gert@greenie.muc.de>
URL: https://sourceforge.net/p/openvpn/mailman/message/59228150/
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 75ca9d5c506c2be1141b3587df35b1ad560d1886..03b6a0c6b22690718bc2d390de4570cd3e8bc58b 100644 (file)
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -818,7 +818,7 @@ process_coarse_timers(struct context *c)
 #ifdef ENABLE_MANAGEMENT
     if (management)
     {
-        management_check_bytecount(c, management, &c->c2.timeval);
+        management_check_bytecount_client(c, management, &c->c2.timeval);
     }
 #endif /* ENABLE_MANAGEMENT */
 }
@@ -998,14 +998,6 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
         }
 #endif
         c->c2.original_recv_size = c->c2.buf.len;
-#ifdef ENABLE_MANAGEMENT
-        if (management)
-        {
-            management_bytes_client(management, c->c2.buf.len, 0);
-            management_bytes_server(management, &c->c2.link_read_bytes, &c->c2.link_write_bytes,
-                                    &c->c2.mda_context);
-        }
-#endif
     }
     else
     {
@@ -1822,14 +1814,6 @@ process_outgoing_link(struct context *c, struct link_socket *sock)
                 {
                     mmap_stats->link_write_bytes = link_write_bytes_global;
                 }
-#endif
-#ifdef ENABLE_MANAGEMENT
-                if (management)
-                {
-                    management_bytes_client(management, 0, size);
-                    management_bytes_server(management, &c->c2.link_read_bytes,
-                                            &c->c2.link_write_bytes, &c->c2.mda_context);
-                }
 #endif
             }
         }

diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index aed04f54b3911ac05a417af35245e2ff5cc5e59c..52b0e5e25c1dc843cd035b53aa0bcefec87f60b9 100644 (file)
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -41,6 +41,7 @@
 #include "manage.h"
 #include "openvpn.h"
 #include "dco.h"
+#include "multi.h"
 
 #include "memdbg.h"
 
@@ -517,29 +518,27 @@ man_bytecount(struct management *man, const int update_seconds)
 }
 
 static void
-man_bytecount_output_client(struct management *man, counter_type dco_read_bytes,
-                            counter_type dco_write_bytes)
+man_bytecount_output_client(counter_type bytes_in_total, counter_type bytes_out_total)
 {
     char in[32];
     char out[32];
 
     /* do in a roundabout way to work around possible mingw or mingw-glibc bug */
-    snprintf(in, sizeof(in), counter_format, man->persist.bytes_in + dco_read_bytes);
-    snprintf(out, sizeof(out), counter_format, man->persist.bytes_out + dco_write_bytes);
+    snprintf(in, sizeof(in), counter_format, bytes_in_total);
+    snprintf(out, sizeof(out), counter_format, bytes_out_total);
     msg(M_CLIENT, ">BYTECOUNT:%s,%s", in, out);
 }
 
-void
-man_bytecount_output_server(const counter_type *bytes_in_total, const counter_type *bytes_out_total,
+static void
+man_bytecount_output_server(const counter_type bytes_in_total, const counter_type bytes_out_total,
                             struct man_def_auth_context *mdac)
 {
     char in[32];
     char out[32];
     /* do in a roundabout way to work around possible mingw or mingw-glibc bug */
-    snprintf(in, sizeof(in), counter_format, *bytes_in_total);
-    snprintf(out, sizeof(out), counter_format, *bytes_out_total);
+    snprintf(in, sizeof(in), counter_format, bytes_in_total);
+    snprintf(out, sizeof(out), counter_format, bytes_out_total);
     msg(M_CLIENT, ">BYTECOUNT_CLI:%lu,%s,%s", mdac->cid, in, out);
-    mdac->bytecount_last_update = now;
 }
 
 static void
@@ -4065,42 +4064,82 @@ management_sleep(const int n)
 }
 
 void
-management_check_bytecount(struct context *c, struct management *man, struct timeval *timeval)
+management_check_bytecount_client(struct context *c, struct management *man, struct timeval *timeval)
 {
-    if (event_timeout_trigger(&man->connection.bytecount_update_interval, timeval, ETT_DEFAULT))
+    if (man->persist.callback.flags & MCF_SERVER)
     {
-        counter_type dco_read_bytes = 0;
-        counter_type dco_write_bytes = 0;
+        return;
+    }
 
+    if (event_timeout_trigger(&man->connection.bytecount_update_interval, timeval, ETT_DEFAULT))
+    {
         if (dco_enabled(&c->options))
         {
             if (dco_get_peer_stats(c, true) < 0)
             {
                 return;
             }
+        }
+
+        man_bytecount_output_client(c->c2.dco_read_bytes + man->persist.bytes_in + c->c2.link_read_bytes,
+                                    c->c2.dco_write_bytes + man->persist.bytes_out + c->c2.link_write_bytes);
+    }
+}
+
+void
+management_check_bytecount_server(struct multi_context *multi)
+{
+    if (!(management->persist.callback.flags & MCF_SERVER))
+    {
+        return;
+    }
 
-            dco_read_bytes = c->c2.dco_read_bytes;
-            dco_write_bytes = c->c2.dco_write_bytes;
+    struct timeval null;
+    CLEAR(null);
+    if (event_timeout_trigger(&management->connection.bytecount_update_interval, &null, ETT_DEFAULT))
+    {
+        /* fetch counters from dco */
+        if (dco_enabled(&multi->top.options))
+        {
+            if (dco_get_peer_stats_multi(&multi->top.c1.tuntap->dco, true) < 0)
+            {
+                return;
+            }
         }
 
-        if (!(man->persist.callback.flags & MCF_SERVER))
+        /* iterate over peers and report counters for each connected peer */
+        struct hash_iterator hi;
+        struct hash_element *he;
+        hash_iterator_init(multi->hash, &hi);
+        while ((he = hash_iterator_next(&hi)))
         {
-            man_bytecount_output_client(man, dco_read_bytes, dco_write_bytes);
+            struct multi_instance *mi = (struct multi_instance *)he->value;
+            struct context_2 *c2 = &mi->context.c2;
+
+            if ((c2->mda_context.flags & (DAF_CONNECTION_ESTABLISHED | DAF_CONNECTION_CLOSED)) == DAF_CONNECTION_ESTABLISHED)
+            {
+                man_bytecount_output_server(c2->dco_read_bytes + c2->link_read_bytes, c2->dco_write_bytes + c2->link_write_bytes, &c2->mda_context);
+            }
         }
+        hash_iterator_free(&hi);
     }
 }
 
-/* DCO resets stats on reconnect. Since client expects stats
- * to be preserved across reconnects, we need to save DCO
+/* context_2 stats are reset on reconnect. Since client expects stats
+ * to be preserved across reconnects, we need to save context_2
  * stats before tearing the tunnel down.
  */
 void
 man_persist_client_stats(struct management *man, struct context *c)
 {
-    /* no need to raise SIGUSR1 since we are already closing the instance */
+    man->persist.bytes_in += c->c2.link_read_bytes;
+    man->persist.bytes_out += c->c2.link_write_bytes;
+
+    /* no need to raise SIGUSR1 on error since we are already closing the instance */
     if (dco_enabled(&c->options) && (dco_get_peer_stats(c, false) == 0))
     {
-        management_bytes_client(man, c->c2.dco_read_bytes, c->c2.dco_write_bytes);
+        man->persist.bytes_in += c->c2.dco_read_bytes;
+        man->persist.bytes_out += c->c2.dco_write_bytes;
     }
 }
 

diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h
index e9e1f95ef84e004481f12d9f83540d89609c49e8..0b064cfe5fa95b2a23076a0e073aa43423a84994 100644 (file)
--- a/src/openvpn/manage.h
+++ b/src/openvpn/manage.h
@@ -70,8 +70,6 @@ struct man_def_auth_context
     unsigned int flags;
 
     unsigned int mda_key_id_counter;
-
-    time_t bytecount_last_update;
 };
 
 /*
@@ -492,34 +490,9 @@ void management_auth_token(struct management *man, const char *token);
  * These functions drive the bytecount in/out counters.
  */
 
-void management_check_bytecount(struct context *c, struct management *man, struct timeval *timeval);
-
-static inline void
-management_bytes_client(struct management *man, const int size_in, const int size_out)
-{
-    if (!(man->persist.callback.flags & MCF_SERVER))
-    {
-        man->persist.bytes_in += size_in;
-        man->persist.bytes_out += size_out;
-    }
-}
+void management_check_bytecount_client(struct context *c, struct management *man, struct timeval *timeval);
 
-void man_bytecount_output_server(const counter_type *bytes_in_total,
-                                 const counter_type *bytes_out_total,
-                                 struct man_def_auth_context *mdac);
-
-static inline void
-management_bytes_server(struct management *man, const counter_type *bytes_in_total,
-                        const counter_type *bytes_out_total, struct man_def_auth_context *mdac)
-{
-    if (man->connection.bytecount_update_seconds > 0
-        && now >= mdac->bytecount_last_update + man->connection.bytecount_update_seconds
-        && (mdac->flags & (DAF_CONNECTION_ESTABLISHED | DAF_CONNECTION_CLOSED))
-               == DAF_CONNECTION_ESTABLISHED)
-    {
-        man_bytecount_output_server(bytes_in_total, bytes_out_total, mdac);
-    }
-}
+void management_check_bytecount_server(struct multi_context *multi);
 
 void man_persist_client_stats(struct management *man, struct context *c);
 

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index e1ce32ab083346abdf3fdf553ffe10ec6a3e9ad5..f1abdbe85c8fbc39f0043fbe4a3980397419adc3 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -3812,6 +3812,13 @@ multi_process_per_second_timers_dowork(struct multi_context *m)
     {
         check_stale_routes(m);
     }
+
+#ifdef ENABLE_MANAGEMENT
+    if (management)
+    {
+        management_check_bytecount_server(m);
+    }
+#endif /* ENABLE_MANAGEMENT */
 }
 
 static void