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. > > 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 > 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 uint8_t *b) > > +{ > > + unsigned long *dw = (unsigned long *)dst; > > + unsigned long *aw = (unsigned long *)a; > > + unsigned long *bw = (unsigned long *)b; > > + size_t i; > > + > > + for (i = 0; i < size / sizeof(long); i++, dw++, aw++, bw++) > > + *dw = *aw & ~*bw; > > + > > + for (i = size / sizeof(long) * sizeof(long); i < size; i++) > > + dst[i] = a[i] & ~b[i]; > > +} > > + > > /** > > * ns_enter() - Enter configured user (unless already joined) and network 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 uint8_t *b); > > +void bitmap_andc(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); > > -- > Stefano > -- 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