Re: [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7736 bytes --]

On Wed, Nov 22, 2023 at 07:21:34AM +0100, Stefano Brivio wrote:
> When pasta periodically scans bound ports and binds them on the other
> side in order to forward traffic, we bind UDP ports for corresponding
> TCP port numbers, too, to support protocols and applications such as
> iperf3 which use UDP port numbers matching the ones used by the TCP
> data connection.
> 
> If we scan UDP ports in order to bind UDP ports, we skip detection of
> the UDP ports we already bound ourselves, to avoid looping back our
> own ports. Same with scanning and binding TCP ports.
> 
> But if we scan for TCP ports in order to bind UDP ports, we need to
> skip bound TCP ports too, otherwise, as David pointed out:
> 
> - we find a bound TCP port on side A, and bind the corresponding TCP
>   and UDP ports on side B
> 
> - at the next periodic scan, we find that UDP port bound on side B,
>   and we bind the corresponding UDP port on side A
> 
> - at this point, we unbind that UDP port on side B: we would
>   otherwise loop back our own port.
> 
> To fix this, we need to avoid binding UDP ports that we already
> bound, on the other side, as a consequence of finding a corresponding
> bound TCP port.
> 
> Reproducing this issue is straightforward:
> 
>   ./pasta -- iperf3 -s
> 
>   # Wait one second, then from another terminal:
>   iperf3 -c ::1 -u
> 
> Reported-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
> Analysed-by: David Gibson <david@gibson.dropbear.id.au>
> Fixes: 457ff122e33c ("udp,pasta: Periodically scan for ports to automatically forward")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v3: Rename bitmap_sum() to bitmap_or()
> 
> v2: Include changes for udp.c
> 
>  port_fwd.c | 27 +++++++++++++++++++--------
>  port_fwd.h |  3 ++-
>  udp.c      |  4 ++--
>  util.c     | 21 +++++++++++++++++++++
>  util.h     |  1 +
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/port_fwd.c b/port_fwd.c
> index eac8d2f..7943a30 100644
> --- a/port_fwd.c
> +++ b/port_fwd.c
> @@ -86,22 +86,33 @@ void port_fwd_scan_tcp(struct port_fwd *fwd, const struct port_fwd *rev)
>   * port_fwd_scan_tcp() - Scan /proc to update TCP forwarding map
>   * @fwd:	Forwarding information to update
>   * @rev:	Forwarding information for the reverse direction
> - * @tcp:	Corresponding TCP forwarding information
> + * @tcp_fwd:	Corresponding TCP forwarding information
> + * @tcp_rev:	TCP forwarding information for the reverse direction
>   */
>  void port_fwd_scan_udp(struct port_fwd *fwd, const struct port_fwd *rev,
> -		       const struct port_fwd *tcp)
> +		       const struct port_fwd *tcp_fwd,
> +		       const struct port_fwd *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, rev->map);
> -	procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, rev->map);
> +	procfs_scan_listen(fwd->scan4, UDP_LISTEN, fwd->map, exclude);
> +	procfs_scan_listen(fwd->scan6, UDP_LISTEN, fwd->map, exclude);
>  
>  	/* 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.
> +	 *
> +	 * 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.
>  	 */
> -	procfs_scan_listen(tcp->scan4, TCP_LISTEN, fwd->map, rev->map);
> -	procfs_scan_listen(tcp->scan6, TCP_LISTEN, fwd->map, rev->map);
> +	procfs_scan_listen(tcp_fwd->scan4, TCP_LISTEN, fwd->map, exclude);
> +	procfs_scan_listen(tcp_fwd->scan6, TCP_LISTEN, fwd->map, exclude);
>  }
>  
>  /**
> @@ -126,7 +137,7 @@ void port_fwd_init(struct ctx *c)
>  		c->udp.fwd_in.f.scan4 = open_in_ns(c, "/proc/net/udp", flags);
>  		c->udp.fwd_in.f.scan6 = open_in_ns(c, "/proc/net/udp6", flags);
>  		port_fwd_scan_udp(&c->udp.fwd_in.f, &c->udp.fwd_out.f,
> -				  &c->tcp.fwd_in);
> +				  &c->tcp.fwd_in, &c->tcp.fwd_out);
>  	}
>  	if (c->tcp.fwd_out.mode == FWD_AUTO) {
>  		c->tcp.fwd_out.scan4 = open("/proc/net/tcp", flags);
> @@ -137,6 +148,6 @@ void port_fwd_init(struct ctx *c)
>  		c->udp.fwd_out.f.scan4 = open("/proc/net/udp", flags);
>  		c->udp.fwd_out.f.scan6 = open("/proc/net/udp6", flags);
>  		port_fwd_scan_udp(&c->udp.fwd_out.f, &c->udp.fwd_in.f,
> -				  &c->tcp.fwd_out);
> +				  &c->tcp.fwd_out, &c->tcp.fwd_in);
>  	}
>  }
> diff --git a/port_fwd.h b/port_fwd.h
> index 8a823d8..f6bf1b5 100644
> --- a/port_fwd.h
> +++ b/port_fwd.h
> @@ -37,7 +37,8 @@ struct port_fwd {
>  
>  void port_fwd_scan_tcp(struct port_fwd *fwd, const struct port_fwd *rev);
>  void port_fwd_scan_udp(struct port_fwd *fwd, const struct port_fwd *rev,
> -		       const struct port_fwd *tcp);
> +		       const struct port_fwd *tcp_fwd,
> +		       const struct port_fwd *tcp_rev);
>  void port_fwd_init(struct ctx *c);
>  
>  #endif /* PORT_FWD_H */
> diff --git a/udp.c b/udp.c
> index cc1ea9c..1f8c306 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1260,13 +1260,13 @@ void udp_timer(struct ctx *c, const struct timespec *ts)
>  	if (c->mode == MODE_PASTA) {
>  		if (c->udp.fwd_out.f.mode == FWD_AUTO) {
>  			port_fwd_scan_udp(&c->udp.fwd_out.f, &c->udp.fwd_in.f,
> -					  &c->tcp.fwd_out);
> +					  &c->tcp.fwd_out, &c->tcp.fwd_in);
>  			NS_CALL(udp_port_rebind_outbound, c);
>  		}
>  
>  		if (c->udp.fwd_in.f.mode == FWD_AUTO) {
>  			port_fwd_scan_udp(&c->udp.fwd_in.f, &c->udp.fwd_out.f,
> -					  &c->tcp.fwd_in);
> +					  &c->tcp.fwd_in, &c->tcp.fwd_out);
>  			udp_port_rebind(c, false);
>  		}
>  	}
> diff --git a/util.c b/util.c
> index c38ab7e..d465e48 100644
> --- a/util.c
> +++ b/util.c
> @@ -325,6 +325,27 @@ int bitmap_isset(const uint8_t *map, int bit)
>  	return !!(*word & BITMAP_BIT(bit));
>  }
>  
> +/**
> + * bitmap_or() - Logical disjunction (OR) of two bitmaps
> + * @dst:	Pointer to result bitmap
> + * @size:	Size of bitmaps, in bytes
> + * @a:		First operand
> + * @b:		Second operand
> + */
> +void bitmap_or(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 78a8fb2..1f02588 100644
> --- a/util.h
> +++ b/util.h
> @@ -216,6 +216,7 @@ int timespec_diff_ms(const struct timespec *a, const struct timespec *b);
>  void bitmap_set(uint8_t *map, int bit);
>  void bitmap_clear(uint8_t *map, int bit);
>  int bitmap_isset(const uint8_t *map, int bit);
> +void bitmap_or(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);

-- 
David Gibson			| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]