Re: [PATCH net-next v5 9/9] tun: add rx timestamp to the vnet header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] tun: missing TUNSETOFFLOAD negotiation for rx timestamp in
  tun_put_user()
- [Medium] tun: missing rx timestamp in tun_vnet_hdr_put() for XDP and
  tap paths
- [Medium] tun: vnet_hdr_sz for rx timestamp enables tunnel GSO in
  tun_get_user()
--

commit d550e223b215243ce50180a91000fc2227098b26
Author: Steffen Trumtrar <s.trumtrar@pengutronix.de>

tun: add rx timestamp to the vnet header

This commit writes the current TAI timestamp into the virtio-net header in
tun_put_user() when the configured vnet_hdr_sz covers the tstamp field, and
updates __tun_vnet_hdr_put() to accept struct virtio_net_common_hdr.

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 8920c797acfd7..1a8c3377753d7 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
[ ... ]
> @@ -2168,12 +2169,16 @@ static ssize_t tun_put_user(struct tun_struct *tun,
>  	}
>  
>  	if (vnet_hdr_sz) {
> -		struct virtio_net_hdr_v1_hash_tunnel hdr;
> -		struct virtio_net_hdr *gso;
> +		struct virtio_net_common_hdr hdr;
>  
>  		memset(&hdr, 0, sizeof(hdr));
> +
> +		/* The configured header covers the tstamp field */
> +		if (vnet_hdr_sz >= offsetofend(struct virtio_net_common_hdr, tstamp))
> +			put_unaligned_le64(ktime_get_clocktai_ns(), hdr.tstamp);

[Severity: Medium]
Should rx timestamp generation be negotiated via a TUN_F_* flag in
TUNSETOFFLOAD rather than gated solely on vnet_hdr_sz?

TUNSETVNETHDRSZ in tun_vnet_ioctl() already accepts any header size >=
sizeof(struct virtio_net_hdr) and zero-fills trailing bytes, so a VMM cannot
use it to probe whether the host kernel supports VIRTIO_NET_F_RX_TSTAMP.
Because struct virtio_net_common_hdr uses a cumulative wire layout, any future
feature placed after tstamp that sets vnet_hdr_sz > 40 without negotiating
VIRTIO_NET_F_RX_TSTAMP will also cause tun_put_user() to write timestamps into
hdr.tstamp on every packet.

[Severity: Medium]
Does configuring vnet_hdr_sz for VIRTIO_NET_F_RX_TSTAMP also unintentionally
enable UDP tunnel GSO on the guest TX path?

drivers/net/tun_vnet.h:tun_vnet_hdr_guest_features() {
	if (vnet_hdr_sz >= TUN_VNET_TNL_SIZE)
		return NETIF_F_GSO_UDP_TUNNEL | NETIF_F_GSO_UDP_TUNNEL_CSUM;
	return 0;
}

Setting vnet_hdr_sz to 40 for rx timestamps satisfies vnet_hdr_sz >=
TUN_VNET_TNL_SIZE (24), causing tun_get_user() and tun_xdp_one() to parse the
24-byte tunnel header and accept UDP-tunneled GSO packets from the guest even
when VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO was not negotiated.

> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1fa44d..d4cc56dac98d6 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
[ ... ]
> @@ -174,7 +176,11 @@ static inline int __tun_vnet_hdr_put(int sz, netdev_features_t features,
>  static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter,
>  				   const struct virtio_net_hdr *hdr)
>  {
> -	return __tun_vnet_hdr_put(sz, 0, iter, hdr);
> +	struct virtio_net_common_hdr common_hdr = {};
> +
> +	common_hdr.hdr = *hdr;
> +
> +	return __tun_vnet_hdr_put(sz, 0, iter, &common_hdr);
>  }

[Severity: Medium]
Does tun_vnet_hdr_put() leave common_hdr.tstamp zeroed when vnet_hdr_sz covers
the tstamp field?

Both tun_put_user_xdp() and tap_put_user() write the virtio-net header via
tun_vnet_hdr_put(), so XDP frames read from a tun device and packets read from
tap/macvtap interfaces never receive an rx timestamp.  In addition,
tun_put_user() samples ktime_get_clocktai_ns() when dequeuing from
tfile->tx_ring rather than using skb_hwtstamps(skb)->hwtstamp or timestamping
on packet arrival in tun_net_xmit(), which includes ring queuing delay in the
reported rx timestamp.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-v6-7-topic-virtio-net-ptp-v5-0-7ab67b572f4f@pengutronix.de?part=9