diff mbox series

[Openvpn-devel] Support --inactive option for DCO

Message ID 20230315133808.1550-1-lstipakov@gmail.com
State Accepted
Headers
Series [Openvpn-devel] Support --inactive option for DCO |

Commit Message

Lev Stipakov March 15, 2023, 1:38 p.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

When DCO is in use, userland doesn't see any traffic
which breaks --inactive option.

Fix by adding inactivity check to inactivity timeout
callback. Get the cumulative tun bytes count (ping packets
are excluded) from DCO and compare it to the previous value
stored in c2.inactivity_bytes. Reset inactivity timer and
update c2.inactivity_bytes if amount of new bytes exceeds
inactivity_minimum_bytes, otherwise terminate session
due to inactivity.

Fixes https://github.com/OpenVPN/openvpn/issues/228

Currently works only on Windows, since we don't yet have
single peer stats implementation for Linux and FreeBSD.

Change-Id: Ib417b965bc4a2c17b51935b43c9627b106716526
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/dco_win.c |  2 ++
 src/openvpn/forward.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Heiko Hund March 16, 2023, 8:27 p.m. UTC | #1 
On Mittwoch, 15. März 2023 14:38:08 CET Lev Stipakov wrote:
> Change-Id: Ib417b965bc4a2c17b51935b43c9627b106716526
> Signed-off-by: Lev Stipakov <lev@openvpn.net>

Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Gert Doering March 17, 2023, 1:36 p.m. UTC | #2 
Stared a bit at the code until I understood the control flow, then
discussed a bit on IRC.  Code looks reasonable, and compiles, but
I did not test on an actual Windows system.

Both Linux and FreeBSD are currently broken wrt DCO and --inactive - this
patch does not make the situation worse, but does not improve it either.

Linux: no dco_get_peer_stats() or dco_get_peer_stats_multi() yet ("pending").

FreeBSD: no dco_get_peer_stats() yet, and this implementation never
queries the dco_get_peer_stats_multi() function.  Doing so might be
a bit of overkill ("one client might be expiring, please give me all!")
so the way counters are queried needs to be changed, or FreeBSD needs
to grow a "query peer stats for single client" function on the kernel side
(+ userland implementation).  I might look into this :-)


As a side effect this change makes dco_get_peer_stats() update
c2.tun_read/write_bytes, which is relevant for F2/SIGUSR2 status
printing (but notably not for --server status files).

As discussed, I've extended the check_inactivity_timeout() comment
quite a bit, explaining the different control flows with and without DCO.

Your patch has been applied to the master and release/2.6 branch.

commit 514eefb14ace41a5790e59b81654d1d5eed60670 (master)
commit fd71bce651d5f606d3c1d430c7c0911fe119f075 (release/2.6)
Author: Lev Stipakov
Date:   Wed Mar 15 15:38:08 2023 +0200

     Support --inactive option for DCO

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
     Message-Id: <20230315133808.1550-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26421.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 0931fb30..aae6b4b5 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -431,6 +431,8 @@  dco_get_peer_stats(struct context *c)
 
     c->c2.dco_read_bytes = stats.TransportBytesReceived;
     c->c2.dco_write_bytes = stats.TransportBytesSent;
+    c->c2.tun_read_bytes = stats.TunBytesReceived;
+    c->c2.tun_write_bytes = stats.TunBytesSent;
 
     return 0;
 }
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 257c7c75..923c04f2 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -464,6 +464,20 @@  check_add_routes(struct context *c)
 static void
 check_inactivity_timeout(struct context *c)
 {
+    if (dco_enabled(&c->options) && dco_get_peer_stats(c) == 0)
+    {
+        int64_t tot_bytes = c->c2.tun_read_bytes + c->c2.tun_write_bytes;
+        int64_t new_bytes = tot_bytes - c->c2.inactivity_bytes;
+
+        if (new_bytes >= c->options.inactivity_minimum_bytes)
+        {
+            c->c2.inactivity_bytes = tot_bytes;
+            event_timeout_reset(&c->c2.inactivity_interval);
+
+            return;
+        }
+    }
+
     msg(M_INFO, "Inactivity timeout (--inactive), exiting");
     register_signal(c->sig, SIGTERM, "inactive");
 }