[PATCH RFC v3 0/2] virtio-net: add flow filter for receive timestamps

[PATCH RFC v3 0/2] virtio-net: add flow filter for receive timestamps

[Steffen Trumtrar]

This series tries to pick up the work on the virtio-net timestamping
feature from Willem de Bruijn.

Original series
    Message-Id: 20210208185558.995292-1-willemdebruijn.kernel@gmail.com
    Subject: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp,
    tx-tstamp and tx-time
    From: Willem de Bruijn <willemb@google.com>

    RFC for four new features to the virtio network device:

    1. pass tx flow state to host, for routing + telemetry
    2. pass rx tstamp to guest, for better RTT estimation
    3. pass tx tstamp to guest, idem
    3. pass tx delivery time to host, for accurate pacing

    All would introduce an extension to the virtio spec.

The changes in this series are to the driver side. For the changes to qemu see:
    https://github.com/strumtrar/qemu/tree/v11.0.1/virtio-rx-stamps    

Currently only virtio-net is supported. Performance was tested with
pktgen which doesn't show a decrease in transfer speeds.

As these patches are now mostly different from the initial patchset, I removed
the Signed-off-bys from Willem, so he mustn't be ashamed of what his work evolved to ;)

---
Changes in v3:
- rework to use virtio_net_common_hdr instead of adding new struct
Changes in v2:
- rework patches to use flow filter instead of feature flag
- Link to v1: https://lore.kernel.org/r/20231218-v6-7-topic-virtio-net-ptp-v1-0-cac92b2d8532@pengutronix.de

To: Michael S. Tsirkin <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
To: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
To: David S. Miller <davem@davemloft.net>
To: Eric Dumazet <edumazet@google.com>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Richard Cochran <richardcochran@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: Eugenio Pérez <eperezma@redhat.com>
To: Jason Wang <jasowangio@gmail.com>
To: Alexei Starovoitov <ast@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
To: Jesper Dangaard Brouer <hawk@kernel.org>
To: John Fastabend <john.fastabend@gmail.com>
To: Stanislav Fomichev <sdf@fomichev.me>
Cc: virtualization@lists.linux.dev
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: bpf@vger.kernel.org
Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

---
Steffen Trumtrar (2):
      virtio-net: support receive timestamp
      tun: support rx-tstamp

 drivers/net/tun.c               |  20 ++++--
 drivers/net/tun_vnet.h          |  27 ++++----
 drivers/net/virtio_net.c        | 134 ++++++++++++++++++++++++++++++++++++----
 include/uapi/linux/virtio_net.h |  12 ++++
 4 files changed, 166 insertions(+), 27 deletions(-)
---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20231218-v6-7-topic-virtio-net-ptp-3df023bc4f4d

Best regards,
--  
Steffen Trumtrar <s.trumtrar@pengutronix.de>

[PATCH RFC v3 1/2] virtio-net: support receive timestamp

[Steffen Trumtrar]

Add optional hardware rx timestamp offload for virtio-net.

Introduce virtio feature VIRTIO_NET_F_TSTAMP. If negotiated, the
virtio-net header is expanded with room for a timestamp.

To get and set the hwtstamp the functions ndo_hwtstamp_set/get are
implemented. This allows filtering the packets and only time stamp
the packets where the filter matches. This way, the timestamping can
be en/disabled at runtime.
Currently, timestamping is handled the same for all supported filters and
therefore handled the same for all received packets.

XDP packets are not supported and timestamping is skipped for the XDP path.

Tested:
  guest: ./timestamping eth0 \
          SOF_TIMESTAMPING_RAW_HARDWARE \
          SOF_TIMESTAMPING_RX_HARDWARE
  host: nc -4 -u 192.168.1.1 319

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

--
  Changes to v2:
  - update filter handling
  - move tstamp into virtio_net_common_hdr
  - remove new struct virtio_net_hdr_v1_hash_tunnel_ts
  Changes to v1:
  - rework series to use flow filters
  - add new struct virtio_net_hdr_v1_hash_tunnel_ts
  - original work done by: Willem de Bruijn <willemb@google.com>
