Re: [PATCH 1/2] vhost_user: Offer VIRTIO_NET_F_GUEST_CSUM

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5226 bytes --]

On Fri, Apr 03, 2026 at 10:02:03AM +0200, Laurent Vivier wrote:
> According to the virtio-net specification, when the VIRTIO_NET_F_GUEST_CSUM
> is negotiated, the device can set VIRTIO_NET_HDR_F_DATA_VALID in the
> virtio-net header to indicate that packet checksums have been validated,
> allowing the guest to skip verification. Without this feature, the device
> must provide fully checksummed packets.
> 
> The vhost-user TCP and UDP paths were unconditionally skipping checksum
> computation, regardless of whether GUEST_CSUM was negotiated. This
> went undetected with Linux guests because Linux's virtio-net driver
> honours VIRTIO_NET_HDR_F_DATA_VALID regardless of whether
> VIRTIO_NET_F_GUEST_CSUM was negotiated, marking such packets as
> CHECKSUM_UNNECESSARY and skipping verification.
> 
> iPXE, however, does not negotiate GUEST_CSUM, ignores the DATA_VALID
> flag entirely, and always verifies checksums. This caused TCP
> connections to fail: the SYN-ACK had a zero TCP checksum, iPXE rejected
> it, and the connection timed out in SYN_RCVD.
> 
> Adding --pcap happened to mask the bug, because the pcap code path
> forces checksum computation to ensure correct captures.
> 
> Offer VIRTIO_NET_F_GUEST_CSUM in the device features, and only skip
> checksum computation when the guest has actually negotiated it. When
> GUEST_CSUM is not negotiated, always compute valid checksums as required
> by the specification.
> 
> We keep setting VIRTIO_NET_HDR_F_DATA_VALID unconditionally in
> VU_HEADER: when GUEST_CSUM is negotiated, the flag lets the guest skip
> checksum verification; when it is not, the spec says the guest should
> ignore the flags field, so setting it is harmless.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp_vu.c     | 8 ++++++--
>  udp_vu.c     | 7 ++++---
>  vhost_user.c | 1 +
>  3 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index 1927b14e0962..49d39e7de201 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -126,6 +126,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
>  	size_t optlen, hdrlen, iov_cnt, iov_used;
>  	struct vu_virtq_element flags_elem[2];
> +	uint32_t csum_flags = IP4_CSUM;
>  	struct iovec flags_iov[64];
>  	struct tcp_syn_opts opts;
>  	struct iov_tail payload;
> @@ -137,6 +138,9 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	int elem_cnt;
>  	int ret;
>  
> +	if (*c->pcap || !vu_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
> +		csum_flags |= TCP_CSUM;
> +
>  	hdrlen = tcp_vu_hdrlen(CONN_V6(conn));
>  
>  	elem_cnt = vu_collect(vdev, vq, &flags_elem[0], 1,
> @@ -175,7 +179,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	iov_from_buf(payload.iov, payload.cnt, payload.off, &opts, optlen);
>  	tcp_fill_headers(c, conn, &eh, CONN_V4(conn) ? &ip4h : NULL,
>  			 CONN_V6(conn) ? &ip6h : NULL, &th, &payload,
> -			 optlen, IP4_CSUM | (*c->pcap ? TCP_CSUM : 0), seq);
> +			 optlen, csum_flags, seq);
>  
>  	vu_pad(flags_elem[0].in_sg, iov_cnt, hdrlen + optlen);
>  	vu_flush(vdev, vq, flags_elem, elem_cnt, hdrlen + optlen);
> @@ -516,7 +520,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  
>  	hdrlen = tcp_vu_hdrlen(v6);
>  	check = IP4_CSUM;
> -	if (*c->pcap)
> +	if (*c->pcap || !vu_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
>  		check |= TCP_CSUM;
>  	for (i = 0, previous_dlen = -1; i < frame_cnt; i++) {
>  		struct iovec *iov = &iov_vu[frame[i].idx_iovec];
> diff --git a/udp_vu.c b/udp_vu.c
> index 5bc9509a1b98..ed888a2baab3 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -234,12 +234,13 @@ void udp_vu_sock_to_tap(const struct ctx *c, int s, int n, flow_sidx_t tosidx)
>  		if (iov_cnt > 0) {
>  			struct iov_tail data = IOV_TAIL(iov_vu, iov_cnt, VNET_HLEN);
>  			size_t l4len = udp_vu_prepare(c, &data, toside, dlen);
> -			if (*c->pcap) {
> +			if (!vu_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM) ||
> +			    *c->pcap)
>  				udp_vu_csum(toside, &data, l4len);
> +			vu_pad(iov_vu, iov_cnt, hdrlen + dlen);
> +			if (*c->pcap)
>  				pcap_iov(iov_vu, iov_cnt, VNET_HLEN,
>  					 hdrlen + dlen - VNET_HLEN);
> -			}
> -			vu_pad(iov_vu, iov_cnt, hdrlen + dlen);
>  			vu_flush(vdev, vq, elem, elem_used, hdrlen + dlen);
>  			vu_queue_notify(vdev, vq);
>  		}
> diff --git a/vhost_user.c b/vhost_user.c
> index f062badd3311..a1259c2624c0 100644
> --- a/vhost_user.c
> +++ b/vhost_user.c
> @@ -322,6 +322,7 @@ static bool vu_get_features_exec(struct vu_dev *vdev,
>  {
>  	uint64_t features =
>  		1ULL << VIRTIO_F_VERSION_1 |
> +		1ULL << VIRTIO_NET_F_GUEST_CSUM |
>  		1ULL << VIRTIO_NET_F_MRG_RXBUF |
>  		1ULL << VHOST_F_LOG_ALL |
>  		1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
> -- 
> 2.53.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]