diff mbox series

[Openvpn-devel] Fix "--inactive <time> 0" behavior for DCO

Message ID 20230322113408.2057-1-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Fix "--inactive <time> 0" behavior for DCO | expand

Commit Message

Lev Stipakov March 22, 2023, 11:34 a.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

Make sure we exit if <bytes> is 0 (not set) and no traffic
was produced.

According to man page and non-DCO --inactive implementation,
we exit if amount of bytes produced is less than <bytes> specified.
DCO implementation will do off-by-ones, but we consider it as okay
since we don't want to complicate code to handle both bytes=0 and >0
cases.

Change-Id: I4c089e486728a43bfe42596787c00355838311da
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gert Doering March 22, 2023, 12:48 p.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks.  I have not actually tested this, but we've discussed this 
at length before - so this is "obviously correct".

The problem here (for readers of the list only) is that "a single packet"
is sufficient to keep the connection active if "--inactive ... 0" is
used - and "0 bytes" will be "abort".  With "if (new_bytes >= 0)", the
condition is always true in the DCO path, even if 0 packets have been
seen, so it never triggers.

It does cause an off-by-one, so if the limit is "1000 bytes", on DCO
it will need 1001 bytes now, while non-DCO will be fine with 1000 bytes,
but we decided that generally this is not "a single byte" precise, but
"lots of traffic" or "not much", so this is acceptable for a simpler
condition that people can actually understand in half a year...

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

commit 6c64b46b15476351ca19f9a8f3cb8185aa2c7e07 (master)
commit a3c9458d233d35d2afdb866aaa602bebaabf2f59 (release/2.6)
Author: Lev Stipakov
Date:   Wed Mar 22 13:34:08 2023 +0200

     Fix '--inactive <time> 0' behavior for DCO

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230322113408.2057-1-lstipakov@gmail.com>
     URL: https://www.mail-archive.com/search?l=mid&q=20230322113408.2057-1-lstipakov@gmail.com
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 28a96f94..b3e0ba5d 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -481,7 +481,7 @@  check_inactivity_timeout(struct context *c)
         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)
+        if (new_bytes > c->options.inactivity_minimum_bytes)
         {
             c->c2.inactivity_bytes = tot_bytes;
             event_timeout_reset(&c->c2.inactivity_interval);