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

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 31 Oct 2025 13:47:10 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <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 ...  
> 
> 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-builtin-ppc-rldimi-rlwimi-builtin-ppc-rlwimi

but ANDC is also remarkable (cf. ANDN in the successful CISC set,
which obviously sounds like the only right choice to me).

-- 
Stefano