From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id E71F15A005E for ; Tue, 29 Nov 2022 06:59:47 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4NLs9p2Gvnz4xG9; Tue, 29 Nov 2022 16:59:42 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1669701582; bh=VfTCaLvaHxl/Z7gyALntbpSeX29gPx6gV0EvmQxHTM0=; h=Date:From:To:Subject:References:In-Reply-To:From; b=JMB96kabx9yQ/MPJMDu+Xx3p0UBrlfOJqJ8vaTYvbT31RXh81u9bZHvrr4NU0N42y aFjEAC5cwB1W/IyZvXl+4rlEDiJOVxL7OKOuAM1yKP0a4F/IaMi1pCkBsWRh64KWzC YNA3uyUfOYBlDJXCF7hxu8apYYqTy55xhESnVSN4= Date: Tue, 29 Nov 2022 16:59:33 +1100 From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: Re: [PATCH v3 16/16] udp: Correct splice forwarding when receiving from multiple sources Message-ID: References: <20221125072916.3060938-1-david@gibson.dropbear.id.au> <20221125072916.3060938-17-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="dHpblfZ8nFSgbEI6" Content-Disposition: inline In-Reply-To: <20221125072916.3060938-17-david@gibson.dropbear.id.au> Message-ID-Hash: 25J47LYLBFUXKTNOY6YDOIDBRHISKT4G X-Message-ID-Hash: 25J47LYLBFUXKTNOY6YDOIDBRHISKT4G 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: --dHpblfZ8nFSgbEI6 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Nov 25, 2022 at 06:29:16PM +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 Oops, replied to the wrong version. v3 still has the nasty bug, do not apply until I send a v4. > --- > udp.c | 27 +++++++++++++++++++-------- > 1 file changed, 19 insertions(+), 8 deletions(-) >=20 > diff --git a/udp.c b/udp.c > index 899dcbb..eaf7a52 100644 > --- a/udp.c > +++ b/udp.c > @@ -571,9 +571,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; > @@ -609,12 +609,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); Buffer overflow here, because I didn't check i + m against n. --=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 --dHpblfZ8nFSgbEI6 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmOFn7sACgkQgypY4gEw YSLoyg//XCVFSibXAwPe9JDzXboyrJwiT8u73+vT1PI6fm5WHrJ05kG2B5LGe7hG q61x75rTKXT9+p4B6rCyk6zupMNQutYEw/UeqPgIfFYk/FTqM6UkZXY30lHsc1lb F/8abxDTdTtcdxm1CCD/9L8vf/CESii3AmKVMRwYMp55azzMUa6v0wqNGINW8dl1 BoI2hbuK3Tx2jizj6LY1J9nsZu9n7GWgLOBkHz2zZbCJBQRzDP7RA3QqrujBUTrP wTCyCUTpLsdp5Ybp7dpY0FCot+CJuJuSN56d1PFLhUINOFamqg8KIzLxKMSV8imC XzFygi3oLGK+Do3Mf5c3m2elt27KUnZLL0yDjLBas7PoRi2PuRDWr2llfDDgjaEF Qarnu/l1G2K0bz8l42Kg3S6se8LgtJN4jVy6uM0E+mB638NxwMhT6zA35nGWaNjY EZ0JVik30im0RhHCufRoZQAeJZzh2gUimcD8EXZxdBl2DJL8ti0Po831Jmebll7e DYO3iww5Ex/k45EChj94So5eJ+jKGMSdmy+Pv8K6oux4m2JSRTe+BSXingXuYBB7 ytweKJ3SZtjJv3KYTRRQOtF9lgjyIXrpI1o57fka1Hw2QVLQCqgdEIpUQuMUoJox kZVFLYBGWSDDJVGqUCqN2CqwexcZ+RbTQa8eD5MwMT47pjchBJY= =8oGA -----END PGP SIGNATURE----- --dHpblfZ8nFSgbEI6--