---
 drivers/net/virtio_net.c        | 127 +++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/virtio_net.h |   1 +
 2 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e34c52d059d39..ef6238cb336b9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -416,6 +416,12 @@ struct virtnet_info {
 	u32 rss_hash_types_supported;
 	u32 rss_hash_types_saved;
 
+	/* Device passes time stamps to the driver */
+	bool has_tstamp;
+	bool hwts_rx_en;
+
+	struct kernel_hwtstamp_config tstamp_config;
+
 	/* Has control virtqueue */
 	bool has_cvq;
 
@@ -499,6 +505,8 @@ struct virtio_net_common_hdr {
 		struct virtio_net_hdr_v1_hash hash_v1_hdr;
 		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
 	};
+
+	__le16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
 };
 
 static struct virtio_net_common_hdr xsk_hdr;
@@ -2468,6 +2476,15 @@ virtio_net_hash_value(const struct virtio_net_hdr_v1_hash *hdr_hash)
 		(__le16_to_cpu(hdr_hash->hash_value_hi) << 16);
 }
 
+static inline u64
+virtio_net_tstamp_value(const struct virtio_net_common_hdr *hdr)
+{
+	return (u64)__le16_to_cpu(hdr->tstamp[0]) |
+	      ((u64)__le16_to_cpu(hdr->tstamp[1]) << 16) |
+	      ((u64)__le16_to_cpu(hdr->tstamp[2]) << 32) |
+	      ((u64)__le16_to_cpu(hdr->tstamp[3]) << 48);
+}
+
 static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
 				struct sk_buff *skb)
 {
@@ -2497,6 +2514,18 @@ static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
 	skb_set_hash(skb, virtio_net_hash_value(hdr_hash), rss_hash_type);
 }
 
+static inline void virtnet_record_rx_tstamp(const struct virtnet_info *vi,
+					    struct sk_buff *skb)
+{
+	struct skb_shared_hwtstamps *shhwtstamps = skb_hwtstamps(skb);
+	const struct virtio_net_common_hdr *h = skb_vnet_common_hdr(skb);
+	u64 ts;
+
+	ts = virtio_net_tstamp_value(h);
+	memset(shhwtstamps, 0, sizeof(struct skb_shared_hwtstamps));
+	shhwtstamps->hwtstamp = ns_to_ktime(ts);
+}
+
 static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *rq,
 				 struct sk_buff *skb, u8 flags)
 {
@@ -2590,6 +2619,9 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 	if (unlikely(!skb))
 		return;
 
+	if (likely(skb) && likely(!vi->xdp_enabled) && READ_ONCE(vi->hwts_rx_en))
+		virtnet_record_rx_tstamp(vi, skb);
+
 	virtnet_receive_done(vi, rq, skb, flags);
 }
 
@@ -5518,6 +5550,30 @@ static int virtnet_get_per_queue_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static int virtnet_get_ts_info(struct net_device *dev,
+			       struct kernel_ethtool_ts_info *info)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	/* setup default software timestamp */
+	ethtool_op_get_ts_info(dev, info);
+
+	if (vi->has_tstamp) {
+		info->so_timestamping |=
+			SOF_TIMESTAMPING_RX_HARDWARE |
+			SOF_TIMESTAMPING_RAW_HARDWARE;
+
+		info->rx_filters = (BIT(HWTSTAMP_FILTER_NONE) |
+				    BIT(HWTSTAMP_FILTER_PTP_V1_L4_SYNC) |
+				    BIT(HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ) |
+				    BIT(HWTSTAMP_FILTER_PTP_V2_L4_SYNC) |
+				    BIT(HWTSTAMP_FILTER_PTP_V2_EVENT) |
+				    BIT(HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ));
+	}
+
+	return 0;
+}
+
 static void virtnet_init_settings(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -5613,7 +5669,7 @@ static const struct ethtool_ops virtnet_ethtool_ops = {
 	.get_ethtool_stats = virtnet_get_ethtool_stats,
 	.set_channels = virtnet_set_channels,
 	.get_channels = virtnet_get_channels,
-	.get_ts_info = ethtool_op_get_ts_info,
+	.get_ts_info = virtnet_get_ts_info,
 	.get_link_ksettings = virtnet_get_link_ksettings,
 	.set_link_ksettings = virtnet_set_link_ksettings,
 	.set_coalesce = virtnet_set_coalesce,
@@ -6212,6 +6268,61 @@ static void virtnet_tx_timeout(struct net_device *dev, unsigned int txqueue)
 		   jiffies_to_usecs(jiffies - READ_ONCE(txq->trans_start)));
 }
 
