| Message ID | 20240506011637.27272-13-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
2024-05-06, 03:16:25 +0200, Antonio Quartulli wrote: > Byte/packet counters for in-tunnel and transport streams > are now initialized and updated as needed. > > To be exported via netlink. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > drivers/net/ovpn/Makefile | 1 + > drivers/net/ovpn/io.c | 10 ++++++++ > drivers/net/ovpn/peer.c | 3 +++ > drivers/net/ovpn/peer.h | 13 +++++++--- > drivers/net/ovpn/stats.c | 21 ++++++++++++++++ > drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++ What I'm seeing in this patch are "success" counters. I don't see any stats for dropped packets that would help the user figure out why their VPN isn't working, or why their CPU is burning up decrypting packets that don't show up on the host, etc. You can guess there are issues by subtracting the link and vpn stats, but that's very limited. For example: - counter for packets dropped during the udp encap/decap - counter for failed encrypt/decrypt (especially failed decrypt) - counter for replay protection failures - counter for malformed packets Maybe not a separate counter for each of the prints you added in the rx/tx code, but at least enough of them to start figuring out what's going on without enabling all the prints and parsing dmesg. > diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h > index da41d711745c..b5ff59a4b40f 100644 > --- a/drivers/net/ovpn/peer.h > +++ b/drivers/net/ovpn/peer.h > @@ -10,14 +10,15 @@ > #ifndef _NET_OVPN_OVPNPEER_H_ > #define _NET_OVPN_OVPNPEER_H_ > > +#include <linux/ptr_ring.h> > +#include <net/dst_cache.h> > +#include <uapi/linux/ovpn.h> > + > #include "bind.h" > #include "pktid.h" > #include "crypto.h" > #include "socket.h" > - > -#include <linux/ptr_ring.h> > -#include <net/dst_cache.h> > -#include <uapi/linux/ovpn.h> > +#include "stats.h" Header reshuffling got squashed into the wrong patch? > diff --git a/drivers/net/ovpn/stats.h b/drivers/net/ovpn/stats.h > new file mode 100644 > index 000000000000..5134e49c0458 > --- /dev/null > +++ b/drivers/net/ovpn/stats.h > @@ -0,0 +1,52 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* OpenVPN data channel offload > + * > + * Copyright (C) 2020-2024 OpenVPN, Inc. > + * > + * Author: James Yonan <james@openvpn.net> > + * Antonio Quartulli <antonio@openvpn.net> > + * Lev Stipakov <lev@openvpn.net> > + */ > + > +#ifndef _NET_OVPN_OVPNSTATS_H_ > +#define _NET_OVPN_OVPNSTATS_H_ > + > +//#include <linux/atomic.h> > +//#include <linux/jiffies.h> Forgot a clean up before posting? :)
On 12/05/2024 10:47, Sabrina Dubroca wrote: > 2024-05-06, 03:16:25 +0200, Antonio Quartulli wrote: >> Byte/packet counters for in-tunnel and transport streams >> are now initialized and updated as needed. >> >> To be exported via netlink. >> >> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >> --- >> drivers/net/ovpn/Makefile | 1 + >> drivers/net/ovpn/io.c | 10 ++++++++ >> drivers/net/ovpn/peer.c | 3 +++ >> drivers/net/ovpn/peer.h | 13 +++++++--- >> drivers/net/ovpn/stats.c | 21 ++++++++++++++++ >> drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++ > > What I'm seeing in this patch are "success" counters. I don't see any > stats for dropped packets that would help the user figure out why > their VPN isn't working, or why their CPU is burning up decrypting > packets that don't show up on the host, etc. You can guess there are > issues by subtracting the link and vpn stats, but that's very limited. This stats are just the bare minimum to make our current userspace happy :-) But we can always extend the stats reporting later on, no? > > For example: > - counter for packets dropped during the udp encap/decap > - counter for failed encrypt/decrypt (especially failed decrypt) > - counter for replay protection failures > - counter for malformed packets > > Maybe not a separate counter for each of the prints you added in the > rx/tx code, but at least enough of them to start figuring out what's > going on without enabling all the prints and parsing dmesg. Definitely a good suggestion! I'd just postpone it for later, unless you think it's a blocker. > > >> diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h >> index da41d711745c..b5ff59a4b40f 100644 >> --- a/drivers/net/ovpn/peer.h >> +++ b/drivers/net/ovpn/peer.h >> @@ -10,14 +10,15 @@ >> #ifndef _NET_OVPN_OVPNPEER_H_ >> #define _NET_OVPN_OVPNPEER_H_ >> >> +#include <linux/ptr_ring.h> >> +#include <net/dst_cache.h> >> +#include <uapi/linux/ovpn.h> >> + >> #include "bind.h" >> #include "pktid.h" >> #include "crypto.h" >> #include "socket.h" >> - >> -#include <linux/ptr_ring.h> >> -#include <net/dst_cache.h> >> -#include <uapi/linux/ovpn.h> >> +#include "stats.h" > > Header reshuffling got squashed into the wrong patch? indeed, darn. Juggling this many patches has been quite tedious > > >> diff --git a/drivers/net/ovpn/stats.h b/drivers/net/ovpn/stats.h >> new file mode 100644 >> index 000000000000..5134e49c0458 >> --- /dev/null >> +++ b/drivers/net/ovpn/stats.h >> @@ -0,0 +1,52 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> +/* OpenVPN data channel offload >> + * >> + * Copyright (C) 2020-2024 OpenVPN, Inc. >> + * >> + * Author: James Yonan <james@openvpn.net> >> + * Antonio Quartulli <antonio@openvpn.net> >> + * Lev Stipakov <lev@openvpn.net> >> + */ >> + >> +#ifndef _NET_OVPN_OVPNSTATS_H_ >> +#define _NET_OVPN_OVPNSTATS_H_ >> + >> +//#include <linux/atomic.h> >> +//#include <linux/jiffies.h> > > Forgot a clean up before posting? :) Yeah..I guess I'll write a small script to catch all these things..it's easy to lose them across the whole patchset. Thanks for spotting them! I will make sure they all go away >
2024-05-13, 09:25:29 +0200, Antonio Quartulli wrote: > On 12/05/2024 10:47, Sabrina Dubroca wrote: > > 2024-05-06, 03:16:25 +0200, Antonio Quartulli wrote: > > > Byte/packet counters for in-tunnel and transport streams > > > are now initialized and updated as needed. > > > > > > To be exported via netlink. > > > > > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > > > --- > > > drivers/net/ovpn/Makefile | 1 + > > > drivers/net/ovpn/io.c | 10 ++++++++ > > > drivers/net/ovpn/peer.c | 3 +++ > > > drivers/net/ovpn/peer.h | 13 +++++++--- > > > drivers/net/ovpn/stats.c | 21 ++++++++++++++++ > > > drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++ > > > > What I'm seeing in this patch are "success" counters. I don't see any > > stats for dropped packets that would help the user figure out why > > their VPN isn't working, or why their CPU is burning up decrypting > > packets that don't show up on the host, etc. You can guess there are > > issues by subtracting the link and vpn stats, but that's very limited. > > This stats are just the bare minimum to make our current userspace happy :-) > > But we can always extend the stats reporting later on, no? > > > > > For example: > > - counter for packets dropped during the udp encap/decap > > - counter for failed encrypt/decrypt (especially failed decrypt) > > - counter for replay protection failures > > - counter for malformed packets > > > > Maybe not a separate counter for each of the prints you added in the > > rx/tx code, but at least enough of them to start figuring out what's > > going on without enabling all the prints and parsing dmesg. > > Definitely a good suggestion! I'd just postpone it for later, unless you > think it's a blocker. I'm not sure. It's not strictly necessary to make the driver work, but from a user/admin's point of view, I think counters would be really useful. Maybe at least increment the rx_dropped/rx_errors/etc counters from rtnl_link_stats on the netdevice? > indeed, darn. Juggling this many patches has been quite tedious > > > > Yeah..I guess I'll write a small script to catch all these things..it's easy > to lose them across the whole patchset. > > Thanks for spotting them! I will make sure they all go away Thanks. I know it's painful :(
On 13/05/2024 11:19, Sabrina Dubroca wrote: > 2024-05-13, 09:25:29 +0200, Antonio Quartulli wrote: >> On 12/05/2024 10:47, Sabrina Dubroca wrote: >>> 2024-05-06, 03:16:25 +0200, Antonio Quartulli wrote: >>>> Byte/packet counters for in-tunnel and transport streams >>>> are now initialized and updated as needed. >>>> >>>> To be exported via netlink. >>>> >>>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >>>> --- >>>> drivers/net/ovpn/Makefile | 1 + >>>> drivers/net/ovpn/io.c | 10 ++++++++ >>>> drivers/net/ovpn/peer.c | 3 +++ >>>> drivers/net/ovpn/peer.h | 13 +++++++--- >>>> drivers/net/ovpn/stats.c | 21 ++++++++++++++++ >>>> drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++ >>> >>> What I'm seeing in this patch are "success" counters. I don't see any >>> stats for dropped packets that would help the user figure out why >>> their VPN isn't working, or why their CPU is burning up decrypting >>> packets that don't show up on the host, etc. You can guess there are >>> issues by subtracting the link and vpn stats, but that's very limited. >> >> This stats are just the bare minimum to make our current userspace happy :-) >> >> But we can always extend the stats reporting later on, no? >> >>> >>> For example: >>> - counter for packets dropped during the udp encap/decap >>> - counter for failed encrypt/decrypt (especially failed decrypt) >>> - counter for replay protection failures >>> - counter for malformed packets >>> >>> Maybe not a separate counter for each of the prints you added in the >>> rx/tx code, but at least enough of them to start figuring out what's >>> going on without enabling all the prints and parsing dmesg. >> >> Definitely a good suggestion! I'd just postpone it for later, unless you >> think it's a blocker. > > I'm not sure. It's not strictly necessary to make the driver work, but > from a user/admin's point of view, I think counters would be really > useful. > > Maybe at least increment the rx_dropped/rx_errors/etc counters from > rtnl_link_stats on the netdevice? Ok, will start with this and see how much work is to add the err counters right away. Thanks for the hint!
diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile index ccdaeced1982..d43fda72646b 100644 --- a/drivers/net/ovpn/Makefile +++ b/drivers/net/ovpn/Makefile @@ -17,4 +17,5 @@ ovpn-y += netlink-gen.o ovpn-y += peer.o ovpn-y += pktid.o ovpn-y += socket.o +ovpn-y += stats.o ovpn-y += udp.o diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c index 66a4c551c191..699e7f1274db 100644 --- a/drivers/net/ovpn/io.c +++ b/drivers/net/ovpn/io.c @@ -10,6 +10,7 @@ #include <linux/netdevice.h> #include <linux/skbuff.h> #include <net/gso.h> +#include <net/ip.h> #include "ovpnstruct.h" #include "io.h" @@ -19,6 +20,7 @@ #include "crypto_aead.h" #include "netlink.h" #include "proto.h" +#include "socket.h" #include "udp.h" int ovpn_struct_init(struct net_device *dev) @@ -163,6 +165,8 @@ static int ovpn_decrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) int ret = -1; u8 key_id; + ovpn_peer_stats_increment_rx(&peer->link_stats, skb->len); + /* get the key slot matching the key Id in the received packet */ key_id = ovpn_key_id_from_skb(skb); ks = ovpn_crypto_key_id_to_slot(&peer->crypto, key_id); @@ -184,6 +188,9 @@ static int ovpn_decrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) goto drop; } + /* increment RX stats */ + ovpn_peer_stats_increment_rx(&peer->vpn_stats, skb->len); + /* check if this is a valid datapacket that has to be delivered to the * tun interface */ @@ -278,6 +285,8 @@ static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) goto err; } + ovpn_peer_stats_increment_tx(&peer->vpn_stats, skb->len); + /* encrypt */ ret = ovpn_aead_encrypt(ks, skb, peer->id); if (unlikely(ret < 0)) { @@ -289,6 +298,7 @@ static bool ovpn_encrypt_one(struct ovpn_peer *peer, struct sk_buff *skb) success = true; + ovpn_peer_stats_increment_tx(&peer->link_stats, skb->len); err: ovpn_crypto_key_slot_put(ks); return success; diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c index 1b941deeede0..99a2ae42a332 100644 --- a/drivers/net/ovpn/peer.c +++ b/drivers/net/ovpn/peer.c @@ -20,6 +20,7 @@ #include "main.h" #include "netlink.h" #include "peer.h" +#include "socket.h" struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) { @@ -42,6 +43,8 @@ struct ovpn_peer *ovpn_peer_new(struct ovpn_struct *ovpn, u32 id) ovpn_crypto_state_init(&peer->crypto); spin_lock_init(&peer->lock); kref_init(&peer->refcount); + ovpn_peer_stats_init(&peer->vpn_stats); + ovpn_peer_stats_init(&peer->link_stats); INIT_WORK(&peer->encrypt_work, ovpn_encrypt_work); INIT_WORK(&peer->decrypt_work, ovpn_decrypt_work); diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h index da41d711745c..b5ff59a4b40f 100644 --- a/drivers/net/ovpn/peer.h +++ b/drivers/net/ovpn/peer.h @@ -10,14 +10,15 @@ #ifndef _NET_OVPN_OVPNPEER_H_ #define _NET_OVPN_OVPNPEER_H_ +#include <linux/ptr_ring.h> +#include <net/dst_cache.h> +#include <uapi/linux/ovpn.h> + #include "bind.h" #include "pktid.h" #include "crypto.h" #include "socket.h" - -#include <linux/ptr_ring.h> -#include <net/dst_cache.h> -#include <uapi/linux/ovpn.h> +#include "stats.h" /** * struct ovpn_peer - the main remote peer object @@ -36,6 +37,8 @@ * @dst_cache: cache for dst_entry used to send to peer * @bind: remote peer binding * @halt: true if ovpn_peer_mark_delete was called + * @vpn_stats: per-peer in-VPN TX/RX stays + * @link_stats: per-peer link/transport TX/RX stats * @delete_reason: why peer was deleted (i.e. timeout, transport error, ..) * @lock: protects binding to peer (bind) * @refcount: reference counter @@ -60,6 +63,8 @@ struct ovpn_peer { struct dst_cache dst_cache; struct ovpn_bind __rcu *bind; bool halt; + struct ovpn_peer_stats vpn_stats; + struct ovpn_peer_stats link_stats; enum ovpn_del_peer_reason delete_reason; spinlock_t lock; /* protects bind */ struct kref refcount; diff --git a/drivers/net/ovpn/stats.c b/drivers/net/ovpn/stats.c new file mode 100644 index 000000000000..78cd030fa26e --- /dev/null +++ b/drivers/net/ovpn/stats.c @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0 +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + */ + +#include <linux/atomic.h> + +#include "stats.h" + +void ovpn_peer_stats_init(struct ovpn_peer_stats *ps) +{ + atomic64_set(&ps->rx.bytes, 0); + atomic_set(&ps->rx.packets, 0); + + atomic64_set(&ps->tx.bytes, 0); + atomic_set(&ps->tx.packets, 0); +} diff --git a/drivers/net/ovpn/stats.h b/drivers/net/ovpn/stats.h new file mode 100644 index 000000000000..5134e49c0458 --- /dev/null +++ b/drivers/net/ovpn/stats.h @@ -0,0 +1,52 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* OpenVPN data channel offload + * + * Copyright (C) 2020-2024 OpenVPN, Inc. + * + * Author: James Yonan <james@openvpn.net> + * Antonio Quartulli <antonio@openvpn.net> + * Lev Stipakov <lev@openvpn.net> + */ + +#ifndef _NET_OVPN_OVPNSTATS_H_ +#define _NET_OVPN_OVPNSTATS_H_ + +//#include <linux/atomic.h> +//#include <linux/jiffies.h> + +/* per-peer stats, measured on transport layer */ + +/* one stat */ +struct ovpn_peer_stat { + atomic64_t bytes; + atomic_t packets; +}; + +/* rx and tx stats, enabled by notify_per != 0 or period != 0 */ +struct ovpn_peer_stats { + struct ovpn_peer_stat rx; + struct ovpn_peer_stat tx; +}; + +void ovpn_peer_stats_init(struct ovpn_peer_stats *ps); + +static inline void ovpn_peer_stats_increment(struct ovpn_peer_stat *stat, + const unsigned int n) +{ + atomic64_add(n, &stat->bytes); + atomic_inc(&stat->packets); +} + +static inline void ovpn_peer_stats_increment_rx(struct ovpn_peer_stats *stats, + const unsigned int n) +{ + ovpn_peer_stats_increment(&stats->rx, n); +} + +static inline void ovpn_peer_stats_increment_tx(struct ovpn_peer_stats *stats, + const unsigned int n) +{ + ovpn_peer_stats_increment(&stats->tx, n); +} + +#endif /* _NET_OVPN_OVPNSTATS_H_ */
Byte/packet counters for in-tunnel and transport streams are now initialized and updated as needed. To be exported via netlink. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/Makefile | 1 + drivers/net/ovpn/io.c | 10 ++++++++ drivers/net/ovpn/peer.c | 3 +++ drivers/net/ovpn/peer.h | 13 +++++++--- drivers/net/ovpn/stats.c | 21 ++++++++++++++++ drivers/net/ovpn/stats.h | 52 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 drivers/net/ovpn/stats.c create mode 100644 drivers/net/ovpn/stats.h