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 62D145A026D for ; Wed, 22 Nov 2023 00:58:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1700611111; bh=+IEC0OlNufCb7OQpxg8QT8D6rMuvZojc6Y/n9hp17jA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ZQKcENzb6OKhSyPX2wlL4y4uJOTPNeqF61VfD5ra3rOTKG4ZuIXExqFBh5npnAL5X 8/GpPfmMtPZIKCcqbtyxKPF6G+To3dLILjTPekSp6fuU+f6cbunxAOsfCUxT2OVTLT kXeSPCFkI+S3MyP/Oc+xMjr/ZcALW7dyqiYZVHOE= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4SZhCq3wcBz4xNt; Wed, 22 Nov 2023 10:58:31 +1100 (AEDT) Date: Wed, 22 Nov 2023 10:58:24 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports Message-ID: References: <20231121180152.1364915-1-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="laOkl3LVjPrz/9kv" Content-Disposition: inline In-Reply-To: <20231121180152.1364915-1-sbrivio@redhat.com> Message-ID-Hash: QXSOOMWKZDAEFVO2I2I2QCHUCM47YWUB X-Message-ID-Hash: QXSOOMWKZDAEFVO2I2I2QCHUCM47YWUB 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, Akihiro Suda 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: --laOkl3LVjPrz/9kv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 21, 2023 at 07:01:52PM +0100, Stefano Brivio wrote: > When pasta periodically scans bound ports and binds them on the other > side in order to forward traffic, we bind UDP ports for corresponding > TCP port numbers, too, to support protocols and applications such as > iperf3 which use UDP port numbers matching the ones used by the TCP > data connection. >=20 > If we scan UDP ports in order to bind UDP ports, we skip detection of > the UDP ports we already bound ourselves, to avoid looping back our > own ports. Same with scanning and binding TCP ports. >=20 > But if we scan for TCP ports in order to bind UDP ports, we need to > skip bound TCP ports too, otherwise, as David pointed out: >=20 > - we find a bound TCP port on side A, and bind the corresponding TCP > and UDP ports on side B >=20 > - at the next periodic scan, we find that UDP port bound on side B, > and we bind the corresponding UDP port on side A >=20 > - at this point, we unbind that UDP port on side B: we would > otherwise loop back our own port. >=20 > To fix this, we need to avoid binding UDP ports that we already > bound, on the other side, as a consequence of finding a corresponding > bound TCP port. >=20 > Reproducing this issue is straightforward: >=20 > ./pasta -- iperf3 -s >=20 > # Wait one second, then from another terminal: > iperf3 -c ::1 -u >=20 > Reported-by: Akihiro Suda > Analysed-by: David Gibson > Fixes: 457ff122e33c ("udp,pasta: Periodically scan for ports to automatic= ally forward") > Signed-off-by: Stefano Brivio LGTM, except for one nit: [...] > +/** > + * bitmap_sum() - Sum (logic or) of two bitmaps I don't like the name bitmap_sum() since this isn't really an addable object in the usual sense. I'd prefer either bitmap_or() (thinking of the bits as bits) or bitmap_union() (thinking of the bitmaps as sets). > + * @dst: Pointer to result bitmap > + * @size: Size of bitmaps, in bytes > + * @a: First operand > + * @b: Second operand > + */ > +void bitmap_sum(uint8_t *dst, size_t size, const uint8_t *a, const uint8= _t *b) > +{ > + unsigned long *dw =3D (unsigned long *)dst; > + unsigned long *aw =3D (unsigned long *)a; > + unsigned long *bw =3D (unsigned long *)b; > + size_t i; > + > + for (i =3D 0; i < size / sizeof(long); i++, dw++, aw++, bw++) > + *dw =3D *aw | *bw; > + > + for (i =3D size / sizeof(long) * sizeof(long); i < size; i++) > + dst[i] =3D a[i] | b[i]; > +} > + > /* > * ns_enter() - Enter configured user (unless already joined) and networ= k ns > * @c: Execution context > diff --git a/util.h b/util.h > index 78a8fb2..1f1b06a 100644 > --- a/util.h > +++ b/util.h > @@ -216,6 +216,7 @@ int timespec_diff_ms(const struct timespec *a, const = struct timespec *b); > void bitmap_set(uint8_t *map, int bit); > void bitmap_clear(uint8_t *map, int bit); > int bitmap_isset(const uint8_t *map, int bit); > +void bitmap_sum(uint8_t *dst, size_t size, const uint8_t *a, const uint8= _t *b); > char *line_read(char *buf, size_t len, int fd); > void ns_enter(const struct ctx *c); > bool ns_is_init(void); --=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 --laOkl3LVjPrz/9kv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVdRAwACgkQzQJF27ox 2GfxERAAjgkwmEUI5libh/6FezrcIyJPkr7pPiNPD+Qnqo0pU+ykbasfbt3yk3/Q QsoBlscK/uzweZt2V4uIBc7OQ4TLYtHI3LrMQdGNeOh+bRHD9kctp6aAlAJnDOBL XU6ooHadtCOTRKxc830LwfcT4jz1smTfEMR08K+D/HkqyBUW7ATIGSVimS6z3+27 pt1pkltTWKzhh+aobWK+tEPRPVl98DSM5nERBvz2wzQCMTImzfAMDwVvkMib5YHI jaxiwgOIGts8hl1mJiH6v4tVGNrbJBxBcp+5o0kohTd9Yb+Ri4facV25WS+bDusW vgnYQeAJaMtvpBJQOnYTdbLonFgoQyVNUUVQWKUbPSsyU6WwQjNAmPZJ4xBNX0lv dsfsKWMen9t5Q04TK+W+CaCTYdfeKtweLYE583VjH/dVlSV3sC5MjgF47onIrb0P eNF9Ugin5QMXITc3RUeYBuWsKnIoZjpnFGRhO1HSYSrRj1L3PdCMPa/jK96HmE7q weILDFvUguLpUtga9vTguAN3QdNxnH/Oagkwi2jHgfNjSEKZ5g2c0njb2CAYOc+/ yjabrgBTFAbu9RuLK6zLITioW8GP/tDwUcS8qrR7vlHO7IvPWRBIp47MA4CEvyzM 8+AqsCMl1wOXWc1AGym+MDXj2Itb0LVs89rAtysAHT0qq84pCNI= =s+D2 -----END PGP SIGNATURE----- --laOkl3LVjPrz/9kv--