Re: [PATCH 7/8] fwd: Update all port maps before applying exclusions

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 11 Oct 2025 15:48:26 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> In fwd_scan_ports() we go through each of the automatic forwarding cases
> (tcp, udp, inbound and outbound) in turn, scanning and calculating the
> new forwarding map.  However, to avoid avoid circular forwarding, some of
> these maps affect each other.  This has the odd effect that the ones
> handled earlier are based on the previous scan of other maps, whereas
> the later ones are based on the latest scan.
> 
> That's not generally harmful, but it is counter-intuitive and results in a
> few odd edge cases.  Avoid this by performing all the scans first, without
> regard to other maps, then applying the exclusions afterwards.
> 
> One case has an extra wrinkle: for UDP we forwarded not just ports that
> were listening on UDP but ones listening on TCP as well, for the benefit of
> protocols like iperf3.  We therefore also excluded listening ports from
> both UDP and TCP from the other direction to avoid circular forwarding.
> 
> This doesn't really make sense, though.  To avoid circular forwarding, we
> don't care *why* the other side is listening on UDP, just that it *is*
> listening.  I believe the explicit handling of the reverse TCP map was only
> needed because the reverse map might have been one cycle out of date and
> therefore not included a port opened because of the corresponding TCP port.

Right, yes, that was the reason. I guess it makes sense to make this
less hypothetical in the commit message if you re-spin. Same in 8/8.

The rest of the series looks good to me.

> Now that we avoid that out of date map possibility, it's sufficient to
> just mask out UDP listening ports in the other direction.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  fwd.c | 48 ++++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/fwd.c b/fwd.c
> index 57941759..395b0def 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -146,10 +146,8 @@ static void procfs_scan_listen(int fd, unsigned int lstate, uint8_t *map)
>  /**
>   * fwd_scan_ports_tcp() - Scan /proc to update TCP forwarding map
>   * @fwd:	Forwarding information to update
> - * @rev:	Forwarding information for the reverse direction
>   */
> -static void fwd_scan_ports_tcp(struct fwd_ports *fwd,
> -			       const struct fwd_ports *rev)
> +static void fwd_scan_ports_tcp(struct fwd_ports *fwd)
>  {
>  	if (fwd->mode != FWD_AUTO)
>  		return;
> @@ -157,20 +155,15 @@ static void fwd_scan_ports_tcp(struct fwd_ports *fwd,
>  	memset(fwd->map, 0, PORT_BITMAP_SIZE);
>  	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);
>  }
>  
>  /**
>   * fwd_scan_ports_udp() - Scan /proc to update UDP forwarding map
>   * @fwd:	Forwarding information to update
> - * @rev:	Forwarding information for the reverse direction
>   * @tcp_fwd:	Corresponding TCP forwarding information
> - * @tcp_rev:	TCP forwarding information for the reverse direction
>   */
>  static void fwd_scan_ports_udp(struct fwd_ports *fwd,
> -			       const struct fwd_ports *rev,
> -			       const struct fwd_ports *tcp_fwd,
> -			       const struct fwd_ports *tcp_rev)
> +			       const struct fwd_ports *tcp_fwd)
>  {
>  	if (fwd->mode != FWD_AUTO)
>  		return;
> @@ -186,15 +179,6 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
>  	 */
>  	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
> -	 * 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.
> -	 */
> -	bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, rev->map);
> -	bitmap_andc(fwd->map, PORT_BITMAP_SIZE, fwd->map, tcp_rev->map);
>  }
>  
>  /**
> @@ -203,12 +187,28 @@ static void fwd_scan_ports_udp(struct fwd_ports *fwd,
>   */
>  static void fwd_scan_ports(struct ctx *c)
>  {
> -	fwd_scan_ports_tcp(&c->tcp.fwd_out, &c->tcp.fwd_in);
> -	fwd_scan_ports_tcp(&c->tcp.fwd_in, &c->tcp.fwd_out);
> -	fwd_scan_ports_udp(&c->udp.fwd_out, &c->udp.fwd_in,
> -			   &c->tcp.fwd_out, &c->tcp.fwd_in);
> -	fwd_scan_ports_udp(&c->udp.fwd_in, &c->udp.fwd_out,
> -			   &c->tcp.fwd_in, &c->tcp.fwd_out);
> +	fwd_scan_ports_tcp(&c->tcp.fwd_out);
> +	fwd_scan_ports_tcp(&c->tcp.fwd_in);
> +	fwd_scan_ports_udp(&c->udp.fwd_out, &c->tcp.fwd_out);
> +	fwd_scan_ports_udp(&c->udp.fwd_in, &c->tcp.fwd_in);
> +
> +	if (c->tcp.fwd_out.mode == FWD_AUTO) {
> +		bitmap_andc(c->tcp.fwd_out.map, PORT_BITMAP_SIZE,
> +			    c->tcp.fwd_out.map, c->tcp.fwd_in.map);
> +	}
> +	if (c->tcp.fwd_in.mode == FWD_AUTO) {
> +		bitmap_andc(c->tcp.fwd_in.map, PORT_BITMAP_SIZE,
> +			    c->tcp.fwd_in.map, c->tcp.fwd_out.map);
> +	}
> +
> +	if (c->udp.fwd_out.mode == FWD_AUTO) {
> +		bitmap_andc(c->udp.fwd_out.map, PORT_BITMAP_SIZE,
> +			    c->udp.fwd_out.map, c->udp.fwd_in.map);
> +	}
> +	if (c->udp.fwd_in.mode == FWD_AUTO) {
> +		bitmap_andc(c->udp.fwd_in.map, PORT_BITMAP_SIZE,
> +			    c->udp.fwd_in.map, c->udp.fwd_out.map);
> +	}
>  }
>  
>  /**

-- 
Stefano