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

[Antonio Quartulli <antonio@openvpn.net>]

I'll have a deeper look.
Some concerns, especially about the stats, seem legit.

pw-bot: cr

On 23/10/2026 14:17, netdev-bot+sashiko@kernel.org wrote:
> 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.
> 
>> diff --git a/drivers/net/ovpn/udp.c b/drivers/net/ovpn/udp.c
>> index 7672094a67668..05e10a2ed1791 100644
>> --- a/drivers/net/ovpn/udp.c
>> +++ b/drivers/net/ovpn/udp.c
>> @@ -495,6 +495,9 @@ int ovpn_udp_send_skb(struct ovpn_peer *peer, struct sock *sk,
>>   		return -EADDRNOTAVAIL;
>>   	}
>>   
>> +	/* the encrypted openvpn packet is the UDP tunnel payload */
>> +	skb_reset_inner_network_header(skb);
>> +
> 
> [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.
> 

-- 
Antonio Quartulli
OpenVPN Inc.