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 <[email protected]> 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-3350851c4...@pengutronix.de?part=1
