* [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports
@ 2023-11-22 6:21 Stefano Brivio
2023-11-22 8:55 ` David Gibson
0 siblings, 1 reply; 2+ messages in thread
From: Stefano Brivio @ 2023-11-22 6:21 UTC (permalink / raw)
To: passt-dev; +Cc: David Gibson, Akihiro Suda
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>
---
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);
--
@@ -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);
--
2.39.2
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports
2023-11-22 6:21 [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports Stefano Brivio
@ 2023-11-22 8:55 ` David Gibson
0 siblings, 0 replies; 2+ messages in thread
From: David Gibson @ 2023-11-22 8:55 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev, Akihiro Suda
[-- 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 --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-11-22 8:56 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-22 6:21 [PATCH v3] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports Stefano Brivio
2023-11-22 8:55 ` David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).