[Openvpn-devel,1/4] Read DCO traffic stats from the kernel

From: Kristof Provost <kp@FreeBSD.org>

When DCO is active userspace doesn't see all of the traffic, so when we
access these stats we must update them.

Retrieve kernel statistics every time we access the
link_(read|write)_bytes values.

Introduce a dco_(read|write)_bytes so that we don't clobber the existing
statistics, which still count control packets, sent or received directly
through the socket.

Signed-off-by: Kristof Provost <kprovost@netgate.com>
---
 src/openvpn/dco.h              |  8 ++++
 src/openvpn/dco_freebsd.c      | 78 ++++++++++++++++++++++++++++++++++
 src/openvpn/dco_linux.c        |  7 +++
 src/openvpn/dco_win.c          |  7 +++
 src/openvpn/multi.c            | 30 +++++++------
 src/openvpn/openvpn.h          |  2 +
 src/openvpn/ovpn_dco_freebsd.h |  1 +
 7 files changed, 120 insertions(+), 13 deletions(-)

Hi,

This is good - I need an API to get stats to make openvpn-gui show
those (via the management interface).

I am not too much into FreeBSD parts, but

> +    hash_iterator_init(m->hash, &hi);
> +
> +    while ((he = hash_iterator_next(&hi)))
> +    {
> +        struct multi_instance *mi = (struct multi_instance *) he->value;
> +
> +        if (mi->context.c2.tls_multi->peer_id != peerid)
> +            continue;

Shouldn't we use m->instances[peerid] instead of iterating over m->hash?

from multi.h:

struct multi_context {
    struct multi_instance **instances;  /**< Array of multi_instances.
An instance can be
                                         * accessed using peer-id as
an index. */

Am 12.12.22 um 13:34 schrieb Lev Stipakov:
> Hi,
> 
> This is good - I need an API to get stats to make openvpn-gui show
> those (via the management interface).
> 
> I am not too much into FreeBSD parts, but
> 
>> +    hash_iterator_init(m->hash, &hi);
>> +
>> +    while ((he = hash_iterator_next(&hi)))
>> +    {
>> +        struct multi_instance *mi = (struct multi_instance *) he->value;
>> +
>> +        if (mi->context.c2.tls_multi->peer_id != peerid)
>> +            continue;
> 
> Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
> 

Yes and a check that kernel does not give something > max_peer

Arne

Hi,

On Mon, Dec 12, 2022 at 01:50:51PM +0100, Arne Schwabe wrote:
> > I am not too much into FreeBSD parts, but
> > 
> >> +    hash_iterator_init(m->hash, &hi);
> >> +
> >> +    while ((he = hash_iterator_next(&hi)))
> >> +    {
> >> +        struct multi_instance *mi = (struct multi_instance *) he->value;
> >> +
> >> +        if (mi->context.c2.tls_multi->peer_id != peerid)
> >> +            continue;
> > 
> > Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
> 
> Yes and a check that kernel does not give something > max_peer

I agree, doing the iterator here for every single active peer when we
already have the array.  But then, of course, p2p peer-id will be 
"something random", not constrained by max-clients.

Anyway, we have a procedural problem here - kp@ is on vacation for a
few weeks now, so we won't see a v2 of this patch any time soon.

Do you think the patch is ready to be merged, "as is", with not-so-good
efficiency?  One of you could then send a followup patch changing this
to use the array accessor.

gert

Am 12.12.22 um 14:30 schrieb Gert Doering:
> Hi,
> 
> On Mon, Dec 12, 2022 at 01:50:51PM +0100, Arne Schwabe wrote:
>>> I am not too much into FreeBSD parts, but
>>>
>>>> +    hash_iterator_init(m->hash, &hi);
>>>> +
>>>> +    while ((he = hash_iterator_next(&hi)))
>>>> +    {
>>>> +        struct multi_instance *mi = (struct multi_instance *) he->value;
>>>> +
>>>> +        if (mi->context.c2.tls_multi->peer_id != peerid)
>>>> +            continue;
>>>
>>> Shouldn't we use m->instances[peerid] instead of iterating over m->hash?
>>
>> Yes and a check that kernel does not give something > max_peer
> 
> I agree, doing the iterator here for every single active peer when we
> already have the array.  But then, of course, p2p peer-id will be
> "something random", not constrained by max-clients.

on p2mp code, it will be contrainst by max-clients. The random peerid 
only happens in p2p mode where do not have multi_instance anyway.

> 
> Anyway, we have a procedural problem here - kp@ is on vacation for a
> few weeks now, so we won't see a v2 of this patch any time soon.
> 
> Do you think the patch is ready to be merged, "as is", with not-so-good
> efficiency?  One of you could then send a followup patch changing this
> to use the array accessor.

Fine by me.

Arne

Hi,

On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
[cut]
> +
> +int
> +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> +{
> +
> +    struct ifdrv drv;
> +    uint8_t buf[4096];
> +    nvlist_t *nvl;
> +    const nvlist_t *const *nvpeers;
> +    size_t npeers;
> +    int ret;
> +
> +    if (!dco || !dco->open)
> +    {
> +        return 0;
> +    }
> +
> +    CLEAR(drv);
> +    snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
> +    drv.ifd_cmd = OVPN_GET_PEER_STATS;

What speaks against having a simple GET_PEER here which returns the full 
peer object with all possible attributes?

This way, if we want to retrieve another attribute in the future, this 
attribute will already be delivered by the same API, without the need to 
implement a new command each time.


Cheers,

Hi,

On Mon, Dec 12, 2022 at 09:53:36PM +0100, Antonio Quartulli wrote:
> On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
> [cut]
> > +int
> > +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> > +{
> > +
> > +    struct ifdrv drv;
> > +    uint8_t buf[4096];
> > +    nvlist_t *nvl;
> > +    const nvlist_t *const *nvpeers;
> > +    size_t npeers;
> > +    int ret;
> > +
> > +    if (!dco || !dco->open)
> > +    {
> > +        return 0;
> > +    }
> > +
> > +    CLEAR(drv);
> > +    snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
> > +    drv.ifd_cmd = OVPN_GET_PEER_STATS;
> 
> What speaks against having a simple GET_PEER here which returns the full 
> peer object with all possible attributes?
> 
> This way, if we want to retrieve another attribute in the future, this 
> attribute will already be delivered by the same API, without the need to 
> implement a new command each time.

One counter-argument would be "the statistics are polled more often than
'all information about a peer object', so it should be less costly"
(also, when do we ever poll 'all attributes'?  Since OpenVPN *sets*
these, we should know, except for the counters...)

gert

Hi,

On 13/12/2022 05:46, Gert Doering wrote:
> Hi,
> 
> On Mon, Dec 12, 2022 at 09:53:36PM +0100, Antonio Quartulli wrote:
>> On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
>> [cut]
>>> +int
>>> +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
>>> +{
>>> +
>>> +    struct ifdrv drv;
>>> +    uint8_t buf[4096];
>>> +    nvlist_t *nvl;
>>> +    const nvlist_t *const *nvpeers;
>>> +    size_t npeers;
>>> +    int ret;
>>> +
>>> +    if (!dco || !dco->open)
>>> +    {
>>> +        return 0;
>>> +    }
>>> +
>>> +    CLEAR(drv);
>>> +    snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
>>> +    drv.ifd_cmd = OVPN_GET_PEER_STATS;
>>
>> What speaks against having a simple GET_PEER here which returns the full
>> peer object with all possible attributes?
>>
>> This way, if we want to retrieve another attribute in the future, this
>> attribute will already be delivered by the same API, without the need to
>> implement a new command each time.
> 
> One counter-argument would be "the statistics are polled more often than
> 'all information about a peer object', so it should be less costly"

It's a netlink/ioctl API: it's already costly and out of any fast-path, 
therefore I don't think this argument really plays an important role here.

What is more important is designing an API that can last long, without 
the need for extra cluttering when we decide to implement something new.

Ideally a GET_PEER command is pretty standard and can also be used for 
any kind of state inspection (i.e. even for 'debugging', although it's 
not the primary usage)

> (also, when do we ever poll 'all attributes'?  Since OpenVPN *sets*
> these, we should know, except for the counters...)

Like I said above, being this an API imho it is important to implement 
it in a way that can serve a reasonably broad set of use cases. (OpenVPN 
is the only user now, but may not be the case later)



Cheers,

Hi,

On Tue, Dec 13, 2022 at 01:12:30PM +0100, Antonio Quartulli wrote:
> Ideally a GET_PEER command is pretty standard and can also be used for 
> any kind of state inspection (i.e. even for 'debugging', although it's 
> not the primary usage)
[..]
> Like I said above, being this an API imho it is important to implement 
> it in a way that can serve a reasonably broad set of use cases. (OpenVPN 
> is the only user now, but may not be the case later)

I think there is nothing that stops us from doing GET_PEER on Linux, 
if that's already there and a proper "people agree that this is the way"
API, and using the API we have for FreeBSD for the time being.

The Linux counter support patch might want rename the OS-independent
functions accordingly then :-)

gert

Hi,

On 05/12/2022 17:41, Kristof Provost via Openvpn-devel wrote:
> From: Kristof Provost <kp@FreeBSD.org>
> 
> When DCO is active userspace doesn't see all of the traffic, so when we
> access these stats we must update them.
> 
> Retrieve kernel statistics every time we access the
> link_(read|write)_bytes values.
> 
> Introduce a dco_(read|write)_bytes so that we don't clobber the existing
> statistics, which still count control packets, sent or received directly
> through the socket.
> 
> Signed-off-by: Kristof Provost <kprovost@netgate.com>

Acked-by: Antonio Quartulli <a@unbstable.cc>

> ---
>   src/openvpn/dco.h              |  8 ++++
>   src/openvpn/dco_freebsd.c      | 78 ++++++++++++++++++++++++++++++++++
>   src/openvpn/dco_linux.c        |  7 +++
>   src/openvpn/dco_win.c          |  7 +++
>   src/openvpn/multi.c            | 30 +++++++------
>   src/openvpn/openvpn.h          |  2 +
>   src/openvpn/ovpn_dco_freebsd.h |  1 +
>   7 files changed, 120 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h
> index e051db06..e5d89358 100644
> --- a/src/openvpn/dco.h
> +++ b/src/openvpn/dco.h
> @@ -225,6 +225,14 @@ void dco_install_iroute(struct multi_context *m, struct multi_instance *mi,
>    */
>   void dco_delete_iroutes(struct multi_context *m, struct multi_instance *mi);
>   
> +/**
> + * Update traffic statistics for all peers
> + *
> + * @param dco	DCO device context
> + * @param m	the server context
> + **/
> +int dco_get_peer_stats(dco_context_t *dco, struct multi_context *m);
> +
>   /**
>    * Retrieve the list of ciphers supported by the current platform
>    *
> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
> index a52ac8c1..5b352859 100644
> --- a/src/openvpn/dco_freebsd.c
> +++ b/src/openvpn/dco_freebsd.c
> @@ -37,6 +37,7 @@
>   #include "dco.h"
>   #include "tun.h"
>   #include "crypto.h"
> +#include "multi.h"
>   #include "ssl_common.h"
>   
>   static nvlist_t *
> @@ -641,6 +642,83 @@ dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
>       nvlist_destroy(nvl);
>   }
>   
> +static void
> +dco_update_peer_stat(struct multi_context *m, uint32_t peerid, const nvlist_t *nvl)
> +{
> +    struct hash_element *he;
> +    struct hash_iterator hi;
> +
> +    hash_iterator_init(m->hash, &hi);
> +
> +    while ((he = hash_iterator_next(&hi)))
> +    {
> +        struct multi_instance *mi = (struct multi_instance *) he->value;
> +
> +        if (mi->context.c2.tls_multi->peer_id != peerid)
> +            continue;
> +
> +        mi->context.c2.dco_read_bytes = nvlist_get_number(nvl, "in");
> +        mi->context.c2.dco_write_bytes = nvlist_get_number(nvl, "out");
> +
> +        return;
> +    }
> +
> +    msg(M_INFO, "Peer %d returned by kernel, but not found locally", peerid);
> +}
> +
> +int
> +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> +{
> +
> +    struct ifdrv drv;
> +    uint8_t buf[4096];
> +    nvlist_t *nvl;
> +    const nvlist_t *const *nvpeers;
> +    size_t npeers;
> +    int ret;
> +
> +    if (!dco || !dco->open)
> +    {
> +        return 0;
> +    }
> +
> +    CLEAR(drv);
> +    snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
> +    drv.ifd_cmd = OVPN_GET_PEER_STATS;
> +    drv.ifd_len = sizeof(buf);
> +    drv.ifd_data = buf;
> +
> +    ret = ioctl(dco->fd, SIOCGDRVSPEC, &drv);
> +    if (ret)
> +    {
> +        msg(M_WARN | M_ERRNO, "Failed to get peer stats");
> +        return -EINVAL;
> +    }
> +
> +    nvl = nvlist_unpack(buf, drv.ifd_len, 0);
> +    if (! nvl)
> +    {
> +        msg(M_WARN, "Failed to unpack nvlist");
> +        return -EINVAL;
> +    }
> +
> +    if (! nvlist_exists_nvlist_array(nvl, "peers")) {
> +        /* no peers */
> +        return 0;
> +    }
> +
> +    nvpeers = nvlist_get_nvlist_array(nvl, "peers", &npeers);
> +    for (size_t i = 0; i < npeers; i++)
> +    {
> +        const nvlist_t *peer = nvpeers[i];
> +        uint32_t peerid = nvlist_get_number(peer, "peerid");
> +
> +        dco_update_peer_stat(m, peerid, nvlist_get_nvlist(peer, "bytes"));
> +    }
> +
> +    return 0;
> +}
> +
>   const char *
>   dco_get_supported_ciphers()
>   {
> diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
> index 10935820..0306cec3 100644
> --- a/src/openvpn/dco_linux.c
> +++ b/src/openvpn/dco_linux.c
> @@ -911,6 +911,13 @@ nla_put_failure:
>       return ret;
>   }
>   
> +int
> +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> +{
> +    /* Not implemented. */
> +    return 0;
> +}
> +
>   bool
>   dco_available(int msglevel)
>   {
> diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
> index 48a1755a..68ec931c 100644
> --- a/src/openvpn/dco_win.c
> +++ b/src/openvpn/dco_win.c
> @@ -399,6 +399,13 @@ dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
>       return 0;
>   }
>   
> +int
> +dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
> +{
> +    /* Not implemented. */
> +    return 0;
> +}
> +
>   void
>   dco_event_set(dco_context_t *dco, struct event_set *es, void *arg)
>   {
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 0a23c2bc..38da87b8 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -538,29 +538,31 @@ multi_del_iroutes(struct multi_context *m,
>   }
>   
>   static void
> -setenv_stats(struct context *c)
> +setenv_stats(struct multi_context *m, struct context *c)
>   {
> -    setenv_counter(c->c2.es, "bytes_received", c->c2.link_read_bytes);
> -    setenv_counter(c->c2.es, "bytes_sent", c->c2.link_write_bytes);
> +    dco_get_peer_stats(&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);
>   }
>   
>   static void
> -multi_client_disconnect_setenv(struct multi_instance *mi)
> +multi_client_disconnect_setenv(struct multi_context *m, struct multi_instance *mi)
>   {
>       /* setenv client real IP address */
>       setenv_trusted(mi->context.c2.es, get_link_socket_info(&mi->context));
>   
>       /* setenv stats */
> -    setenv_stats(&mi->context);
> +    setenv_stats(m, &mi->context);
>   
>       /* setenv connection duration */
>       setenv_long_long(mi->context.c2.es, "time_duration", now - mi->created);
>   }
>   
>   static void
> -multi_client_disconnect_script(struct multi_instance *mi)
> +multi_client_disconnect_script(struct multi_context *m, struct multi_instance *mi)
>   {
> -    multi_client_disconnect_setenv(mi);
> +    multi_client_disconnect_setenv(m, mi);
>   
>       if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_DISCONNECT))
>       {
> @@ -667,7 +669,7 @@ multi_close_instance(struct multi_context *m,
>   
>       if (mi->context.c2.tls_multi->multi_state >= CAS_CONNECT_DONE)
>       {
> -        multi_client_disconnect_script(mi);
> +        multi_client_disconnect_script(m, mi);
>       }
>   
>       close_context(&mi->context, SIGTERM, CC_GC_FREE);
> @@ -837,6 +839,8 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
>   
>           status_reset(so);
>   
> +        dco_get_peer_stats(&m->top.c1.tuntap->dco, m);
> +
>           if (version == 1)
>           {
>               /*
> @@ -856,8 +860,8 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
>                       status_printf(so, "%s,%s," counter_format "," counter_format ",%s",
>                                     tls_common_name(mi->context.c2.tls_multi, false),
>                                     mroute_addr_print(&mi->real, &gc),
> -                                  mi->context.c2.link_read_bytes,
> -                                  mi->context.c2.link_write_bytes,
> +                                  mi->context.c2.link_read_bytes + mi->context.c2.dco_read_bytes,
> +                                  mi->context.c2.link_write_bytes + mi->context.c2.dco_write_bytes,
>                                     time_string(mi->created, 0, false, &gc));
>                   }
>                   gc_free(&gc);
> @@ -932,8 +936,8 @@ multi_print_status(struct multi_context *m, struct status_output *so, const int
>                                     sep, mroute_addr_print(&mi->real, &gc),
>                                     sep, print_in_addr_t(mi->reporting_addr, IA_EMPTY_IF_UNDEF, &gc),
>                                     sep, print_in6_addr(mi->reporting_addr_ipv6, IA_EMPTY_IF_UNDEF, &gc),
> -                                  sep, mi->context.c2.link_read_bytes,
> -                                  sep, mi->context.c2.link_write_bytes,
> +                                  sep, mi->context.c2.link_read_bytes + mi->context.c2.dco_read_bytes,
> +                                  sep, mi->context.c2.link_write_bytes + mi->context.c2.dco_write_bytes,
>                                     sep, time_string(mi->created, 0, false, &gc),
>                                     sep, (unsigned int)mi->created,
>                                     sep, tls_username(mi->context.c2.tls_multi, false),
> @@ -2752,7 +2756,7 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>            * did not fail */
>           if (mi->context.c2.tls_multi->multi_state == CAS_PENDING_DEFERRED_PARTIAL)
>           {
> -            multi_client_disconnect_script(mi);
> +            multi_client_disconnect_script(m, mi);
>           }
>   
>           mi->context.c2.tls_multi->multi_state = CAS_FAILED;
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index c543cbf6..5981e4d5 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -267,8 +267,10 @@ struct context_2
>       counter_type tun_read_bytes;
>       counter_type tun_write_bytes;
>       counter_type link_read_bytes;
> +    counter_type dco_read_bytes;
>       counter_type link_read_bytes_auth;
>       counter_type link_write_bytes;
> +    counter_type dco_write_bytes;
>   #ifdef PACKET_TRUNCATION_CHECK
>       counter_type n_trunc_tun_read;
>       counter_type n_trunc_tun_write;
> diff --git a/src/openvpn/ovpn_dco_freebsd.h b/src/openvpn/ovpn_dco_freebsd.h
> index cf92d597..cc90111e 100644
> --- a/src/openvpn/ovpn_dco_freebsd.h
> +++ b/src/openvpn/ovpn_dco_freebsd.h
> @@ -61,5 +61,6 @@ enum ovpn_key_cipher {
>   #define OVPN_POLL_PKT           _IO('D', 10)
>   #define OVPN_GET_PKT            _IO('D', 11)
>   #define OVPN_SET_IFMODE         _IO('D', 12)
> +#define OVPN_GET_PEER_STATS     _IO('D', 13)
>   
>   #endif /* ifndef _NET_IF_OVPN_H_ */

I've stared at the code for a bit (reasonable, with the enhancements
to come from Arne / Lev), and tested on Linux without DCO (client/server),
Linux with DCO (client/server) and FreeBSD with DCO (client/server),
and everything worked.

To make this new code work for the "configure --disable-dco" case,
I had to add an empty stub to dco.h

+static inline int
+dco_get_peer_stats(dco_context_t *dco, struct multi_context *m)
+{
+    return 0;
+}
+

.. which is not something I normally do, but this is the same 
"non function" as for the "no linux and no windows implementation yet",
and we want this to proceed now.  It's quite obviously no code that
does anything.


I have not tested the actual counter functionality, as the kernel
side bits are not merged yet (in FreeBSD review process, but they
all look quite straightforward).  It will log warnings about
not being able to get the stats...

Dec 13 22:40:03 fbsd14 tun-udp-p2mp-topology-subnet[19846]: Failed to get peer stats: Operation not supported (errno=45)

.. but this is "just cosmetic", if a bit annoying.  Will go away
as soon as the kernel bits are there :-)


Uncrustify demanded a few whitespace amendments, which I've done.

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

commit ce2b459dabc29d071be28b8ddaa0512f8c8143ec (master)
commit 7695a55be06ccdcfae070b0cb3f0dfddca605003 (release/2.6)
Author: Kristof Provost
Date:   Mon Dec 5 17:41:00 2022 +0100

     Read DCO traffic stats from the kernel

     Signed-off-by: Kristof Provost <kprovost@netgate.com>
     Acked-by: Antonio Quartulli <antonio@openvpn.net>
     Message-Id: <20221205164103.9190-2-kprovost@netgate.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25618.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering