From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id AE83D5A005E for ; Tue, 29 Nov 2022 06:55:48 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4NLs5B5MQ0z4xG9; Tue, 29 Nov 2022 16:55:42 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1669701342; bh=KF5tHnnrcdcm8e4nfT1DCvSPLurLEbrIrvZRsc6IqUk=; h=Date:From:To:Subject:References:In-Reply-To:From; b=aCMpa6uIV6m/78PZWOGYwsd9wjkmVRkhunOF/iAsT2lmilsb1JMP+DD5tTHSJePe+ udd/Tp3x0d5O8mZzlzTNpPlgyJSety+DKneWUfZeAWBSxHiGAdvYcMrftdqZbpkveU INySJnnU4eM40bk/lqmG+X16+Edu/AtglW9RJFpU= Date: Tue, 29 Nov 2022 16:55:38 +1100 From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: Re: [PATCH v2 16/16] udp: Correct splice forwarding when receiving from multiple sources Message-ID: References: <20221124011659.1024901-1-david@gibson.dropbear.id.au> <20221124011659.1024901-17-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="fr8XzL69VcDABuBI" Content-Disposition: inline In-Reply-To: <20221124011659.1024901-17-david@gibson.dropbear.id.au> Message-ID-Hash: QX6DYCJPL6737LQ5YGVT5ZJJZPYOEBWC X-Message-ID-Hash: QX6DYCJPL6737LQ5YGVT5ZJJZPYOEBWC 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 X-Mailman-Version: 3.3.3 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: --fr8XzL69VcDABuBI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Nov 24, 2022 at 12:16:59PM +1100, David Gibson wrote: > udp_sock_handler_splice() reads a whole batch of datagrams at once with > recvmmsg(). It then forwards them all via a single socket on the other > side, based on the source port. >=20 > However, it's entirely possible that the datagrams in the set have > different source ports, and thus ought to be forwarded via different > sockets on the destination side. In fact this situation arises with the > iperf -P4 throughput tests in our own test suite. AFAICT we only get away > with this because iperf3 is strictly one way and doesn't send reply packe= ts > which would be misdirected because of the incorrect source ports. >=20 > Alter udp_sock_handler_splice() to split the packets it receives into > batches with the same source address and send each batch with a separate > sendmmsg(). >=20 > For now we only look for already contiguous batches, which means that if > there are multiple active flows interleaved this is likely to degenerate > to batches of size 1. For now this is the simplest way to correct the > behaviour and we can try to optimize later. >=20 > Signed-off-by: David Gibson This has a nasty bug, do not apply. I'll send a v3 shortly. Specifically: > --- > udp.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) >=20 > diff --git a/udp.c b/udp.c > index 2311e7d..ee5c2c5 100644 > --- a/udp.c > +++ b/udp.c > @@ -572,9 +572,9 @@ static void udp_splice_sendfrom(const struct ctx *c, = struct mmsghdr *mmh, int n, > static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref= ref, > uint32_t events, const struct timespec *now) > { > - in_port_t src, dst =3D ref.r.p.udp.udp.port; > + in_port_t dst =3D ref.r.p.udp.udp.port; > + int v6 =3D ref.r.p.udp.udp.v6, n, i, m; > struct mmsghdr *mmh_recv, *mmh_send; > - int v6 =3D ref.r.p.udp.udp.v6, n, i; > =20 > if (!(events & EPOLLIN)) > return; > @@ -610,12 +610,23 @@ static void udp_sock_handler_splice(const struct ct= x *c, union epoll_ref ref, > }); > } > =20 > - for (i =3D 0; i < n; i++) > - mmh_send[i].msg_hdr.msg_iov->iov_len =3D mmh_recv[i].msg_len; > - > - src =3D sa_port(v6, mmh_recv[0].msg_hdr.msg_name); > - udp_splice_sendfrom(c, mmh_send, n, src, ref.r.p.udp.udp.port, > - v6, ref.r.p.udp.udp.ns, ref.r.p.udp.udp.orig, now); > + for (i =3D 0; i < n; i +=3D m) { > + const struct mmsghdr *mmh =3D &mmh_recv[i]; > + in_port_t src =3D sa_port(v6, mmh->msg_hdr.msg_name); > + > + m =3D 0; > + do { > + mmh_send[i + m].msg_hdr.msg_iov->iov_len =3D mmh->msg_len; > + mmh++; > + m++; > + } while (sa_port(v6, mmh->msg_hdr.msg_name) =3D=3D src); I missed a bounds check here, so this inner loop can overrun the mmh array. --=20 David Gibson | 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 --fr8XzL69VcDABuBI Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmOFntEACgkQgypY4gEw YSLw6g//Yw7Ff3Mx8GOHvHO/9nryuZaq6pSSktcqI7bEiXS+bd7NNVkYqxIyQZQq dEOS+TE/WSe6meg/rln3kACci8XsSgKFgEDC+Fj83MW/N+yjjDYRfvKD3E1pRRTG o3wHGC3aTW8UtNNpebYQl533BX6bmkbLmsWZYvyb18vnz9sF7vBhsLUdJolydKwN 65EXLa7ybSoIYXIxyrGMKTPC78ajLwdlu7V6NTMFHt/pLvOu/rVmKoPnAakMrye2 kDEkSGgNlD3ENNUEbIcjEWM2XVWtpwJgFFrtIJx7GLPfdnAXWbY64/RpOa32kcVA ix8EMjRro9cvmsNYq+eRsIfKjnjBww5tboTCSGNyCXsjzZRIto7WFIFwrdwHsZ1E hj3trKfnzQz6F7TnE9QOeypvfb2lEuF2E2F/BHPJfQryJN526OVOFHTkSzk2m3BB JfpdfPmxHzXLDGxxPrAxxeT+OUmFNJjF1JGXT2TAkVlcBai2DNtgXiuK+q8Lwg2y umSo8c91dzcQEJTBbKjSGeQVxYfSFuDi2/NYseH1n6T29gFf3j3JWMo/GzJsW/DY /HsDH8WLVjw3F9KIAnhEFX/9Nb1spzrH/+PSQOuX0cifXeU+ZphWlA1LTa7+9M4e GudiWWd4Zz1+A2YXkR0hRssKxLgSALMlWNNbPoTDPdOSzNN3inE= =YQuG -----END PGP SIGNATURE----- --fr8XzL69VcDABuBI--