Re: [PATCH v2] 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: 3395 bytes --]

On Tue, Nov 21, 2023 at 07:01:52PM +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>

LGTM, except for one nit:
[...]
> +/**
> + * bitmap_sum() - Sum (logic or) of two bitmaps

I don't like the name bitmap_sum() since this isn't really an addable
object in the usual sense.  I'd prefer either bitmap_or() (thinking of
the bits as bits) or bitmap_union() (thinking of the bitmaps as sets).

> + * @dst:	Pointer to result bitmap
> + * @size:	Size of bitmaps, in bytes
> + * @a:		First operand
> + * @b:		Second operand
> + */
> +void bitmap_sum(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..1f1b06a 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_sum(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 --]