diff mbox series

[net-next,v11,11/23] ovpn: store tunnel and transport statistics

Message ID 20241029-b4-ovpn-v11-11-de4698c73a25@openvpn.net
State New
Headers show
Series Introducing OpenVPN Data Channel Offload | expand

Commit Message

Antonio Quartulli Oct. 29, 2024, 10:47 a.m. UTC 
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/crypto_aead.c |  2 ++
 drivers/net/ovpn/io.c          | 11 ++++++++++
 drivers/net/ovpn/peer.c        |  2 ++
 drivers/net/ovpn/peer.h        |  5 +++++
 drivers/net/ovpn/skb.h         |  1 +
 drivers/net/ovpn/stats.c       | 21 +++++++++++++++++++
 drivers/net/ovpn/stats.h       | 47 ++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 90 insertions(+)

Comments

Sabrina Dubroca Oct. 31, 2024, 11:37 a.m. UTC | #1 
2024-10-29, 11:47:24 +0100, Antonio Quartulli wrote:
> @@ -136,6 +139,10 @@ void ovpn_decrypt_post(void *data, int ret)
>  		goto drop;
>  	}
>  
> +	/* increment RX stats */
> +	ovpn_peer_stats_increment_rx(&peer->vpn_stats, skb->len);
> +	ovpn_peer_stats_increment_rx(&peer->link_stats, orig_len);

[I don't know much about the userspace implementation, so maybe this
is a silly question]

What's the value of keeping track of 2 separate stats if they are
incremented exactly at the same time? Packet count will be the same,
and the difference in bytes will be just measuring the encap overhead.

Should one of them be "packets/individual messages that get received
over the UDP/TCP link" and the other "packets that get passed up to
the stack"?


> @@ -197,6 +206,8 @@ void ovpn_encrypt_post(void *data, int ret)
>  		goto err;
>  
>  	skb_mark_not_on_list(skb);
> +	ovpn_peer_stats_increment_tx(&peer->link_stats, skb->len);
> +	ovpn_peer_stats_increment_tx(&peer->vpn_stats, orig_len);
>  
>  	switch (peer->sock->sock->sk->sk_protocol) {
>  	case IPPROTO_UDP:

And on TX maybe something like "packets that the stack wants to send
through the tunnel" and "packets that actually make it onto the
UDP/TCP socket after encap/encrypt"?
Antonio Quartulli Oct. 31, 2024, 1:12 p.m. UTC | #2 
On 31/10/2024 12:37, Sabrina Dubroca wrote:
> 2024-10-29, 11:47:24 +0100, Antonio Quartulli wrote:
>> @@ -136,6 +139,10 @@ void ovpn_decrypt_post(void *data, int ret)
>>   		goto drop;
>>   	}
>>   
>> +	/* increment RX stats */
>> +	ovpn_peer_stats_increment_rx(&peer->vpn_stats, skb->len);
>> +	ovpn_peer_stats_increment_rx(&peer->link_stats, orig_len);
> 
> [I don't know much about the userspace implementation, so maybe this
> is a silly question]
> 
> What's the value of keeping track of 2 separate stats if they are
> incremented exactly at the same time? Packet count will be the same,
> and the difference in bytes will be just measuring the encap overhead.
> 
> Should one of them be "packets/individual messages that get received
> over the UDP/TCP link" and the other "packets that get passed up to
> the stack"?

You're correct: link_stats if "received over the TCP/UDP socket", while 
vpn_stats if what is passing through the ovpn virtual device.

Packet count may not match though, for example when something happens 
between "received packet on the link" and "packet passed up to the 
device" (i.e. decryption error).

This makes me wonder why we increment them at the very same place....
link_stats should be increased upon RX from the socket, while vpn_stats 
just before delivery. I'll double check.

> 
> 
>> @@ -197,6 +206,8 @@ void ovpn_encrypt_post(void *data, int ret)
>>   		goto err;
>>   
>>   	skb_mark_not_on_list(skb);
>> +	ovpn_peer_stats_increment_tx(&peer->link_stats, skb->len);
>> +	ovpn_peer_stats_increment_tx(&peer->vpn_stats, orig_len);
>>   
>>   	switch (peer->sock->sock->sk->sk_protocol) {
>>   	case IPPROTO_UDP:
> 
> And on TX maybe something like "packets that the stack wants to send
> through the tunnel" and "packets that actually make it onto the
> UDP/TCP socket after encap/encrypt"?

Correct.

Same issue here. Increments should not happen back to back.


Thanks a lot for spotting these.

Regards,


>
diff mbox series

Patch

diff --git a/drivers/net/ovpn/Makefile b/drivers/net/ovpn/Makefile
index ccdaeced1982c851475657860a005ff2b9dfbd13..d43fda72646bdc7644d9a878b56da0a0e5680c98 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/crypto_aead.c b/drivers/net/ovpn/crypto_aead.c
index f9e3feb297b19868b1084048933796fcc7a47d6e..072bb0881764752520e8e26e18337c1274ce1aa4 100644
--- a/drivers/net/ovpn/crypto_aead.c
+++ b/drivers/net/ovpn/crypto_aead.c
@@ -48,6 +48,7 @@  int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
 	int nfrags, ret;
 	u32 pktid, op;
 
+	ovpn_skb_cb(skb)->orig_len = skb->len;
 	ovpn_skb_cb(skb)->peer = peer;
 	ovpn_skb_cb(skb)->ks = ks;
 
@@ -159,6 +160,7 @@  int ovpn_aead_decrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
 	payload_offset = OVPN_OP_SIZE_V2 + NONCE_WIRE_SIZE + tag_size;
 	payload_len = skb->len - payload_offset;
 
+	ovpn_skb_cb(skb)->orig_len = skb->len;
 	ovpn_skb_cb(skb)->payload_offset = payload_offset;
 	ovpn_skb_cb(skb)->peer = peer;
 	ovpn_skb_cb(skb)->ks = ks;
diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 4c81c4547d35d2a73f680ef1f5d8853ffbd952e0..d56e74660c7be9020b5bdf7971322d41afd436d6 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
@@ -12,6 +12,7 @@ 
 #include <linux/skbuff.h>
 #include <net/gro_cells.h>
 #include <net/gso.h>
+#include <net/ip.h>
 
 #include "ovpnstruct.h"
 #include "peer.h"
@@ -68,6 +69,7 @@  void ovpn_decrypt_post(void *data, int ret)
 	unsigned int payload_offset = 0;
 	struct sk_buff *skb = data;
 	struct ovpn_peer *peer;
+	unsigned int orig_len;
 	__be16 proto;
 	__be32 *pid;
 
@@ -80,6 +82,7 @@  void ovpn_decrypt_post(void *data, int ret)
 	payload_offset = ovpn_skb_cb(skb)->payload_offset;
 	ks = ovpn_skb_cb(skb)->ks;
 	peer = ovpn_skb_cb(skb)->peer;
+	orig_len = ovpn_skb_cb(skb)->orig_len;
 
 	/* crypto is done, cleanup skb CB and its members */
 
@@ -136,6 +139,10 @@  void ovpn_decrypt_post(void *data, int ret)
 		goto drop;
 	}
 