+static int virtnet_hwtstamp_get(struct net_device *dev,
+				struct kernel_hwtstamp_config *config)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	if (!vi->has_tstamp)
+		return -EOPNOTSUPP;
+
+	*config = vi->tstamp_config;
+
+	return 0;
+}
+
+static int virtnet_hwtstamp_set(struct net_device *dev,
+				struct kernel_hwtstamp_config *config,
+				struct netlink_ext_ack *extack)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	if (!vi->has_tstamp)
+		return -EOPNOTSUPP;
+
+	config->tx_type = HWTSTAMP_TX_OFF;
+
+	if (unlikely(vi->xdp_enabled))
+		return -EOPNOTSUPP;
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		WRITE_ONCE(vi->hwts_rx_en, false);
+		break;
+	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+		WRITE_ONCE(vi->hwts_rx_en, true);
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	default:
+		config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	vi->tstamp_config = *config;
+
+	return 0;
+}
+
 static int virtnet_init_irq_moder(struct virtnet_info *vi)
 {
 	u8 profile_flags = 0, coal_flags = 0;
@@ -6272,6 +6383,8 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_get_phys_port_name	= virtnet_get_phys_port_name,
 	.ndo_set_features	= virtnet_set_features,
 	.ndo_tx_timeout		= virtnet_tx_timeout,
+	.ndo_hwtstamp_set	= virtnet_hwtstamp_set,
+	.ndo_hwtstamp_get	= virtnet_hwtstamp_get,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
@@ -6878,6 +6991,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
 		vi->has_rss_hash_report = true;
 
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_TSTAMP))
+		vi->has_tstamp = true;
+
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
 		vi->has_rss = true;
 
@@ -6922,6 +7038,12 @@ static int virtnet_probe(struct virtio_device *vdev)
 	else
 		vi->hdr_len = sizeof(struct virtio_net_hdr);
 
+	if (vi->has_tstamp)
+		vi->hdr_len = offsetof(struct virtio_net_common_hdr, tstamp) +
+			      sizeof_field(struct virtio_net_common_hdr, tstamp);
+
+	vi->hwts_rx_en = false;
+
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM))
 		vi->rx_tnl_csum = true;
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO))
@@ -7248,7 +7370,8 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
 	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL, \
 	VIRTIO_NET_F_VQ_NOTF_COAL, \
-	VIRTIO_NET_F_GUEST_HDRLEN, VIRTIO_NET_F_DEVICE_STATS
+	VIRTIO_NET_F_GUEST_HDRLEN, VIRTIO_NET_F_DEVICE_STATS, \
+	VIRTIO_NET_F_TSTAMP
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 1db45b01532b5..39977765b72aa 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -56,6 +56,7 @@
 #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
+#define VIRTIO_NET_F_TSTAMP	  49	/* Device sends TAI receive time */
 #define VIRTIO_NET_F_DEVICE_STATS 50	/* Device can provide device-level statistics. */
 #define VIRTIO_NET_F_VQ_NOTF_COAL 52	/* Device supports virtqueue notification coalescing */
 #define VIRTIO_NET_F_NOTF_COAL	53	/* Device supports notifications coalescing */

-- 
2.52.0

[PATCH RFC v3 2/2] tun: support rx-tstamp

[Steffen Trumtrar]

Demonstrate support for new virtio-net features

VIRTIO_NET_HDR_F_TSTAMP

This is not intended to be merged.

A full feature test also requires a patched qemu binary that knows
these features and negotiates correct vnet_hdr_sz in
virtio_net_set_mrg_rx_bufs. See
https://github.com/strumtrar/qemu/tree/v11.0.1/virtio-rx-stamps

Not-yet-signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

---
 Changes since v2:
 - use tstamp from virtio_net_common_hdr
 - use the negotiated vnet_hdr_sz in __tun_vnet_hdr_get

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/net/tun.c               | 20 +++++++++++++++-----
 drivers/net/tun_vnet.h          | 27 ++++++++++++++++-----------
 drivers/net/virtio_net.c        | 11 -----------
 include/uapi/linux/virtio_net.h | 11 +++++++++++
 4 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 5a302709a68aa..924784771b6b7 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2157,12 +2157,23 @@ 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));
