From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202512 header.b=SlFCIO6s; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id E71FA5A0623 for ; Mon, 08 Dec 2025 03:16:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202512; t=1765160214; bh=a9XVrnYz3U3xazfgIfOKkRKrFK9zOn5bac0eCIEf0dI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SlFCIO6sA3+lxJ3tqeTyqNLshPrk//Lf5zh34CZeHPcuxDVhAXEKvJqaBqKhMUFQ9 PXVJWbIxN7+JNFeqzVpz49u2yCXXyev496ErWfvjbfVmETWVG9jQXmlwfwou0R6dm4 Goh5izzoRd+h6d/XrOTw+yzlNFa359/BxqARUZY192rt6BbySKkoqYiAd1AfBFMdF5 rsJt584ef3OyLkJQ5+BSy01tLCBF3MJPOvxje3b7aQtzC4YAEscmg88qYMCu3wKxk8 pXyxZAXnjO89Xk3FmJhMWpn8N/knyjLH0OzInYVHpDcL3Ej+2TG/p8IjzGagbfInIf cz5mUyFENpubw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4dPlwk0Mvkz4wMG; Mon, 08 Dec 2025 13:16:54 +1100 (AEDT) Date: Mon, 8 Dec 2025 13:16:47 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 5/5] tcp, udp: Pad batched frames for vhost-user modes to 60 bytes (802.3 minimum) Message-ID: References: <20251206012657.553742-1-sbrivio@redhat.com> <20251206012657.553742-6-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="QzzO91ApA6f1eZWC" Content-Disposition: inline In-Reply-To: <20251206012657.553742-6-sbrivio@redhat.com> Message-ID-Hash: RQDM5SOIYHLKG2P4TT4YB56BWZKFJASK X-Message-ID-Hash: RQDM5SOIYHLKG2P4TT4YB56BWZKFJASK X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --QzzO91ApA6f1eZWC Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Dec 06, 2025 at 02:26:57AM +0100, Stefano Brivio wrote: > For both TCP and UDP, instead of just expecting the first provided > buffer to be large enough to contain the headers we need (from 42 > bytes for UDP data over IPv4 to 82 bytes for TCP with options over > IPv6), change that assumption to make sure that buffers are anyway > at least ETH_ZLEN-sized buffers (60 bytes). >=20 > This looks reasonable because there are no known vhost-user > hypervisor implementations that would give us smaller buffers than > that, and we would anyway hit an assertion failure with IPv6 if we > ever had less than 60 bytes per buffer. >=20 > At this point, all we need to do is to pad the first (and only) > buffer, should data and headers use less than that. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D166 > Signed-off-by: Stefano Brivio I find the way iov_len changes through the whole path a bit confusing: 1. First, it represents buffer length we get from vu_collect 2. We check that it has room for a padded frame 3. We decrease it based on the _unpadded_ frame 4. We write the padding to the buffer 5. We increase it again to include the padding The last step is safe because of the earlier check. I think it's now adequately clear that it's safe, but it's not _super_ clear because there are still some in between steps. I'd kind of prefer to not shrink iov_len below the value for the padded frame size in the first place. But, this does the job, and I don't really want to hold this up any longer so, Reviewed-by: David Fibson > --- > tcp_vu.c | 17 ++++++++++++----- > udp_vu.c | 8 ++++++-- > vu_common.c | 14 ++++++++++++++ > vu_common.h | 1 + > 4 files changed, 33 insertions(+), 7 deletions(-) >=20 > diff --git a/tcp_vu.c b/tcp_vu.c > index 1c81ce3..db9db78 100644 > --- a/tcp_vu.c > +++ b/tcp_vu.c > @@ -91,12 +91,12 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_= tap_conn *conn, int flags) > vu_set_element(&flags_elem[0], NULL, &flags_iov[0]); > =20 > elem_cnt =3D vu_collect(vdev, vq, &flags_elem[0], 1, > - hdrlen + sizeof(struct tcp_syn_opts), NULL); > + MAX(hdrlen + sizeof(*opts), ETH_ZLEN), NULL); > if (elem_cnt !=3D 1) > return -1; > =20 > ASSERT(flags_elem[0].in_sg[0].iov_len >=3D > - hdrlen + sizeof(struct tcp_syn_opts)); > + MAX(hdrlen + sizeof(*opts), ETH_ZLEN)); > =20 > vu_set_vnethdr(vdev, flags_elem[0].in_sg[0].iov_base, 1); > =20 > @@ -138,6 +138,8 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_= tap_conn *conn, int flags) > tcp_fill_headers(c, conn, NULL, eh, ip4h, ip6h, th, &payload, > NULL, seq, !*c->pcap); > =20 > + vu_pad(&flags_elem[0].in_sg[0], hdrlen + optlen); > + > if (*c->pcap) { > pcap_iov(&flags_elem[0].in_sg[0], 1, > sizeof(struct virtio_net_hdr_mrg_rxbuf)); > @@ -211,7 +213,8 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, = struct vu_virtq *vq, > =20 > cnt =3D vu_collect(vdev, vq, &elem[elem_cnt], > VIRTQUEUE_MAX_SIZE - elem_cnt, > - MIN(mss, fillsize) + hdrlen, &frame_size); > + MAX(MIN(mss, fillsize) + hdrlen, ETH_ZLEN), > + &frame_size); > if (cnt =3D=3D 0) > break; > =20 > @@ -254,6 +257,7 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, = struct vu_virtq *vq, > =20 > len -=3D iov->iov_len; > } > + > /* adjust head count */ > while (*head_cnt > 0 && head[*head_cnt - 1] >=3D i) > (*head_cnt)--; > @@ -301,9 +305,9 @@ static void tcp_vu_prepare(const struct ctx *c, struc= t tcp_tap_conn *conn, > struct ethhdr *eh; > =20 > /* we guess the first iovec provided by the guest can embed > - * all the headers needed by L2 frame > + * all the headers needed by L2 frame, including any padding > */ > - ASSERT(iov[0].iov_len >=3D hdrlen); > + ASSERT(iov[0].iov_len >=3D MAX(hdrlen, ETH_ZLEN)); > =20 > eh =3D vu_eth(base); > =20 > @@ -456,6 +460,9 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct= tcp_tap_conn *conn) > =20 > tcp_vu_prepare(c, conn, iov, buf_cnt, &check, !*c->pcap, push); > =20 > + /* Pad first/single buffer only, it's at least ETH_ZLEN long */ > + vu_pad(iov, dlen + hdrlen); > + > if (*c->pcap) { > pcap_iov(iov, buf_cnt, > sizeof(struct virtio_net_hdr_mrg_rxbuf)); > diff --git a/udp_vu.c b/udp_vu.c > index 099677f..c30dcf9 100644 > --- a/udp_vu.c > +++ b/udp_vu.c > @@ -72,8 +72,8 @@ static int udp_vu_sock_recv(const struct ctx *c, struct= vu_virtq *vq, int s, > { > const struct vu_dev *vdev =3D c->vdev; > int iov_cnt, idx, iov_used; > + size_t off, hdrlen, l2len; > struct msghdr msg =3D { 0 }; > - size_t off, hdrlen; > =20 > ASSERT(!c->no_udp); > =20 > @@ -90,7 +90,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct= vu_virtq *vq, int s, > return 0; > =20 > /* reserve space for the headers */ > - ASSERT(iov_vu[0].iov_len >=3D hdrlen); > + ASSERT(iov_vu[0].iov_len >=3D MAX(hdrlen, ETH_ZLEN)); > iov_vu[0].iov_base =3D (char *)iov_vu[0].iov_base + hdrlen; > iov_vu[0].iov_len -=3D hdrlen; > =20 > @@ -116,6 +116,10 @@ static int udp_vu_sock_recv(const struct ctx *c, str= uct vu_virtq *vq, int s, > iov_vu[idx].iov_len =3D off; > iov_used =3D idx + !!off; > =20 > + /* pad frame to 60 bytes: first buffer is at least ETH_ZLEN long */ > + l2len =3D *dlen + hdrlen - sizeof(struct virtio_net_hdr_mrg_rxbuf); > + vu_pad(&iov_vu[0], l2len); > + > vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used); > =20 > /* release unused buffers */ > diff --git a/vu_common.c b/vu_common.c > index ce61fa6..c682498 100644 > --- a/vu_common.c > +++ b/vu_common.c > @@ -293,3 +293,17 @@ err: > =20 > return -1; > } > + > +/** > + * vu_pad() - Pad 802.3 frame to minimum length (60 bytes) if needed > + * @iov: Buffer in iovec array where end of 802.3 frame is stored > + * @l2len: Layer-2 length already filled in frame > + */ > +void vu_pad(struct iovec *iov, size_t l2len) > +{ > + if (l2len >=3D ETH_ZLEN) > + return; > + > + memset((char *)iov->iov_base + iov->iov_len, 0, ETH_ZLEN - l2len); > + iov->iov_len +=3D ETH_ZLEN - l2len; > +} > diff --git a/vu_common.h b/vu_common.h > index c0883b2..27fe7e0 100644 > --- a/vu_common.h > +++ b/vu_common.h > @@ -57,5 +57,6 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virt= q *vq, > void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, > const struct timespec *now); > int vu_send_single(const struct ctx *c, const void *buf, size_t size); > +void vu_pad(struct iovec *iov, size_t l2len); > =20 > #endif /* VU_COMMON_H */ > --=20 > 2.43.0 >=20 --=20 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 --QzzO91ApA6f1eZWC Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmk2NQAACgkQzQJF27ox 2GfoUA/7BvL4rXIFQTckhbx7hwaFglabuSLqT1VdEWzJIQ0eDrI7TOA1RRQyRtzy 2CAaMZyVt47c1t26NLS2RSj7QCJK776P9NGbccEygw7mnU/HkzUKp0QEQZJL0FBb 9G3l4dX3xM/z3EewlxNcyLHn4vlkuwlmq90NZrDXYkouGvK7K4Xxu04au+5SH28m AtvFpZ7iObpdCB3DvAkdNfjQPBSh9S9w0AuDqTuKgAQu73tvuFvwapKYulYG9/Wu Roil6BvHle2nh0ytgnfC0pS6Wvb5ZWLXveAikbYd8gXz8kDzqAWfwFa/x30Kc9i1 T4nSW/+fPVtNaXZ4QbXMccHxBD8l61jbDUHx1iW+LdSXL8nR79rGtbiPBxpR0N34 5luS+1BaNhFyESmtb9ma4EOZRjiB9rOfPNArR0NPewuJHwEwMX5TnbSCGThBiuCh k00H68MuegEimEAXRKhB9h1lsnqz2Rtosy52Ck5InF8fle1vhQsFH4c/AgfA0Aaj QqMJMlb2fjJGuQ72j9l7XWIGCSBfqI3VS9rJkwb1hcnXyvx5FOjgKOQ1/rFpnQ2C FUxRlnQcLdI965arPOtwd4QltHoQmv1TUNJO3dTuAf4Mf/u+Un2BON/DPxcSGmrY 1LuKscLnjhPphigvZlzusixwdNTmYSAISn9dujU+yhwKblj+gEU= =D18n -----END PGP SIGNATURE----- --QzzO91ApA6f1eZWC--