Re: [PATCH net 5/7] ovpn: fix UDP TX byte accounting

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, the ovpn netdev's rx_bytes and tx_bytes
  measure different layers.

--

ovpn: fix UDP TX byte accounting

This commit resets the inner network header in ovpn_udp_send_skb() before
the skb is handed to the UDP tunnel helpers. The goal is for the ovpn
netdev TX byte counter to include the OpenVPN data-channel header and the
authentication tag.

[Severity: Medium]
With this change, do rx_bytes and tx_bytes on the ovpn netdev now measure
different layers?

UDP and TCP share the same RX path, and it counts the decrypted plaintext
length:

drivers/net/ovpn/io.c:ovpn_netdev_write() {
    ...
	pkt_len = skb->len;
    ...
	if (likely(ret == NET_RX_SUCCESS)) {
		/* update RX stats with the size of decrypted packet */
		ovpn_peer_stats_increment_rx(&peer->vpn_stats, pkt_len);
		dev_dstats_rx_add(peer->ovpn->dev, pkt_len);
	}
    ...
}

UDP TX goes through iptunnel_xmit() / ip6tunnel_xmit(), which compute:

	int pkt_len = skb->len - skb_inner_network_offset(skb);

The inner network header now sits at the start of the encrypted packet.
So TX counts the opcode/peer-id (4 bytes), the packet-id (4 bytes), the
auth tag (16 bytes) and the ciphertext. That is about 24 bytes per packet
that TX counts and RX does not.

In effect, netdev TX now follows the per-peer link_stats semantics, while
netdev RX follows vpn_stats.

TCP TX uses yet another convention. ovpn_tcp_send_sock() calls
dev_dstats_tx_add(peer->ovpn->dev, skb->len) after ovpn_tcp_send_skb() has
pushed the 2-byte length prefix:

	*(__be16 *)__skb_push(skb, sizeof(u16)) = htons(len);

Other users of the same UDP tunnel stats helper do the opposite. WireGuard
sets the inner network header before it pushes its own header, so that
header is not counted:

drivers/net/wireguard/send.c:encrypt_packet() {
    ...
	skb_set_inner_network_header(skb, 0);
	header = (struct message_data *)skb_push(skb, sizeof(*header));
    ...
}

Take the case the commit message calls an undercount, where the inner
header still points at the plaintext. Before this patch, UDP TX counted the
ciphertext length there. Since the tag sits in front, that equals the
plaintext length, so it matched RX.

Is TX meant to include protocol overhead when RX does not? If so, could
the commit message explain why? If not, should RX and TCP TX be changed
to use the same convention?

The mismatch is still there at the end of the series. "ovpn: fix TCP TX
accounting for locally dropped packets" leaves both the TCP TX
dev_dstats_tx_add(skb->len) call and the RX accounting unchanged.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022121606.732029-2-antonio%40openvpn.net