From: Lev Stipakov Date: Tue, 2 Sep 2025 10:36:01 +0000 (+0200) Subject: Refactor management bytecount tracking X-Git-Tag: v2.7_beta1~8 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=da309c1e8bed7b9c25e800499400c8ad908276db;p=thirdparty%2Fopenvpn.git 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 Acked-by: Gert Doering Message-Id: <20250902103606.22181-1-gert@greenie.muc.de> URL: https://sourceforge.net/p/openvpn/mailman/message/59228150/ Signed-off-by: Gert Doering --- diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 75ca9d5c5..03b6a0c6b 100644 --- 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 aed04f54b..52b0e5e25 100644 --- 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 e9e1f95ef..0b064cfe5 100644 --- 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 e1ce32ab0..f1abdbe85 100644 --- 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