+
+		/* hdr has at least the size up to and including the tstamp field. */
+		if (vnet_hdr_sz >= offsetof(struct virtio_net_common_hdr, tstamp) +
+				   sizeof_field(struct virtio_net_common_hdr, tstamp)) {
+			u64 tstamp = ktime_get_clocktai_ns();
+
+			hdr.tstamp[0] = (tstamp & 0x000000000000ffffULL) >> 0;
+			hdr.tstamp[1] = (tstamp & 0x00000000ffff0000ULL) >> 16;
+			hdr.tstamp[2] = (tstamp & 0x0000ffff00000000ULL) >> 32;
+			hdr.tstamp[3] = (tstamp & 0xffff000000000000ULL) >> 48;
+		}
+
 		ret = tun_vnet_hdr_tnl_from_skb(tun->flags, tun->dev, skb,
-						&hdr);
+						(struct virtio_net_hdr_v1_hash_tunnel *)&hdr);
 		if (ret)
 			return ret;
 
@@ -2170,9 +2181,8 @@ static ssize_t tun_put_user(struct tun_struct *tun,
 		 * Drop the packet if the configured header size is too small
 		 * WRT the enabled offloads.
 		 */
-		gso = (struct virtio_net_hdr *)&hdr;
 		ret = __tun_vnet_hdr_put(vnet_hdr_sz, tun->dev->features,
-					 iter, gso);
+					 iter, &hdr);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
index f4c652b1fa44d..0a7fe1ce889b5 100644
--- a/drivers/net/tun_vnet.h
+++ b/drivers/net/tun_vnet.h
@@ -109,12 +109,9 @@ static inline long tun_vnet_ioctl(int *vnet_hdr_sz, unsigned int *flags,
 	}
 }
 