+	/* increment RX stats */
+	ovpn_peer_stats_increment_rx(&peer->vpn_stats, skb->len);
+	ovpn_peer_stats_increment_rx(&peer->link_stats, orig_len);
+
 	ovpn_netdev_write(peer, skb);
 	/* skb is passed to upper layer - don't free it */
 	skb = NULL;
@@ -175,6 +182,7 @@  void ovpn_encrypt_post(void *data, int ret)
 	struct ovpn_crypto_key_slot *ks;
 	struct sk_buff *skb = data;
 	struct ovpn_peer *peer;
+	unsigned int orig_len;
 
 	/* encryption is happening asynchronously. This function will be
 	 * called later by the crypto callback with a proper return value
@@ -184,6 +192,7 @@  void ovpn_encrypt_post(void *data, int ret)
 
 	ks = ovpn_skb_cb(skb)->ks;
 	peer = ovpn_skb_cb(skb)->peer;
+	orig_len = ovpn_skb_cb(skb)->orig_len;
 
 	/* crypto is done, cleanup skb CB and its members */
 
@@ -197,6 +206,8 @@  void ovpn_encrypt_post(void *data, int ret)
 		goto err;
 
 	skb_mark_not_on_list(skb);
+	ovpn_peer_stats_increment_tx(&peer->link_stats, skb->len);
+	ovpn_peer_stats_increment_tx(&peer->vpn_stats, orig_len);
 
 	switch (peer->sock->sock->sk->sk_protocol) {
 	case IPPROTO_UDP:
diff --git a/drivers/net/ovpn/peer.c b/drivers/net/ovpn/peer.c
index 98ae7662f1e76811e625dc5f4b4c5c884856fbd6..5025bfb759d6a5f31e3f2ec094fe561fbdb9f451 100644
--- a/drivers/net/ovpn/peer.c
+++ b/drivers/net/ovpn/peer.c
@@ -48,6 +48,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);
 
 	ret = dst_cache_init(&peer->dst_cache, GFP_KERNEL);
 	if (ret < 0) {
diff --git a/drivers/net/ovpn/peer.h b/drivers/net/ovpn/peer.h
index 754fea470d1b4787f64a931d6c6adc24182fc16f..eb1e31e854fbfff25d07fba8026789e41a76c113 100644
--- a/drivers/net/ovpn/peer.h
+++ b/drivers/net/ovpn/peer.h
@@ -13,6 +13,7 @@ 
 #include <net/dst_cache.h>
 
 #include "crypto.h"
+#include "stats.h"
 
 /**
  * struct ovpn_peer - the main remote peer object
@@ -26,6 +27,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
@@ -44,6 +47,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/skb.h b/drivers/net/ovpn/skb.h
index 2a75cef403845e2262f033a78b3fa1369b8c3b5e..96afa01466ab1a3456d1f3ca0ffd397302460d53 100644
--- a/drivers/net/ovpn/skb.h
+++ b/drivers/net/ovpn/skb.h
@@ -22,6 +22,7 @@  struct ovpn_cb {
 	struct ovpn_crypto_key_slot *ks;
 	struct aead_request *req;
 	struct scatterlist *sg;
+	unsigned int orig_len;
 	unsigned int payload_offset;
 };
 
diff --git a/drivers/net/ovpn/stats.c b/drivers/net/ovpn/stats.c
new file mode 100644
index 0000000000000000000000000000000000000000..a383842c3449b73694c318837b0b92eb9afaec22
--- /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);
+	atomic64_set(&ps->rx.packets, 0);
+
+	atomic64_set(&ps->tx.bytes, 0);
+	atomic64_set(&ps->tx.packets, 0);
+}
diff --git a/drivers/net/ovpn/stats.h b/drivers/net/ovpn/stats.h
new file mode 100644
index 0000000000000000000000000000000000000000..868f49d25eaa8fef04a02a61c363d95f9c9ef80a
--- /dev/null
+++ b/drivers/net/ovpn/stats.h
@@ -0,0 +1,47 @@ 
+/* 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_
+
+/* one stat */
+struct ovpn_peer_stat {
+	atomic64_t bytes;
+	atomic64_t packets;
+};
+
+/* rx and tx stats combined */
+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);
+	atomic64_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_ */