[Openvpn-devel] multi: don't call DCO APIs if DCO is disabled

Message ID 20230321102842.10780-1-a@unstable.cc
State Accepted
Headers show
Series [Openvpn-devel] multi: don't call DCO APIs if DCO is disabled | expand

Commit Message

Antonio Quartulli March 21, 2023, 10:28 a.m. UTC
The agreement with the DCO submodule is that no API should be called if
DCO is actually disabled. For this reason, every invocation must happen
only after having checked that dco_enabled() returns true.

Add missing checks before invoking dco_get_peer_stats_multi()

Reported-by: Lev Stipakov <lev@openvpn.net>
Signed-off-by: Antonio Quartulli <a@unstable.cc>
---
 src/openvpn/multi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Lev Stipakov March 21, 2023, 11:54 a.m. UTC | #1
LGTM.

Without this patch and with Linux DCO peer stats openvpn crashes, with
this patch it doesn't.

Acked-by: Lev Stipakov <lstipakov@gmail.com>

ti 21. maalisk. 2023 klo 12.30 Antonio Quartulli (a@unstable.cc) kirjoitti:
>
> The agreement with the DCO submodule is that no API should be called if
> DCO is actually disabled. For this reason, every invocation must happen
> only after having checked that dco_enabled() returns true.
>
> Add missing checks before invoking dco_get_peer_stats_multi()
>
> Reported-by: Lev Stipakov <lev@openvpn.net>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> ---
>  src/openvpn/multi.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 53c17b3a..1f0a9c01 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -549,7 +549,10 @@ multi_del_iroutes(struct multi_context *m,
>  static void
>  setenv_stats(struct multi_context *m, struct context *c)
>  {
> -    dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
> +    if (dco_enabled(&m->top.options))
> +    {
> +        dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
> +    }
>
>      setenv_counter(c->c2.es, "bytes_received", c->c2.link_read_bytes + c->c2.dco_read_bytes);
>      setenv_counter(c->c2.es, "bytes_sent", c->c2.link_write_bytes + c->c2.dco_write_bytes);
> @@ -849,7 +852,10 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
>
>          status_reset(so);
>
> -        dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
> +        if (dco_enabled(&m->top.options))
> +        {
> +            dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
> +        }
>
>          if (version == 1)
>          {
> --
> 2.39.2
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering March 21, 2023, 3:15 p.m. UTC | #2
I have not tested this extensively (as in "instrument the functions
not called anymore if --disable-dco is in use"), just ran some basic
tests on FreeBSD 14 with DCO, and "counters with DCO" still work,
as does --inactive (with a "this many bytes" specification).

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

commit 891c71db5e26291b19885b9a5ae5c72011b86658 (master)
commit 8f503708ed954ff3ae43357bd9f59809581a1381 (release/2.6)
Author: Antonio Quartulli
Date:   Tue Mar 21 11:28:42 2023 +0100

     multi: don't call DCO APIs if DCO is disabled

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20230321102842.10780-1-a@unstable.cc>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26458.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 53c17b3a..1f0a9c01 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -549,7 +549,10 @@  multi_del_iroutes(struct multi_context *m,
 static void
 setenv_stats(struct multi_context *m, struct context *c)
 {
-    dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
+    if (dco_enabled(&m->top.options))
+    {
+        dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
+    }
 
     setenv_counter(c->c2.es, "bytes_received", c->c2.link_read_bytes + c->c2.dco_read_bytes);
     setenv_counter(c->c2.es, "bytes_sent", c->c2.link_write_bytes + c->c2.dco_write_bytes);
@@ -849,7 +852,10 @@  multi_print_status(struct multi_context *m, struct status_output *so, const int
 
         status_reset(so);
 
-        dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
+        if (dco_enabled(&m->top.options))
+        {
+            dco_get_peer_stats_multi(&m->top.c1.tuntap->dco, m);
+        }
 
         if (version == 1)
         {