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

[sashiko-bot@kernel.org]

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