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

22 Nov 2023


      On Wed, 22 Nov 2023 10:58:24 +1100
David Gibson  wrote:
...
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 
Analysed-by: David Gibson 
Fixes: 457ff122e33c ("udp,pasta: Periodically scan for ports to automatically forward")
Signed-off-by: Stefano Brivio 
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.
Hmm, yes, I went with "sum" in the sense of logical addition -- not
that it really "sums" the bitmaps. But then...
...
I'd prefer either bitmap_or() (thinking of
the bits as bits) or bitmap_union() (thinking of the bitmaps as sets).
I could just use "_or" like the Linux kernel does. Respinning.

-- 
Stefano