-static inline unsigned int tun_vnet_parse_size(netdev_features_t features)
+static inline unsigned int tun_vnet_parse_size(int vnet_hdr_sz)
 {
-	if (!(features & NETIF_F_GSO_UDP_TUNNEL))
-		return sizeof(struct virtio_net_hdr);
-
-	return TUN_VNET_TNL_SIZE;
+	return min_t(unsigned int, sizeof(struct virtio_net_common_hdr), vnet_hdr_sz);
 }
 
 static inline int __tun_vnet_hdr_get(int sz, unsigned int flags,
@@ -122,15 +119,20 @@ static inline int __tun_vnet_hdr_get(int sz, unsigned int flags,
 				     struct iov_iter *from,
 				     struct virtio_net_hdr *hdr)
 {
-	unsigned int parsed_size = tun_vnet_parse_size(features);
+	unsigned int parsed_size = tun_vnet_parse_size(sz);
+	u8 tmp[sizeof(struct virtio_net_common_hdr)]; // temp buffer with known size
 	u16 hdr_len;
 
 	if (iov_iter_count(from) < sz)
 		return -EINVAL;
 
-	if (!copy_from_iter_full(hdr, parsed_size, from))
+	/* copy parsed size data to the tmp buffer, otherwise compiler will complain */
+	if (!copy_from_iter_full(tmp, parsed_size, from))
 		return -EFAULT;
 
+	/* now copy the relevant data from tmp to hdr */
+	memcpy(hdr, tmp, min(parsed_size, sizeof(*hdr)));
+
 	hdr_len = tun_vnet16_to_cpu(flags, hdr->hdr_len);
 
 	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
@@ -155,14 +157,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags,
 
 static inline int __tun_vnet_hdr_put(int sz, netdev_features_t features,
 				     struct iov_iter *iter,
-				     const struct virtio_net_hdr *hdr)
+				     const struct virtio_net_common_hdr *hdr)
 {
-	unsigned int parsed_size = tun_vnet_parse_size(features);
+	unsigned int parsed_size = tun_vnet_parse_size(sz);
+	u8 buf[sizeof(*hdr)];
 
 	if (unlikely(iov_iter_count(iter) < sz))
 		return -EINVAL;
 
-	if (unlikely(copy_to_iter(hdr, parsed_size, iter) != parsed_size))
+	memcpy(buf, hdr, parsed_size);
+
+	if (unlikely(copy_to_iter(buf, parsed_size, iter) != parsed_size))
 		return -EFAULT;
 
 	if (iov_iter_zero(sz - parsed_size, iter) != sz - parsed_size)
@@ -174,7 +179,7 @@ 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);
+	return __tun_vnet_hdr_put(sz, 0, iter, (struct virtio_net_common_hdr *)hdr);
 }
 
 static inline int tun_vnet_hdr_to_skb(unsigned int flags, struct sk_buff *skb,
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ef6238cb336b9..60ae9392f9876 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -498,17 +498,6 @@ struct padded_vnet_hdr {
 	char padding[12];
 };
 
-struct virtio_net_common_hdr {
-	union {
-		struct virtio_net_hdr hdr;
-		struct virtio_net_hdr_mrg_rxbuf	mrg_hdr;
-		struct virtio_net_hdr_v1_hash hash_v1_hdr;
-		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
-	};
-
-	__le16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
-};
-
 static struct virtio_net_common_hdr xsk_hdr;
 
 static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 39977765b72aa..8fed76755dcb1 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -238,6 +238,17 @@ struct virtio_net_hdr_mrg_rxbuf {
 	struct virtio_net_hdr hdr;
 	__virtio16 num_buffers;	/* Number of merged rx buffers */
 };
+
+struct virtio_net_common_hdr {
+	union {
+		struct virtio_net_hdr hdr;
+		struct virtio_net_hdr_mrg_rxbuf	mrg_hdr;
+		struct virtio_net_hdr_v1_hash hash_v1_hdr;
+		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
+	};
+
+	__virtio16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
+};
 #endif /* ...VIRTIO_NET_NO_LEGACY */
 
 /*

-- 
2.52.0

Re: [PATCH RFC v3 1/2] virtio-net: support receive timestamp

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] virtio-net: VIRTIO_NET_F_TSTAMP hardcodes header length to 32 bytes, corrupting packet data
- [High] virtio-net: enabling XDP silently drops active hardware timestamps
- [Medium] virtio-net: virtnet_hwtstamp_set silently ignores TX requests and violates RX filter API
--

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

virtio-net: support receive timestamp

This commit adds optional hardware RX timestamp offload support for virtio-net
by introducing the VIRTIO_NET_F_TSTAMP feature. It expands the virtio-net
header and provides implementation for ndo_hwtstamp_set and get.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..ef6238cb336b9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -499,6 +505,8 @@ struct virtio_net_common_hdr {
>  		struct virtio_net_hdr_v1_hash hash_v1_hdr;
>  		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
>  	};
> +
> +	__le16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
>  };

[Severity: Critical]
Does appending tstamp after the union statically pin its offset to 24 bytes
(the size of virtio_net_hdr_v1_hash_tunnel)? If smaller headers are
negotiated, this layout does not appear to match the actual hardware format.

[ ... ]

> @@ -2590,6 +2619,9 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  	if (unlikely(!skb))
>  		return;
>  
> +	if (likely(skb) && likely(!vi->xdp_enabled) && READ_ONCE(vi->hwts_rx_en))
> +		virtnet_record_rx_tstamp(vi, skb);
> +
>  	virtnet_receive_done(vi, rq, skb, flags);
>  }

[Severity: High]
While virtnet_hwtstamp_set() prevents enabling hardware timestamps if XDP is
already loaded, does the reverse sequence leave the system in an inconsistent
state?

If a user enables hardware timestamping first, and then loads an XDP program,
virtnet_xdp_set() does not check vi->hwts_rx_en. Once vi->xdp_enabled becomes
true, receive_buf() will silently drop all hardware timestamps without
returning an error to userspace.

[ ... ]

> @@ -6212,6 +6268,61 @@ static void virtnet_tx_timeout(struct net_device *dev, unsigned int txqueue)
[ ... ]
> +static int virtnet_hwtstamp_set(struct net_device *dev,
> +				struct kernel_hwtstamp_config *config,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +
> +	if (!netif_running(dev))
> +		return -EINVAL;
> +
> +	if (!vi->has_tstamp)
> +		return -EOPNOTSUPP;
> +
> +	config->tx_type = HWTSTAMP_TX_OFF;

[Severity: Medium]
Since this overwrites the requested tx_type without returning an error code
like -ERANGE, will this silently drop TX timestamp requests and mislead
userspace applications into assuming their TX configuration was applied?

> +
> +	if (unlikely(vi->xdp_enabled))
> +		return -EOPNOTSUPP;
> +
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		WRITE_ONCE(vi->hwts_rx_en, false);
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +		WRITE_ONCE(vi->hwts_rx_en, true);
> +		break;
> +	case HWTSTAMP_FILTER_ALL:
> +	default:
> +		config->rx_filter = HWTSTAMP_FILTER_NONE;
> +		return -ERANGE;
> +	}

[Severity: Medium]
When specific PTP RX filters are requested, the driver enables vi->hwts_rx_en,
and receive_buf() then unconditionally reads timestamps for all packets.

Since the driver effectively implements HWTSTAMP_FILTER_ALL semantics,
shouldn't it accept HWTSTAMP_FILTER_ALL and upgrade specific PTP filters to
it, rather than rejecting it with -ERANGE?

[ ... ]

> @@ -6922,6 +7038,12 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	else
>  		vi->hdr_len = sizeof(struct virtio_net_hdr);
>  
> +	if (vi->has_tstamp)
> +		vi->hdr_len = offsetof(struct virtio_net_common_hdr, tstamp) +
> +			      sizeof_field(struct virtio_net_common_hdr, tstamp);
> +

[Severity: Critical]
Does setting the header length to 32 bytes unconditionally here break the
dynamic header sizing for other feature combinations?

If a device negotiates VIRTIO_NET_F_TSTAMP and VIRTIO_NET_F_MRG_RXBUF but
not HASH or TUNNEL, the hardware sends a 20-byte header, but the driver now
assumes 32 bytes. Will this cause the receive path to consume 12 bytes of
payload as part of the header, and the transmit path to push 12 bytes of
padding into the device, leading to data corruption?

> +	vi->hwts_rx_en = false;
> +
>  	if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM))
>  		vi->rx_tnl_csum = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-v6-7-topic-virtio-net-ptp-v3-0-3350851c4f88@pengutronix.de?part=1

Re: [PATCH RFC v3 2/2] tun: support rx-tstamp

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: info leak via out-of-bounds stack read in __tun_vnet_hdr_put()
- [High] net: truncation of extended virtio-net headers breaks offloads in __tun_vnet_hdr_get()
- [Medium] net: incorrect endianness when storing virtio-net timestamp in tun_put_user()
--

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

tun: support rx-tstamp

Demonstrate support for new virtio-net features
VIRTIO_NET_HDR_F_TSTAMP

[ ... ]

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 5a302709a68aa..924784771b6b7 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -2157,12 +2157,23 @@ 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));
> +
> +		/* hdr has at least the size up to and including the tstamp field. */
> +		if (vnet_hdr_sz >= offsetof(struct virtio_net_common_hdr, tstamp) +
> +				   sizeof_field(struct virtio_net_common_hdr, tstamp)) {
> +			u64 tstamp = ktime_get_clocktai_ns();
> +
> +			hdr.tstamp[0] = (tstamp & 0x000000000000ffffULL) >> 0;
> +			hdr.tstamp[1] = (tstamp & 0x00000000ffff0000ULL) >> 16;
> +			hdr.tstamp[2] = (tstamp & 0x0000ffff00000000ULL) >> 32;
> +			hdr.tstamp[3] = (tstamp & 0xffff000000000000ULL) >> 48;

[Severity: Medium]
Since tstamp is defined as a __virtio16 array, does this need to use
cpu_to_tun_vnet16() for byte swapping?

Assigning a host-endian value directly using native 16-bit shifts could corrupt
the timestamp when read by the guest if the negotiated virtio endianness differs
from the host native endianness.

> +		}
> +
>  		ret = tun_vnet_hdr_tnl_from_skb(tun->flags, tun->dev, skb,

[ ... ]

> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1fa44d..0a7fe1ce889b5 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -122,15 +119,20 @@ static inline int __tun_vnet_hdr_get(int sz, unsigned int flags,
>  				     struct iov_iter *from,
>  				     struct virtio_net_hdr *hdr)
>  {
> -	unsigned int parsed_size = tun_vnet_parse_size(features);
> +	unsigned int parsed_size = tun_vnet_parse_size(sz);
> +	u8 tmp[sizeof(struct virtio_net_common_hdr)]; // temp buffer with known size
>  	u16 hdr_len;
>  
>  	if (iov_iter_count(from) < sz)
>  		return -EINVAL;
>  
> -	if (!copy_from_iter_full(hdr, parsed_size, from))
> +	/* copy parsed size data to the tmp buffer, otherwise compiler will complain */
> +	if (!copy_from_iter_full(tmp, parsed_size, from))
>  		return -EFAULT;
>  
> +	/* now copy the relevant data from tmp to hdr */
> +	memcpy(hdr, tmp, min(parsed_size, sizeof(*hdr)));

[Severity: High]
Does this artificially limit the copy to exactly 10 bytes?

Because hdr is passed as a pointer to struct virtio_net_hdr, sizeof(*hdr)
evaluates statically to 10 bytes.

Callers like tun_get_user() might allocate a larger structure, but they will
only receive 10 bytes, causing extended virtio-net header fields (such as hash
values or tunnel offsets) to be silently dropped.

> +
>  	hdr_len = tun_vnet16_to_cpu(flags, hdr->hdr_len);
>  
>  	if (hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> @@ -155,14 +157,17 @@ static inline int tun_vnet_hdr_get(int sz, unsigned int flags,
>  
>  static inline int __tun_vnet_hdr_put(int sz, netdev_features_t features,
>  				     struct iov_iter *iter,
> -				     const struct virtio_net_hdr *hdr)
> +				     const struct virtio_net_common_hdr *hdr)
>  {
> -	unsigned int parsed_size = tun_vnet_parse_size(features);
> +	unsigned int parsed_size = tun_vnet_parse_size(sz);
> +	u8 buf[sizeof(*hdr)];
>  
>  	if (unlikely(iov_iter_count(iter) < sz))
>  		return -EINVAL;
>  
> -	if (unlikely(copy_to_iter(hdr, parsed_size, iter) != parsed_size))
> +	memcpy(buf, hdr, parsed_size);

[Severity: High]
Could this copy leak uninitialized kernel stack memory to userspace?

Callers like tap_put_user() and tun_put_user_xdp() allocate a 10-byte
struct virtio_net_hdr on the kernel stack and pass its pointer to
tun_vnet_hdr_put().

Because tun_vnet_hdr_put() casts the 10-byte pointer to the 32-byte
struct virtio_net_common_hdr:

    return __tun_vnet_hdr_put(sz, 0, iter, (struct virtio_net_common_hdr *)hdr);

and parsed_size can be up to 32 bytes (depending on vnet_hdr_sz), this
memcpy could read 22 bytes past the end of the 10-byte struct.

> +
> +	if (unlikely(copy_to_iter(buf, parsed_size, iter) != parsed_size))
>  		return -EFAULT;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-v6-7-topic-virtio-net-ptp-v3-0-3350851c4f88@pengutronix.de?part=2

Re: [PATCH RFC v3 0/2] virtio-net: add flow filter for receive timestamps

[Willem de Bruijn]

Steffen Trumtrar wrote:
> This series tries to pick up the work on the virtio-net timestamping
> feature from Willem de Bruijn.
> 
> Original series
>     Message-Id: 20210208185558.995292-1-willemdebruijn.kernel@gmail.com
>     Subject: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp,
>     tx-tstamp and tx-time
>     From: Willem de Bruijn <willemb@google.com>
> 
>     RFC for four new features to the virtio network device:
> 
>     1. pass tx flow state to host, for routing + telemetry
>     2. pass rx tstamp to guest, for better RTT estimation
>     3. pass tx tstamp to guest, idem
>     3. pass tx delivery time to host, for accurate pacing
> 
>     All would introduce an extension to the virtio spec.
> 
> The changes in this series are to the driver side. For the changes to qemu see:
>     https://github.com/strumtrar/qemu/tree/v11.0.1/virtio-rx-stamps    

What is the plan wrt the virtio spec? +1 on demonstrating working code
first for feasibility.

Does this now also need a spec proposal to virtio-comment@lists.linux.dev?

Re: [PATCH RFC v3 1/2] virtio-net: support receive timestamp

[Willem de Bruijn]

Steffen Trumtrar wrote:
> Add optional hardware rx timestamp offload for virtio-net.
> 
> Introduce virtio feature VIRTIO_NET_F_TSTAMP. If negotiated, the
> virtio-net header is expanded with room for a timestamp.
> 
> To get and set the hwtstamp the functions ndo_hwtstamp_set/get are
> implemented. This allows filtering the packets and only time stamp
> the packets where the filter matches. This way, the timestamping can
> be en/disabled at runtime.
> Currently, timestamping is handled the same for all supported filters and
> therefore handled the same for all received packets.
> 
> XDP packets are not supported and timestamping is skipped for the XDP path.
> 
> Tested:
>   guest: ./timestamping eth0 \
>           SOF_TIMESTAMPING_RAW_HARDWARE \
>           SOF_TIMESTAMPING_RX_HARDWARE
>   host: nc -4 -u 192.168.1.1 319
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> 
> --
>   Changes to v2:
>   - update filter handling
>   - move tstamp into virtio_net_common_hdr
>   - remove new struct virtio_net_hdr_v1_hash_tunnel_ts
>   Changes to v1:
>   - rework series to use flow filters
>   - add new struct virtio_net_hdr_v1_hash_tunnel_ts
>   - original work done by: Willem de Bruijn <willemb@google.com>
> ---
>  drivers/net/virtio_net.c        | 127 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_net.h |   1 +
>  2 files changed, 126 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..ef6238cb336b9 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -416,6 +416,12 @@ struct virtnet_info {
>  	u32 rss_hash_types_supported;
>  	u32 rss_hash_types_saved;
>  
> +	/* Device passes time stamps to the driver */
> +	bool has_tstamp;
> +	bool hwts_rx_en;
> +
> +	struct kernel_hwtstamp_config tstamp_config;
> +
>  	/* Has control virtqueue */
>  	bool has_cvq;
>  
> @@ -499,6 +505,8 @@ struct virtio_net_common_hdr {
>  		struct virtio_net_hdr_v1_hash hash_v1_hdr;
>  		struct virtio_net_hdr_v1_hash_tunnel tnl_hdr;
>  	};
> +
> +	__le16 tstamp[4];	/* 64-bit timestamp, 2-byte aligned */
>  };

What is the alignment of the structs in the union and thus the padding
here?

Moving away from introducing a new variant of the struct with every
field is the right approach.

I don't think virtio_net_common_hdr was intended to be extended in
this way. But it might be the simplest approach.

Re: [PATCH RFC v3 0/2] virtio-net: add flow filter for receive timestamps

[Steffen Trumtrar]

On 2026-09-21 at 21:31 -04, Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote:

> Steffen Trumtrar wrote:
> > This series tries to pick up the work on the virtio-net timestamping
> > feature from Willem de Bruijn.
> > 
> > Original series
> >     Message-Id: 20210208185558.995292-1-willemdebruijn.kernel@gmail.com
> >     Subject: [PATCH RFC v2 0/4] virtio-net: add tx-hash, rx-tstamp,
> >     tx-tstamp and tx-time
> >     From: Willem de Bruijn <willemb@google.com>
> > 
> >     RFC for four new features to the virtio network device:
> > 
> >     1. pass tx flow state to host, for routing + telemetry
> >     2. pass rx tstamp to guest, for better RTT estimation
> >     3. pass tx tstamp to guest, idem
> >     3. pass tx delivery time to host, for accurate pacing
> > 
> >     All would introduce an extension to the virtio spec.
> > 
> > The changes in this series are to the driver side. For the changes to qemu see:
> >     https://github.com/strumtrar/qemu/tree/v11.0.1/virtio-rx-stamps    
> 
> What is the plan wrt the virtio spec? +1 on demonstrating working code
> first for feasibility.
> 
> Does this now also need a spec proposal to virtio-comment@lists.linux.dev?

If this shows to be the right direction, I'd create the spec proposal. I find writing the code and see if it works much easier ;)


Steffen

-- 
Pengutronix e.K.                | Dipl.-Inform. Steffen Trumtrar |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |