From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Gibson To: passt-dev@passt.top Subject: Re: [PATCH 1/2] udp: Don't drop zero-length outbound UDP packets Date: Tue, 13 Sep 2022 19:30:43 +1000 Message-ID: In-Reply-To: <20220913100846.1bc77870@elisabeth> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============9182662549862717153==" --===============9182662549862717153== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Tue, Sep 13, 2022 at 10:08:46AM +0100, Stefano Brivio wrote: > On Tue, 13 Sep 2022 16:39:26 +1000 > David Gibson wrote: >=20 > > On Fri, Sep 09, 2022 at 06:06:59PM +0200, Stefano Brivio wrote: > > > On Fri, 9 Sep 2022 20:39:44 +1000 > > > David Gibson wrote: > > > =20 > > > > On Fri, Sep 09, 2022 at 11:26:58AM +0200, Stefano Brivio wrote: =20 > > > > > On Fri, 9 Sep 2022 14:27:13 +1000 > > > > > David Gibson wrote: > > > > > =20 > > > > > > udp_tap_handler() currently skips outbound packets if they have a= payload > > > > > > length of zero. This is not correct, since in a datagram protoco= l zero > > > > > > length packets still have meaning. =20 > > > > >=20 > > > > > Right, nice catch. As far as I can tell it's an issue I added with > > > > > commit bb708111833e ("treewide: Packet abstraction with mandatory > > > > > boundary checks"). > > > > > =20 > > > > > > Adjust this to correctly forward the zero-length packets by using= a msghdr > > > > > > with msg_iovlen =3D=3D 0. > > > > > >=20 > > > > > > Bugzilla: https://bugs.passt.top/show_bug.cgi?id=3D19 > > > > > >=20 > > > > > > Signed-off-by: David Gibson > > > > > > --- > > > > > > udp.c | 10 +++++----- > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > >=20 > > > > > > diff --git a/udp.c b/udp.c > > > > > > index c4ebecc..caa852a 100644 > > > > > > --- a/udp.c > > > > > > +++ b/udp.c > > > > > > @@ -1075,19 +1075,19 @@ int udp_tap_handler(struct ctx *c, int af= , const void *addr, > > > > > > uh_send =3D packet_get(p, i, 0, sizeof(*uh), &len); > > > > > > if (!uh_send) > > > > > > return p->count; > > > > > > + > > > > > > + mm[i].msg_hdr.msg_name =3D sa; > > > > > > + mm[i].msg_hdr.msg_namelen =3D sl; > > > > > > + count++; > > > > > > + > > > > > > if (!len) > > > > > > continue; > > > > > > =20 > > > > > > m[i].iov_base =3D (char *)(uh_send + 1); > > > > > > m[i].iov_len =3D len; =20 > > > > >=20 > > > > > I haven't tested this yet, but: > > > > >=20 > > > > > - shouldn't iov_len be set to 0 (moving also this line before)? Note > > > > > that I'm not initialising m > > > > >=20 > > > > > - shouldn't iov_base point to NULL to avoid noise from valgrind? = =20 > > > >=20 > > > > No, because with this change m[i] is entirely unreferenced by mm[]. > > > > =20 > > > > > Also: > > > > > =20 > > > > > > =20 > > > > > > - mm[i].msg_hdr.msg_name =3D sa; > > > > > > - mm[i].msg_hdr.msg_namelen =3D sl; > > > > > > - > > > > > > mm[i].msg_hdr.msg_iov =3D m + i; > > > > > > mm[i].msg_hdr.msg_iovlen =3D 1; =20 > > > > >=20 > > > > > ...I guess we should still go through those even if the size is zer= o, > > > > > because we're appending a message. If we don't, I would expect some > > > > > subsequent messages in the batch to be dropped (as many as zero siz= ed > > > > > packets we have). =20 > > > >=20 > > > > Here I'm relying on the fact that mm[] (unlike m[]) *is* initialized, > > > > so if we don't alter it here, msg_iov is NULL and msg_iovlen is 0. =20 > > > =20 > > > > I was looking at removing that initialization, but I haven't gotten > > > > that working yet. =20 > > >=20 > > > Oops, I see now. > > >=20 > > > So, I suppose that if you want to drop that initialisation, you might > > > need to zero msg_hdr.controllen as well. =20 > >=20 > > Duh. I completely failed to consider the other fields. I actually > > suspect msg_hdr.flags is the most vital one (without flags I don't > > know if it will examine control or controllen). >=20 > Hmm, if we're talking about msg_flags, it should be ignored on > sendmsg(), and only used for received messages flags (MSG_EOR, > MSG_TRUNC, MSG_CTRUNC, MSG_OOB, MSG_ERRQUEUE) on recvmsg(). Oh, right, I was mixing up msg_flags and the separate flags argument to sendmsg() and sendmmsg(). > But, >=20 > > But in any case I'm initializing them all now and it's working. >=20 > yes, I guess it's a good idea to avoid sending the kernel random bytes > there, in any case. Right. --=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 --===============9182662549862717153== Content-Type: application/pgp-signature Content-Transfer-Encoding: base64 Content-Disposition: attachment; filename="signature.asc" MIME-Version: 1.0 LS0tLS1CRUdJTiBQR1AgU0lHTkFUVVJFLS0tLS0KCmlRSXpCQUVCQ0FBZEZpRUVvVUx4V3U0L1dz MGRCK1h0Z3lwWTRnRXdZU0lGQW1NZ1Rid0FDZ2tRZ3lwWTRnRXcKWVNML2J3LytLUW0wWkRIZkF2 OGFTeDQyS2FwNGxzUGtlcDVsM1E5aFBQZmR3UTVPZGxKdGV6VmtXZEhSYVhzTAo5TDlUUVZ1aG44 dm5GZHNQSlZhZGFGOHdJdGw5UGxpNWxHWHIxSUFrL1ZTTHRFVDBLZW9lOG13T1o3dEFhV1FwCkRD dytsZjRtT0V3YlZSNEQ0bTl0RE5pWGJYRUx3OVIzUnM5WnR2R00xWm8wVEtMdWo0L3ZrcDd6S2xx WFBkYTgKWkVOVit0ZXE3eFdXM0JsUEYyaCtsU2FXVUovSlNRb3A5bS96bFYyRDQ3Szl2dE1QcCtW cy8vaWtGVGx4MDFUMwpUdjJuMlIvczZpZEFGbDFXMU1vYUFnc1pnWXhMUkNhekJKL1N3OThmekIy Wjh3cG1KVkVmV0p2UW1vekhZQ09pCmZkK2JKL01sZ0IxbUFSVGNUd3hQQWxVdVFEdFhkMXBEaUln S3pPLy9ydkp5Y1laWUI4ZlFsNG1VK1AwYXFQTFYKNUZ3VDRRU0Mxa0FPYTduM0EweDZHendXdHRv akp0bGJLd1hkaW9FMjRzUWxUU2tOdUd6WjQ3SWpXMVhZVjBMVQpWelZYeVVOZ1EwRXY1K0xUOWtK ZXZNVVg4SjlWSU5hZG5BWno4SEJwV3doY1VSRkJWbGxWRjc1NUw0clNmcFFoCi9VZzBVNTRWMGp5 NWc1Y2dZcUpobEpycmZ1Qkd2MGNxTkxIM2V5QjYxdWZXMGJOSnF0bXljQ1ZNVmJ5YWxEYUQKTnhq ZjZBbTVJN3djQzIrcjZZajBoMTRVSXRsaTZpRkpuL0xqRmVlUkZNcWx6RUp1R0UrdEowb1BuVE9z ckt5ZwpiQXgrL1RkczhoRWx4QWxXUk4yY3NqZU5YUWVXdmZPVXdQZHlEOHkxYi9XcmY4T0p6ZDA9 Cj1TMzE4Ci0tLS0tRU5EIFBHUCBTSUdOQVRVUkUtLS0tLQo= --===============9182662549862717153==--