| Message ID | 20221214164824.172-1-lstipakov@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Introduce dco_get_peer_stats API and Windows implementation | expand |
Hi, On Wed, Dec 14, 2022 at 11:49 AM Lev Stipakov <lstipakov@gmail.com> wrote: > From: Lev Stipakov <lev@openvpn.net> > > dco_get_peer_stats fetches stats for a single peer. This is mostly > useful in client mode. So far only Windows implements that. > Good to see this happening.. Do you have a link to a build including this patch for a quick test? Does this data from the driver include both control and data channel bytes? Right now what the GUI receives as bytecount is not zero, I suppose the daemon is reporting the control channel traffic. By the way: + msg(M_WARN | M_ERRNO, "DeviceIoControl(OVPN_IOCTL_SET_PEER) failed"); That would be OVPN_IOCTL_GET_STATS Selva
Hi, > Good to see this happening.. Turns out there is a bug in the driver at the moment - it doesn't update OUT bytes. This particular API hasn't been used in a while. > Does this data from the driver include both control and data channel bytes? Yes, at the moment those are "link" bytes and they include both control and data channels. > Right now what the GUI receives as bytecount is not zero, I suppose the daemon is reporting the control channel traffic. Yes. I will fix it in the driver so that it reports only data channel bytes, since control channel bytes are already counted in userspace. > That would be OVPN_IOCTL_GET_STATS Let me send v2 with fixed error message text and produce an MSI with the driver fixes.
Hi, On Wed, Dec 14, 2022 at 2:06 PM Lev Stipakov <lstipakov@gmail.com> wrote: > > > Right now what the GUI receives as bytecount is not zero, I suppose the > daemon is reporting the control channel traffic. > > Yes. I will fix it in the driver so that it reports only data channel > bytes, since control channel bytes are already counted in userspace. This data will also show up as stats on the adapter (device node) and should include all traffic that passes through it, no? The userspace reporting can handle either approach as long as all dco platforms count stats the same way. Selva
> This data will also show up as stats on the adapter (device node) and should include all traffic that passes through it, no? System adapter stats show only tun traffic - the one driver indicates to NetAdapter. For BYTECOUNT we (userspace client) currently show link traffic - encapsulated control and data packets. > The userspace reporting can handle either approach as long as all dco platforms count stats the same way. On Windows control packets are handled by userspace via link read/write routines (which use device handle from CreateFile). Both FreeBSD and Linux implementations use additional, netlink-based (or FreeBSD analogue) channel to send/receive control packets, so existing link read/write routines are not called (except initial handshake, before socket handover to kernel) and therefore stats for control packets are not duplicated.
Hi, On Wed, Dec 14, 2022 at 10:50:19PM +0200, Lev Stipakov wrote: > On Windows control packets are handled by userspace via link > read/write routines (which use device handle from CreateFile). Both > FreeBSD and Linux implementations use additional, netlink-based (or > FreeBSD analogue) channel to send/receive control packets, > so existing link read/write routines are not called (except initial > handshake, before socket handover to kernel) and therefore stats for > control packets are not duplicated. FreeBSD uses standard socket read/write calls for control packets. The "tunnel control packets through the ioctl() interface" method was never used by the kernel and will be removed from OpenVPN in kp's 4/4 patch (to be merged soon). But since the FreeBSD kernel does not count control packets (I've been told that it "sniffs" the socket and only removes "interesting" packets, the rest is passed through to userland - so control packets are not handled by kernel), there is no duplication. No idea about the Linux details... Antonio? gert
Hi, Selva has asked about a build which includes this patch. Here is MSI installer which incorporates required client patches - management: add timer to output BYTECOUNT - Introduce dco_get_peer_stats API and Windows implementation and a new driver version (0.8.3) with stats fixes. https://github.com/lstipakov/openvpn-build/actions/runs/3699097077 -Lev
Hi, On Wed, Dec 14, 2022 at 1:55 PM Selva Nair <selva.nair@gmail.com> wrote: > Hi, > > On Wed, Dec 14, 2022 at 11:49 AM Lev Stipakov <lstipakov@gmail.com> wrote: > >> From: Lev Stipakov <lev@openvpn.net> >> >> dco_get_peer_stats fetches stats for a single peer. This is mostly >> useful in client mode. So far only Windows implements that. >> > > Good to see this happening.. Do you have a link to a build including this > patch for a quick test? > At last I built this and gave it a quick test including the patch for management i/f. Also used the latest dco-win driver (0.8.3) with the stats fixes. I see two issues: (1) Bytes out for data are still not getting reported -- I see a small number even after transferring large amount of data indicating only control channel bytes shown (2) On reconnect, the stats revert back to what looks like the accumulated control channel stats. As if the stats in the driver is getting reset. This happens even on SIGUSR1 restart which can happen automatically on auth-failure token expiry, for example. Selva P.S. I couldn't compare this with the non-dco behaviour as it seems reconnecting with the wintun adapter is no longer working -- I get all wintun adapters are currently in use error on reconnect while a disconnect/connect fixes it. This is unrelated -- will test further and report separately.
Hi On Wed, Dec 14, 2022 at 6:09 PM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > Selva has asked about a build which includes this patch. > > Here is MSI installer which incorporates required client patches > > - management: add timer to output BYTECOUNT > - Introduce dco_get_peer_stats API and Windows implementation > > and a new driver version (0.8.3) with stats fixes. > > https://github.com/lstipakov/openvpn-build/actions/runs/3699097077 Thanks. Tested again using this. Ignore my previous comment about Out bytes not reported. Something must be wrong with my build --- this one shows in and out bytes correctly. But the problem with data getting reset on reconnect is real. Selva
More on the data channel traffic stats getting reset on reconnect: >> Here is MSI installer which incorporates required client patches >> >> - management: add timer to output BYTECOUNT >> - Introduce dco_get_peer_stats API and Windows implementation >> >> and a new driver version (0.8.3) with stats fixes. >> >> https://github.com/lstipakov/openvpn-build/actions/runs/3699097077 > > > Thanks. > > Tested again using this. Ignore my previous comment about Out bytes not > reported. Something must be wrong with my build --- this one shows in and > out bytes correctly. But the problem with data getting reset on > reconnect is real. > In non-dco use, the stats as persisted by the management interface are not reset throughout the lifetime of the process. With dco, what the driver provides is "Peer Stats" which is reset in OvpnPeerNew() (linux appears to do the same). A quick option would be to move the zero-ing to OvpnEvtFileCleanup() which, I guess, would work at least when persist-tun is in use. Or in a new callback that gets called on tun-open. But then it wont be strictly "Peer Stats". Otherwise we need a way to add the current data channel stats to the accumulated control-channel stats just before SIGUSR1 and SIGHUP. Or learn to live with the new paradigm. Selva >
Hi, > In non-dco use, the stats as persisted by the management interface are not reset throughout the lifetime of the process. With dco, what the driver provides is "Peer Stats" which is reset in OvpnPeerNew() (linux appears to do the same). A quick option > would be to move the zero-ing to OvpnEvtFileCleanup() which, I guess, would work at least when persist-tun is in use. Yeah, without persist-tun client closes the handle, which triggers OvpnEvtFileCleanup(), which would reset stats. > Or in a new callback that gets called on tun-open. But then it wont be strictly "Peer Stats". Yeah that might be an option, to have RESET_STATS ioctl. Also I wonder if the driver could remember, say, the last pid, and then reset stats on NEW_PEER if pid != last pid. Since pids are recycled, we might "leak" stats in unfortunate cases, but that's okay I guess?
Hi, On 15/12/2022 10:31, Lev Stipakov wrote: > Hi, > >> In non-dco use, the stats as persisted by the management interface are not reset throughout the lifetime of the process. With dco, what the driver provides is "Peer Stats" which is reset in OvpnPeerNew() (linux appears to do the same). A quick option > would be to move the zero-ing to OvpnEvtFileCleanup() which, I guess, would work at least when persist-tun is in use. > > Yeah, without persist-tun client closes the handle, which triggers > OvpnEvtFileCleanup(), which would reset stats. > >> Or in a new callback that gets called on tun-open. But then it wont be strictly "Peer Stats". > > Yeah that might be an option, to have RESET_STATS ioctl. > > Also I wonder if the driver could remember, say, the last pid, and > then reset stats on NEW_PEER if pid != last pid. Since pids are > recycled, we might "leak" stats in unfortunate cases, but that's okay > I guess? > Sorry for jumping in the discussion a bit late. IMHO the stats belong to the Peer object. Let's not forget that, apart from windows, DCO supports multi-peer mode, therefore we have to consider that case as well. Due to the above, I'd "reset" the stats when a new peer object is instantiated. Basically when a new peer object is instantiated it should come with empty stats, as expected. If the desired behvaiour in userspace is to keep the stats persistent across a number of events, it should simply be userspace to keep those stats. This way we don't mix up the logic of counting the bytes per peer, and keeping a general picture of the VPN process. Cheers,
Hi, On Thu, Dec 15, 2022 at 5:15 AM Antonio Quartulli <a@unstable.cc> wrote: > Hi, > > On 15/12/2022 10:31, Lev Stipakov wrote: > > Hi, > > > >> In non-dco use, the stats as persisted by the management interface are > not reset throughout the lifetime of the process. With dco, what the driver > provides is "Peer Stats" which is reset in OvpnPeerNew() (linux appears to > do the same). A quick option > would be to move the zero-ing to > OvpnEvtFileCleanup() which, I guess, would work at least when persist-tun > is in use. > > > > Yeah, without persist-tun client closes the handle, which triggers > > OvpnEvtFileCleanup(), which would reset stats. > Currently its worse than that -- stats is reset in OvpnNewPeer() which would happen (I guess) even if the tun handle is not closed. My suggestion was to move it to OvpnEvtFileCleanup() which will help those who use persist-tun. But all that is moot based on Antonio's explanation below -- this is Peer Stats, not adapter stats, so it has to reset as it is done now... I was treating it like the stats in a network interface which would be reset only on rare events lke adapter reset or driver update or reboot. > > > >> Or in a new callback that gets called on tun-open. But then it wont be > strictly "Peer Stats". > > > > Yeah that might be an option, to have RESET_STATS ioctl. > > > > Also I wonder if the driver could remember, say, the last pid, and > > then reset stats on NEW_PEER if pid != last pid. Since pids are > > recycled, we might "leak" stats in unfortunate cases, but that's okay > > I guess? > > > > Sorry for jumping in the discussion a bit late. > > IMHO the stats belong to the Peer object. > Let's not forget that, apart from windows, DCO supports multi-peer mode, > therefore we have to consider that case as well. > > Due to the above, I'd "reset" the stats when a new peer object is > instantiated. Basically when a new peer object is instantiated it should > come with empty stats, as expected. > > If the desired behvaiour in userspace is to keep the stats persistent > across a number of events, it should simply be userspace to keep those > stats. > > This way we don't mix up the logic of counting the bytes per peer, and > keeping a general picture of the VPN process. > Good point. I was late to realize that Peer Stats has to remain as it is to make sense in multi-peer scenarios. In that case we'll have to find a way to persist the stats across NEW_PEER controls within the client process. Possibly we can merge the current patch for openvpn.exe and then improve it later in bug-fix releases of 2.6? Selva
Hi, On Thu, Dec 15, 2022 at 07:49:53AM -0500, Selva Nair wrote: > Possibly we can merge the current patch > for openvpn.exe and then improve it later in bug-fix releases of 2.6? This was my thinking as well - so the patch is in, and provides at least some statistics, and room for improvements :-) gert
Hi, On 15/12/2022 13:55, Gert Doering wrote: > Hi, > > On Thu, Dec 15, 2022 at 07:49:53AM -0500, Selva Nair wrote: >> Possibly we can merge the current patch >> for openvpn.exe and then improve it later in bug-fix releases of 2.6? > > This was my thinking as well - so the patch is in, and provides at > least some statistics, and room for improvements :-) Yeah, this was the best course of action in my opinion as well. Cheers,
diff --git a/src/openvpn/dco.h b/src/openvpn/dco.h index 9a34d5b6..866aa0fb 100644 --- a/src/openvpn/dco.h +++ b/src/openvpn/dco.h @@ -235,6 +235,13 @@ void dco_delete_iroutes(struct multi_context *m, struct multi_instance *mi); **/ int dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m); +/** + * Update traffic statistics for single peer + * + * @param c instance context of the peer + **/ +int dco_get_peer_stats(struct context *c); + /** * Retrieve the list of ciphers supported by the current platform * @@ -362,6 +369,12 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m) return 0; } +static inline int +dco_get_peer_stats(struct context *c) +{ + return 0; +} + static inline const char * dco_get_supported_ciphers() { diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c index 8d342159..7c46f64e 100644 --- a/src/openvpn/dco_freebsd.c +++ b/src/openvpn/dco_freebsd.c @@ -722,6 +722,13 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m) return 0; } +int +dco_get_peer_stats(struct context *c) +{ + /* Not implemented. */ + return 0; +} + const char * dco_get_supported_ciphers() { diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c index 200d0b19..222537fc 100644 --- a/src/openvpn/dco_linux.c +++ b/src/openvpn/dco_linux.c @@ -924,6 +924,13 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m) return 0; } +int +dco_get_peer_stats(struct context *c) +{ + /* Not implemented. */ + return 0; +} + bool dco_available(int msglevel) { diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index 675dece2..b85665eb 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -33,6 +33,7 @@ #include "tun.h" #include "crypto.h" #include "ssl_common.h" +#include "openvpn.h" #include <bcrypt.h> #include <winsock2.h> @@ -406,6 +407,33 @@ dco_get_peer_stats_multi(dco_context_t *dco, struct multi_context *m) return 0; } +int +dco_get_peer_stats(struct context *c) +{ + struct tuntap *tt = c->c1.tuntap; + + if (!tuntap_defined(tt)) + { + return -1; + } + + OVPN_STATS stats; + ZeroMemory(&stats, sizeof(OVPN_STATS)); + + DWORD bytes_returned = 0; + if (!DeviceIoControl(tt->hand, OVPN_IOCTL_GET_STATS, NULL, 0, + &stats, sizeof(stats), &bytes_returned, NULL)) + { + msg(M_WARN | M_ERRNO, "DeviceIoControl(OVPN_IOCTL_SET_PEER) failed"); + return -1; + } + + c->c2.dco_read_bytes = stats.TransportBytesReceived; + c->c2.dco_write_bytes = stats.TransportBytesSent; + + return 0; +} + void dco_event_set(dco_context_t *dco, struct event_set *es, void *arg) { diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index 435efaf8..2c0bb6a8 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -43,6 +43,7 @@ #include "common.h" #include "manage.h" #include "openvpn.h" +#include "dco.h" #include "memdbg.h" @@ -4051,11 +4052,15 @@ management_check_bytecount(struct context *c, struct management *man, struct tim if (event_timeout_trigger(&man->connection.bytecount_update_interval, timeval, ETT_DEFAULT)) { - /* TODO: get stats from DCO */ - counter_type dco_read_bytes = 0; counter_type dco_write_bytes = 0; + if (dco_enabled(&c->options) && (dco_get_peer_stats(c) == 0)) + { + dco_read_bytes = c->c2.dco_read_bytes; + dco_write_bytes = c->c2.dco_write_bytes; + } + if (!(man->persist.callback.flags & MCF_SERVER)) { man_bytecount_output_client(man, dco_read_bytes, dco_write_bytes);