[PATCH] vu_common: Always set num_buffers in virtio-net header

[PATCH] vu_common: Always set num_buffers in virtio-net header

[Laurent Vivier]

Legacy virtio used two different header formats: struct virtio_net_hdr
(10 bytes) when VIRTIO_NET_F_MRG_RXBUF was not negotiated, and
struct virtio_net_hdr_mrg_rxbuf (12 bytes) when it was.  The
num_buffers field only existed in the larger header.

Modern virtio (VIRTIO_F_VERSION_1, i.e. virtio 1.0+) always uses the
12-byte struct virtio_net_hdr_mrg_rxbuf header regardless of whether
VIRTIO_NET_F_MRG_RXBUF is negotiated, so num_buffers is always present
in the header.  passt only supports modern virtio and dies if
VIRTIO_F_VERSION_1 is not negotiated (vhost_user.c), and VNET_HLEN is
unconditionally defined as sizeof(struct virtio_net_hdr_mrg_rxbuf).

The virtio specification (v1.1, section 5.1.6) requires that:

  "The device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has
   not been negotiated."

vu_set_vnethdr() only set num_buffers when VIRTIO_NET_F_MRG_RXBUF was
negotiated.  When it was not, num_buffers was left uninitialised,
violating the spec.

Since vu_collect() already limits buffer collection to a single element
when VIRTIO_NET_F_MRG_RXBUF is not negotiated, num_buffers passed by
callers is guaranteed to be 1 in that case.  We can therefore
unconditionally set num_buffers, which makes the vdev parameter
unnecessary.

Drop the vdev parameter from vu_set_vnethdr() and update all callers.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 tcp_vu.c    |  4 ++--
 udp_vu.c    |  2 +-
 vu_common.c | 13 ++++++-------
 vu_common.h |  4 +---
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/tcp_vu.c b/tcp_vu.c
index bb05fbf45826..88be232dca66 100644
--- a/tcp_vu.c
+++ b/tcp_vu.c
@@ -97,7 +97,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
 	ASSERT(flags_elem[0].in_sg[0].iov_len >=
 	       MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN));
 
-	vu_set_vnethdr(vdev, flags_elem[0].in_sg[0].iov_base, 1);
+	vu_set_vnethdr(flags_elem[0].in_sg[0].iov_base, 1);
 
 	eh = vu_eth(flags_elem[0].in_sg[0].iov_base);
 
@@ -452,7 +452,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
 		bool push = i == head_cnt - 1;
 		size_t l2len;
 
-		vu_set_vnethdr(vdev, iov->iov_base, buf_cnt);
+		vu_set_vnethdr(iov->iov_base, buf_cnt);
 
 		/* The IPv4 header checksum varies only with dlen */
 		if (previous_dlen != dlen)
diff --git a/udp_vu.c b/udp_vu.c
index 51f3718f5925..3520f89e5671 100644
--- a/udp_vu.c
+++ b/udp_vu.c
@@ -127,7 +127,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s,
 	l2len = *dlen + hdrlen - VNET_HLEN;
 	vu_pad(&iov_vu[0], l2len);
 
-	vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used);
+	vu_set_vnethdr(iov_vu[0].iov_base, iov_used);
 
 	/* release unused buffers */
 	vu_queue_rewind(vq, iov_cnt - iov_used);
diff --git a/vu_common.c b/vu_common.c
index aa14598ea028..5f2ce18e5b71 100644
--- a/vu_common.c
+++ b/vu_common.c
@@ -121,17 +121,16 @@ int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
 
 /**
  * vu_set_vnethdr() - set virtio-net headers
- * @vdev:		vhost-user device
  * @vnethdr:		Address of the header to set
  * @num_buffers:	Number of guest buffers of the frame
  */
