Re: [PATCH 4/8] fwd: Move port exclusion handling from procfs_scan_listen() to callers

On Fri, 31 Oct 2025 13:47:10 +1100 David Gibson wrote:

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:

...

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.

Signed-off-by: David Gibson --- fwd.c | 32 ++++++++++++++------------------ util.c | 23 +++++++++++++++++++++++ util.h | 1 + 3 files changed, 38 insertions(+), 18 deletions(-)

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 *map) { struct lineread lr; unsigned long port; @@ -141,10 +139,7 @@ static void procfs_scan_listen(int fd, unsigned int lstate, if (state != lstate) continue;

- if (bitmap_isset(exclude, port)) - bitmap_clear(map, port); - else - bitmap_set(map, port); + bitmap_set(map, port); } }

@@ -157,8 +152,9 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fwd, 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);

I'm not entirely sure why I implemented it this way in 9657b6ed05cc ("conf, tcp: Periodic detection of bound ports for pasta port forwarding").

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).

...

}

/** @@ -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);

/* 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

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); }

/** 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 = (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 */

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.

Sure.

...

...

void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b) { unsigned long *dw = (unsigned long *)dst; @@ -365,6 +367,27 @@ void bitmap_or(uint8_t *dst, size_t size, const uint8_t *a, const uint8_t *b) dst[i] = a[i] | b[i]; }

+/** + * bitmap_andc() - Logical conjunction with complement (AND NOT) of bitmap

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=set-andc-complement-instruction

I obviously don't have a POWER background but I do remember a couple of instructions simply because I found their names hilariously dissonant (other than finding them in whatever disassembly I was looking at). My favourite mouthful is RLWIMI: https://www.ibm.com/docs/en/openxl-c-and-cpp-aix/17.1.3?topic=rf-rldimi-buil... but ANDC is also remarkable (cf. ANDN in the successful CISC set, which obviously sounds like the only right choice to me). -- Stefano