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

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 11 Oct 2025 15:48:23 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> ---
>  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 ...

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

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

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.

> + * @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