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

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 22 Nov 2023 10:58:24 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <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.

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