From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202510 header.b=j9Pvk16z; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 5B40B5A061A for ; Fri, 31 Oct 2025 03:55:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1761879318; bh=9DCLAgYG03VMfzkPptFSG20WvKTOIx+t8I7L0N9IUeM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=j9Pvk16zVNE42vHU/TbxsgkrsCtBxc7aoDmxo7reCl9kUqvfu6SztqBYZEONs9Yol NhCOb+uXkfcesISAIYQByfqKaH2P/9QjwyfTyikfgDWdjb64gz6uKIrHKmnYwij551 IKxC5M/TekCBcqv+MwwYdThawKEyOrLIKgHoEd+zIegKXbGmN76OBHAhxuA3q4M9yM 98gbf2k+W23kpMEj7WCm31KVaftDcaxcdjuz5IniZrbe65PsNSfieglE2dEJZ4tBpu +On84OYy56LYsVs301hfzkmlEaLADth5EHfzMrFil8XmD1c1czRDZ/2LSpZeQua8ba W+Qq+2s3YjTaQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4cyQZZ55XQz4xGn; Fri, 31 Oct 2025 13:55:18 +1100 (AEDT) Date: Fri, 31 Oct 2025 13:47:10 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 4/8] fwd: Move port exclusion handling from procfs_scan_listen() to callers Message-ID: References: <20251011044827.862757-1-david@gibson.dropbear.id.au> <20251011044827.862757-5-david@gibson.dropbear.id.au> <20251030212404.3eda9c82@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="XD0yslvZD9tYs6DY" Content-Disposition: inline In-Reply-To: <20251030212404.3eda9c82@elisabeth> Message-ID-Hash: VM6P3XOV4IF6ONADZ4LLL64BIGZZGWEZ X-Message-ID-Hash: VM6P3XOV4IF6ONADZ4LLL64BIGZZGWEZ 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 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: --XD0yslvZD9tYs6DY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 30, 2025 at 09:24:04PM +0100, Stefano Brivio wrote: > On Sat, 11 Oct 2025 15:48:23 +1100 > David Gibson wrote: >=20 > > To avoid forwarding loops, we need to exclude certain ports from being > > auto-forwarded. To accomplish this, procfs_scan_listen() takes a bitmap > > of exclusions. As it detects each port, it checks against that bitmap. > > This is a complicated way of accomplishing what we need. We can instead > > mask out the excluded ports in the callers using a new bitmap_andc() > > helper. > >=20 > > Signed-off-by: David Gibson > > --- > > fwd.c | 32 ++++++++++++++------------------ > > util.c | 23 +++++++++++++++++++++++ > > util.h | 1 + > > 3 files changed, 38 insertions(+), 18 deletions(-) > >=20 > > diff --git a/fwd.c b/fwd.c > > index 19309f14..c7b768d5 100644 > > --- a/fwd.c > > +++ b/fwd.c > > @@ -110,13 +110,11 @@ bool fwd_port_is_ephemeral(in_port_t port) > > * @fd: fd for relevant /proc/net file > > * @lstate: Code for listening state to scan for > > * @map: Bitmap where numbers of ports in listening state will be set > > - * @exclude: Bitmap of ports to exclude from setting (and clear) > > * > > * #syscalls:pasta lseek > > * #syscalls:pasta ppc64le:_llseek ppc64:_llseek arm:_llseek > > */ > > -static void procfs_scan_listen(int fd, unsigned int lstate, > > - uint8_t *map, const uint8_t *exclude) > > +static void procfs_scan_listen(int fd, unsigned int lstate, uint8_t *m= ap) > > { > > struct lineread lr; > > unsigned long port; > > @@ -141,10 +139,7 @@ static void procfs_scan_listen(int fd, unsigned in= t lstate, > > if (state !=3D lstate) > > continue; > > =20 > > - if (bitmap_isset(exclude, port)) > > - bitmap_clear(map, port); > > - else > > - bitmap_set(map, port); > > + bitmap_set(map, port); > > } > > } > > =20 > > @@ -157,8 +152,9 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fw= d, > > const struct fwd_ports *rev) > > { > > memset(fwd->map, 0, PORT_BITMAP_SIZE); > > - procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map, rev->map); > > - procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map, rev->map); > > + procfs_scan_listen(fwd->scan4, TCP_LISTEN, fwd->map); > > + procfs_scan_listen(fwd->scan6, TCP_LISTEN, fwd->map); > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map); >=20 > I'm not entirely sure why I implemented it this way in 9657b6ed05cc > ("conf, tcp: Periodic detection of bound ports for pasta port > forwarding"). >=20 > I guess the original idea was that only procfs_scan_listen() would > manipulate bitmaps, for whatever reason. In any case, I'm fairly > sure that this is equivalent (it obviously look like it, but I'm > having a hard time to convince myself because of the weird way I > implemented it originally). >=20 > > } > > =20 > > /** > > @@ -173,26 +169,26 @@ static void fwd_scan_ports_udp(struct fwd_ports *= fwd, > > const struct fwd_ports *tcp_fwd, > > const struct fwd_ports *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, exclude); > > - procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, exclude); > > + procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map); > > + procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map); > > =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. > > - * > > + */ > > + procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map); > > + procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map); > > + > > + /* > > * This means we need to skip numbers of TCP ports bound on the other >=20 > Nit: /* This ... Fixed. > > * 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_fwd->scan4, TCP_LISTEN, fwd->map, exclude); > > - procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map, exclude); > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map); > > + bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, tcp_rev->map); > > } > > =20 > > /** > > diff --git a/util.c b/util.c > > index c492f904..eabadf7d 100644 > > --- a/util.c > > +++ b/util.c > > @@ -322,6 +322,7 @@ void bitmap_set(uint8_t *map, unsigned bit) > > * @map: Pointer to bitmap > > * @bit: Bit number to clear > > */ > > +/* cppcheck-suppress unusedFunction */ > > void bitmap_clear(uint8_t *map, unsigned bit) > > { > > unsigned long *word =3D (unsigned long *)map + BITMAP_WORD(bit); > > @@ -351,6 +352,7 @@ bool bitmap_isset(const uint8_t *map, unsigned bit) > > * @a: First operand > > * @b: Second operand > > */ > > +/* cppcheck-suppress unusedFunction */ >=20 > Should we just drop those? git stores them for us. Eh, maybe? It felt weird to remove a function that seemed like it belonged in the bitmap "library" of functions. Plus at the time I thought I might use it again later in the series. I didn't but I still might use it relatively soon in follow up series. > > void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint= 8_t *b) > > { > > unsigned long *dw =3D (unsigned long *)dst; > > @@ -365,6 +367,27 @@ void bitmap_or(uint8_t *dst, size_t size, const ui= nt8_t *a, const uint8_t *b) > > dst[i] =3D a[i] | b[i]; > > } > > =20 > > +/** > > + * bitmap_andc() - Logical conjunction with complement (AND NOT) of bi= tmap >=20 > Nit: this function name mixes classic logic terminology (conjunction, > complement) and operator names (and, not), which makes it hard to guess, > I think. That's my POWER background showing: https://www.ibm.com/docs/en/aix/7.3.0?topic=3Dset-andc-complement-instr= uction > The Linux kernel has __bitmap_andnot(), which I find rather cacophonic, > so what about bitmap_and_not() instead? It kind of fits with how we call > these things in passt. Makes sense, done. > > + * @dst: Pointer to result bitmap > > + * @size: Size of bitmaps, in bytes > > + * @a: First operand > > + * @b: Second operand > > + */ > > +void bitmap_andc(uint8_t *dst, size_t size, const uint8_t *a, const ui= nt8_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 netw= ork ns > > * @c: Execution context > > diff --git a/util.h b/util.h > > index 22eaac56..ac8339a3 100644 > > --- a/util.h > > +++ b/util.h > > @@ -213,6 +213,7 @@ void bitmap_set(uint8_t *map, unsigned bit); > > void bitmap_clear(uint8_t *map, unsigned bit); > > bool bitmap_isset(const uint8_t *map, unsigned bit); > > void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint= 8_t *b); > > +void bitmap_andc(uint8_t *dst, size_t size, const uint8_t *a, const ui= nt8_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 > --=20 > Stefano >=20 --=20 David Gibson (he or they) | 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 --XD0yslvZD9tYs6DY Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmkEIyEACgkQzQJF27ox 2GfxHQ/+MOwIHf+N4+REKXiMY2HKSGCqIwCXW+ocuN0VHFTnMpnGnkV+12MOq+Eh 2WLp1QkzhvqhGvHwgdxagfcaRAp5cZqYkofjR21mlLVQR0j5RocziVsR6fVF1n/b bHvNT8NOXPayZwrT9TPUUEc6LVrO+DlBBlldGjGfiq7ebm0GKAAxpR8WPTHF6M/F SDxS+7UScRyKLJCIQ+Yny0hwEOo4cq5QRQMpGf2r8Dt4KCr4WrUlvj9PQ2etBw4R KuRBKreq2RuKh25NfAdiKtI0avKDMVpTxR6HyeL43ddTRf41dlteq3Wj6rU+OZdn 4CK7giKxeJ0/BQyoTacIgaDJUHBmKAxTKYITizW42Te6phUBORCW0+VNLh80R5zd JJ1EdvavjX99wc0nZ4rtDFnzdNNbLnqyvrTZtFUEn/RQk4FFX3UXg5KnJYLt81t4 d16pwJzxcLOlotfgpX+/sYYQnwnSNpaw/7bisacGkuMuUaeVhDRKK3DVNxtRhx2U 2/QAuC860U38XGNDGpd3/Vovvv2RM243CODbD6ivLhsTkA7BD8/+uSNuciHetMR7 BgEwG1DYnfLbYGIpPSGeRSyuqfhi+CvPRr+GFZ+hWXZe5fPT6+iughF+Clktmiod fsnuwDnSuTL9IYK4RYV2fVBoYDz9Kkevw1cBI+aPOQonLmSU/zE= =33Wq -----END PGP SIGNATURE----- --XD0yslvZD9tYs6DY--