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 F07865A026F for ; Wed, 22 Nov 2023 09:56:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1700643381; bh=sw25dZ4Nu83a1kDUHkg/YQ+6PqLIC/EElfiLXwzc8d0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qMnemNovJ7/yD3Lrvwc7A4ZyqAtrGuuGvGn4L5cMazP1sM5ps9HNOsN62P1wykD9C 74pz0AKfu5nB0w2MzGvWdh/mRqD40VlGQSpAQykELNVYRaWnXV6ko6S58wLVaEphx5 yQtNLvRBJdJPVlORhIvJPqzbCuG0p94ofUuqrDHA= Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4SZw8P3dCGz4xSy; Wed, 22 Nov 2023 19:56:21 +1100 (AEDT) Date: Wed, 22 Nov 2023 19:55:39 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports Message-ID: References: <20231122062134.1526750-1-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3mC5efFrxx1tX2c2" Content-Disposition: inline In-Reply-To: <20231122062134.1526750-1-sbrivio@redhat.com> Message-ID-Hash: X2DMFIHHMFHI2EPNHGM47TNXS6NHO3X7 X-Message-ID-Hash: X2DMFIHHMFHI2EPNHGM47TNXS6NHO3X7 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: --3mC5efFrxx1tX2c2 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Nov 22, 2023 at 07:21:34AM +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 Reviewed-by: David Gibson > --- > v3: Rename bitmap_sum() to bitmap_or() >=20 > v2: Include changes for udp.c >=20 > port_fwd.c | 27 +++++++++++++++++++-------- > port_fwd.h | 3 ++- > udp.c | 4 ++-- > util.c | 21 +++++++++++++++++++++ > util.h | 1 + > 5 files changed, 45 insertions(+), 11 deletions(-) >=20 > diff --git a/port_fwd.c b/port_fwd.c > index eac8d2f..7943a30 100644 > --- a/port_fwd.c > +++ b/port_fwd.c > @@ -86,22 +86,33 @@ void port_fwd_scan_tcp(struct port_fwd *fwd, const st= ruct port_fwd *rev) > * port_fwd_scan_tcp() - Scan /proc to update TCP forwarding map > * @fwd: Forwarding information to update > * @rev: Forwarding information for the reverse direction > - * @tcp: Corresponding TCP forwarding information > + * @tcp_fwd: Corresponding TCP forwarding information > + * @tcp_rev: TCP forwarding information for the reverse direction > */ > void port_fwd_scan_udp(struct port_fwd *fwd, const struct port_fwd *rev, > - const struct port_fwd *tcp) > + const struct port_fwd *tcp_fwd, > + const struct port_fwd *tcp_rev) > { > + uint8_t exclude[PORT_BITMAP_SIZE]; > + > + bitmap_or(exclude, PORT_BITMAP_SIZE, rev->map, tcp_rev->map); > + > memset(fwd->map, 0, PORT_BITMAP_SIZE); > - procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map, rev->map); > - procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, rev->map); > + procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map, exclude); > + procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, exclude); > =20 > /* Also forward UDP ports with the same numbers as bound TCP ports. > * This is useful for a handful of protocols (e.g. iperf3) where a TCP > * control port is used to set up transfers on a corresponding UDP > * port. > + * > + * This means we need to skip numbers of TCP ports bound on the other > + * side, too. Otherwise, we would detect corresponding UDP ports as > + * bound and try to forward them from the opposite side, but it's > + * already us handling them. > */ > - procfs_scan_listen(tcp->scan4, TCP_LISTEN, fwd->map, rev->map); > - procfs_scan_listen(tcp->scan6, TCP_LISTEN, fwd->map, rev->map); > + procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map, exclude); > + procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map, exclude); > } > =20 > /** > @@ -126,7 +137,7 @@ void port_fwd_init(struct ctx *c) > c->udp.fwd_in.f.scan4 =3D open_in_ns(c, "/proc/net/udp", flags); > c->udp.fwd_in.f.scan6 =3D open_in_ns(c, "/proc/net/udp6", flags); > port_fwd_scan_udp(&c->udp.fwd_in.f, &c->udp.fwd_out.f, > - &c->tcp.fwd_in); > + &c->tcp.fwd_in, &c->tcp.fwd_out); > } > if (c->tcp.fwd_out.mode =3D=3D FWD_AUTO) { > c->tcp.fwd_out.scan4 =3D open("/proc/net/tcp", flags); > @@ -137,6 +148,6 @@ void port_fwd_init(struct ctx *c) > c->udp.fwd_out.f.scan4 =3D open("/proc/net/udp", flags); > c->udp.fwd_out.f.scan6 =3D open("/proc/net/udp6", flags); > port_fwd_scan_udp(&c->udp.fwd_out.f, &c->udp.fwd_in.f, > - &c->tcp.fwd_out); > + &c->tcp.fwd_out, &c->tcp.fwd_in); > } > } > diff --git a/port_fwd.h b/port_fwd.h > index 8a823d8..f6bf1b5 100644 > --- a/port_fwd.h > +++ b/port_fwd.h > @@ -37,7 +37,8 @@ struct port_fwd { > =20 > void port_fwd_scan_tcp(struct port_fwd *fwd, const struct port_fwd *rev); > void port_fwd_scan_udp(struct port_fwd *fwd, const struct port_fwd *rev, > - const struct port_fwd *tcp); > + const struct port_fwd *tcp_fwd, > + const struct port_fwd *tcp_rev); > void port_fwd_init(struct ctx *c); > =20 > #endif /* PORT_FWD_H */ > diff --git a/udp.c b/udp.c > index cc1ea9c..1f8c306 100644 > --- a/udp.c > +++ b/udp.c > @@ -1260,13 +1260,13 @@ void udp_timer(struct ctx *c, const struct timesp= ec *ts) > if (c->mode =3D=3D MODE_PASTA) { > if (c->udp.fwd_out.f.mode =3D=3D FWD_AUTO) { > port_fwd_scan_udp(&c->udp.fwd_out.f, &c->udp.fwd_in.f, > - &c->tcp.fwd_out); > + &c->tcp.fwd_out, &c->tcp.fwd_in); > NS_CALL(udp_port_rebind_outbound, c); > } > =20 > if (c->udp.fwd_in.f.mode =3D=3D FWD_AUTO) { > port_fwd_scan_udp(&c->udp.fwd_in.f, &c->udp.fwd_out.f, > - &c->tcp.fwd_in); > + &c->tcp.fwd_in, &c->tcp.fwd_out); > udp_port_rebind(c, false); > } > } > diff --git a/util.c b/util.c > index c38ab7e..d465e48 100644 > --- a/util.c > +++ b/util.c > @@ -325,6 +325,27 @@ int bitmap_isset(const uint8_t *map, int bit) > return !!(*word & BITMAP_BIT(bit)); > } > =20 > +/** > + * bitmap_or() - Logical disjunction (OR) of two bitmaps > + * @dst: Pointer to result bitmap > + * @size: Size of bitmaps, in bytes > + * @a: First operand > + * @b: Second operand > + */ > +void bitmap_or(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..1f02588 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_or(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 --3mC5efFrxx1tX2c2 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmVdwfYACgkQzQJF27ox 2GdcfRAAoBCHZeOoblNEYkuVPFmp+gF2nSi3VcFRELTpcgO/Ns+bCokcJLXlr2bK ssD9ZkliTaNweQacnP5QZMM8iH4r+EypxmOHT4NlH6VWFfrNh9oFNKWzT1S6LeA7 mF12hVoqvHa2XfHeV5ANj6yX0aBFM6bhzISenq50WB0U97DmHJtSbkDVslBj6bce xWf5k6ZP4x02dTqPSlHYucnN64Qmua0ZmHJrlO1lckMf1IhEFLzp1g4Nl/bpesXr UpaiHre4cLQO42gwdMA0Wci6xf8halK2ytBVBC5FFA5G6lKp6R1Q0ruBZCYyGlqC CxHA5f9XuBU799HPhjCl+cmSxlNB2c1zkM2m/djVxreiU+cbl2AezkoDjlsvYHR9 S0IeZ+8fRBGzpSE5nBqDsXgyhnh5XhYvTXMywbjET7zkEKVDEzIG927ub2ng876p mEr6oQNOHDCkr0S1LHaSTE8k1i69U5C3nRd+NflKMdV/OpXW7yd9tug/I0G5cu8z C51nX1jcLUNBw66sTk+HMro4OnxV9CScuSsgqBe8NBfzndd0e1X2IlKeov2hSnw9 VMITuz8v1aSOWS5DH5CE7T12EC57FYbBjo+0uAASBbG7aqZvzr4Oj+42F6eyGU3f 8yHvy9yC7WwGM/OP3e80T/0+1dP5yC2hFKptdDU+sESh85O5M3c= =N0JV -----END PGP SIGNATURE----- --3mC5efFrxx1tX2c2--