[Openvpn-devel] Introduce dco_get_peer_stats API and Windows implementation

  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.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/dco.h         | 13 +++++++++++++
 src/openvpn/dco_freebsd.c |  7 +++++++
 src/openvpn/dco_linux.c   |  7 +++++++
 src/openvpn/dco_win.c     | 28 ++++++++++++++++++++++++++++
 src/openvpn/manage.c      |  9 +++++++--
 5 files changed, 62 insertions(+), 2 deletions(-)
  

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,