-void vu_set_vnethdr(const struct vu_dev *vdev,
-		    struct virtio_net_hdr_mrg_rxbuf *vnethdr,
-		    int num_buffers)
+void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers)
 {
 	vnethdr->hdr = VU_HEADER;
-	if (vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
-		vnethdr->num_buffers = htole16(num_buffers);
+	/* Note: if VIRTIO_NET_F_MRG_RXBUF is not negotiated,
+	 * num_buffers must be 1
+	 */
+	vnethdr->num_buffers = htole16(num_buffers);
 }
 
 /**
@@ -269,7 +268,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
 		goto err;
 	}
 
-	vu_set_vnethdr(vdev, in_sg[0].iov_base, elem_cnt);
+	vu_set_vnethdr(in_sg[0].iov_base, elem_cnt);
 
 	total -= VNET_HLEN;
 
diff --git a/vu_common.h b/vu_common.h
index 052aff710502..20868a7f62ce 100644
--- a/vu_common.h
+++ b/vu_common.h
@@ -49,9 +49,7 @@ void vu_init_elem(struct vu_virtq_element *elem, struct iovec *iov,
 int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
 	       struct vu_virtq_element *elem, int max_elem, size_t size,
 	       size_t *collected);
-void vu_set_vnethdr(const struct vu_dev *vdev,
-		    struct virtio_net_hdr_mrg_rxbuf *vnethdr,
-		    int num_buffers);
+void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers);
 void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
 	      struct vu_virtq_element *elem, int elem_cnt);
 void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,
-- 
2.53.0

Re: [PATCH] vu_common: Always set num_buffers in virtio-net header

[David Gibson]

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

On Tue, Mar 03, 2026 at 04:17:34PM +0100, Laurent Vivier wrote:
> Legacy virtio used two different header formats: struct virtio_net_hdr
> (10 bytes) when VIRTIO_NET_F_MRG_RXBUF was not negotiated, and
> struct virtio_net_hdr_mrg_rxbuf (12 bytes) when it was.  The
> num_buffers field only existed in the larger header.
> 
> Modern virtio (VIRTIO_F_VERSION_1, i.e. virtio 1.0+) always uses the
> 12-byte struct virtio_net_hdr_mrg_rxbuf header regardless of whether
> VIRTIO_NET_F_MRG_RXBUF is negotiated, so num_buffers is always present
> in the header.  passt only supports modern virtio and dies if
> VIRTIO_F_VERSION_1 is not negotiated (vhost_user.c), and VNET_HLEN is
> unconditionally defined as sizeof(struct virtio_net_hdr_mrg_rxbuf).
> 
> The virtio specification (v1.1, section 5.1.6) requires that:
> 
>   "The device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has
>    not been negotiated."
> 
> vu_set_vnethdr() only set num_buffers when VIRTIO_NET_F_MRG_RXBUF was
> negotiated.  When it was not, num_buffers was left uninitialised,
> violating the spec.
> 
> Since vu_collect() already limits buffer collection to a single element
> when VIRTIO_NET_F_MRG_RXBUF is not negotiated, num_buffers passed by
> callers is guaranteed to be 1 in that case.  We can therefore
> unconditionally set num_buffers, which makes the vdev parameter
> unnecessary.
> 
> Drop the vdev parameter from vu_set_vnethdr() and update all callers.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

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

> ---
>  tcp_vu.c    |  4 ++--
>  udp_vu.c    |  2 +-
>  vu_common.c | 13 ++++++-------
>  vu_common.h |  4 +---
>  4 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/tcp_vu.c b/tcp_vu.c
> index bb05fbf45826..88be232dca66 100644
> --- a/tcp_vu.c
> +++ b/tcp_vu.c
> @@ -97,7 +97,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags)
>  	ASSERT(flags_elem[0].in_sg[0].iov_len >=
>  	       MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN));
>  
> -	vu_set_vnethdr(vdev, flags_elem[0].in_sg[0].iov_base, 1);
> +	vu_set_vnethdr(flags_elem[0].in_sg[0].iov_base, 1);
>  
>  	eh = vu_eth(flags_elem[0].in_sg[0].iov_base);
>  
> @@ -452,7 +452,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
>  		bool push = i == head_cnt - 1;
>  		size_t l2len;
>  
> -		vu_set_vnethdr(vdev, iov->iov_base, buf_cnt);
> +		vu_set_vnethdr(iov->iov_base, buf_cnt);
>  
>  		/* The IPv4 header checksum varies only with dlen */
>  		if (previous_dlen != dlen)
> diff --git a/udp_vu.c b/udp_vu.c
> index 51f3718f5925..3520f89e5671 100644
> --- a/udp_vu.c
> +++ b/udp_vu.c
> @@ -127,7 +127,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s,
>  	l2len = *dlen + hdrlen - VNET_HLEN;
>  	vu_pad(&iov_vu[0], l2len);
>  
> -	vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used);
> +	vu_set_vnethdr(iov_vu[0].iov_base, iov_used);
>  
>  	/* release unused buffers */
>  	vu_queue_rewind(vq, iov_cnt - iov_used);
> diff --git a/vu_common.c b/vu_common.c
> index aa14598ea028..5f2ce18e5b71 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -121,17 +121,16 @@ int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
>  
>  /**
>   * vu_set_vnethdr() - set virtio-net headers
> - * @vdev:		vhost-user device
>   * @vnethdr:		Address of the header to set
>   * @num_buffers:	Number of guest buffers of the frame
>   */
> -void vu_set_vnethdr(const struct vu_dev *vdev,
> -		    struct virtio_net_hdr_mrg_rxbuf *vnethdr,
> -		    int num_buffers)
> +void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers)
>  {
>  	vnethdr->hdr = VU_HEADER;
> -	if (vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
> -		vnethdr->num_buffers = htole16(num_buffers);
> +	/* Note: if VIRTIO_NET_F_MRG_RXBUF is not negotiated,
> +	 * num_buffers must be 1
> +	 */
> +	vnethdr->num_buffers = htole16(num_buffers);
>  }
>  
>  /**
> @@ -269,7 +268,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size)
>  		goto err;
>  	}
>  
> -	vu_set_vnethdr(vdev, in_sg[0].iov_base, elem_cnt);
> +	vu_set_vnethdr(in_sg[0].iov_base, elem_cnt);
>  
>  	total -= VNET_HLEN;
>  
> diff --git a/vu_common.h b/vu_common.h
> index 052aff710502..20868a7f62ce 100644
> --- a/vu_common.h
> +++ b/vu_common.h
> @@ -49,9 +49,7 @@ void vu_init_elem(struct vu_virtq_element *elem, struct iovec *iov,
>  int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq,
>  	       struct vu_virtq_element *elem, int max_elem, size_t size,
>  	       size_t *collected);
> -void vu_set_vnethdr(const struct vu_dev *vdev,
> -		    struct virtio_net_hdr_mrg_rxbuf *vnethdr,
> -		    int num_buffers);
> +void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers);
>  void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq,
>  	      struct vu_virtq_element *elem, int elem_cnt);
>  void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,
> -- 
> 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 --]

Re: [PATCH] vu_common: Always set num_buffers in virtio-net header

[Stefano Brivio]

On Tue,  3 Mar 2026 16:17:34 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> Legacy virtio used two different header formats: struct virtio_net_hdr
> (10 bytes) when VIRTIO_NET_F_MRG_RXBUF was not negotiated, and
> struct virtio_net_hdr_mrg_rxbuf (12 bytes) when it was.  The
> num_buffers field only existed in the larger header.
> 
> Modern virtio (VIRTIO_F_VERSION_1, i.e. virtio 1.0+) always uses the
> 12-byte struct virtio_net_hdr_mrg_rxbuf header regardless of whether
> VIRTIO_NET_F_MRG_RXBUF is negotiated, so num_buffers is always present
> in the header.  passt only supports modern virtio and dies if
> VIRTIO_F_VERSION_1 is not negotiated (vhost_user.c), and VNET_HLEN is
> unconditionally defined as sizeof(struct virtio_net_hdr_mrg_rxbuf).
> 
> The virtio specification (v1.1, section 5.1.6) requires that:
> 
>   "The device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has
>    not been negotiated."
> 
> vu_set_vnethdr() only set num_buffers when VIRTIO_NET_F_MRG_RXBUF was
> negotiated.  When it was not, num_buffers was left uninitialised,
> violating the spec.
> 
> Since vu_collect() already limits buffer collection to a single element
> when VIRTIO_NET_F_MRG_RXBUF is not negotiated, num_buffers passed by
> callers is guaranteed to be 1 in that case.  We can therefore
> unconditionally set num_buffers, which makes the vdev parameter
> unnecessary.
> 
> Drop the vdev parameter from vu_set_vnethdr() and update all callers.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Applied.

-- 
Stefano