November 2023 - dev

[PATCH] port_fwd, util: Don't bind UDP ports with opposite-side bound TCP ports
by Stefano Brivio 21 Nov '23

21 Nov '23

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(a)hco.ntt.co.jp> Analysed-by: David Gibson <david(a)gibson.dropbear.id.au> Fixes: 457ff122e33c ("udp,pasta: Periodically scan for ports to automatically forward") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- port_fwd.c | 27 +++++++++++++++++++-------- port_fwd.h | 3 ++- util.c | 21 +++++++++++++++++++++ util.h | 1 + 4 files changed, 43 insertions(+), 9 deletions(-) diff --git a/port_fwd.c b/port_fwd.c index eac8d2f..91b021b 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_sum(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/util.c b/util.c index c38ab7e..d296fd9 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_sum() - Sum (logic or) of two bitmaps + * @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); -- 2.39.2

1 0

passt: new version 2023_11_19.4f1709d available
by Stefano Brivio 19 Nov '23

19 Nov '23

The new version with tag 2023_11_19.4f1709d includes the following changes: 4f1709d valgrind: Don't disable optimizations for valgrind builds f772464 valgrind: Adjust suppression for MSG_TRUNC with NULL buffer 457ff12 udp,pasta: Periodically scan for ports to automatically forward 4ccdeec tcp: Simplify away tcp_port_rebind() 1776e7a tcp: Use common helper for rebinding inbound and outbound ports 3be9e00 clang-tidy: Suppress silly misc-include-cleaner warnings https://passt.top/passt/log/?qt=range&q=2023_11_10.5ec3634..2023_11_19.4f17… Packages: - Arch Linux: https://www.archlinux.org/packages/extra/x86_64/passt/ https://archlinuxarm.org/packages/aarch64/passt https://archlinuxarm.org/packages/armv7h/passt - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/6668324/ permanent mirror: https://passt.top/builds/copr/0^20231119.g4f1709d/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Ubuntu tracker: https://launchpad.net/ubuntu/+source/passt - Void Linux: https://voidlinux.org/packages/?q=passt - Static builds: - Package for other RPM-based distributions, x86_64 only: https://passt.top/builds/latest/x86_64/passt-g4f1709d-1.x86_64.rpm - x86_64 static binaries: https://passt.top/builds/latest/x86_64/ - Debian package, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt_4f1709d-1_all.deb -- Stefano

1 0

[PATCH v3 0/2] Some fixes for valgrind suppressions
by David Gibson 19 Nov '23

19 Nov '23

With the new versions of compiler and valgrind in Fedora 39, I hit some spurious test failures. Here are some workarounds. Changes since v2: * Fix spelling error in comment Changes since v1: * Took entirely different approach, fixing the existing suppression * Extra patch allowing optimised valgrind builds David Gibson (2): valgrind: Adjust suppression for MSG_TRUNC with NULL buffer valgrind: Don't disable optimizations for valgrind builds Makefile | 2 +- tcp.c | 9 +++++++++ test/valgrind.supp | 3 +-- 3 files changed, 11 insertions(+), 3 deletions(-) -- 2.41.0

2 3

[PATCH 0/3] pasta: Implement runtime auto-forwarding for UDP
by David Gibson 19 Nov '23

19 Nov '23

In "auto" mode port forwarding, we scan for listening TCP ports at start up, and then every second thereafter. For UDP, however, we scan only at start up. That's documented, but surprising, and there's no real reason for it to be that way. Implement periodic scanning for UDP auto forwarding too. Link: https://bugs.passt.top/show_bug.cgi?id=45 *** BLURB HERE *** David Gibson (3): tcp: Use common helper for rebinding inbound and outbound ports tcp: Simplify away tcp_port_rebind() udp,pasta: Periodically scan for ports to automatically forward passt.1 | 3 +- tcp.c | 109 +++++++++++++++++++++++--------------------------------- udp.c | 74 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 66 deletions(-) -- 2.41.0

2 4

[PATCH] clang-tidy: Suppress silly misc-include-cleaner warnings
by David Gibson 19 Nov '23

19 Nov '23

clang-tidy from LLVM 17.0.3 (which is in Fedora 39) includes a new "misc-include-cleaner" warning that tries to make sure that headers *directly* provide the things that are used in the .c file. That sounds great in theory but is in practice unusable: Quite a few common things in the standard library are ultimately provided by OS-specific system headers, but for portability should be accessed via closer-to-standardised library headers. This will warn constantly about such cases: e.g. it will want you to include <linux/limits.h> instead of <limits.h> to get PATH_MAX. So, suppress this warning globally in the Makefile. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- Makefile | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 743c123..ff21459 100644 --- a/Makefile +++ b/Makefile @@ -250,6 +250,12 @@ docs: README.md # - bugprone-assignment-in-if-condition # Dubious value over the compiler's built-in warning. Would # increase verbosity. +# +# - misc-include-cleaner +# Wants to include headers which *directly* provide the things +# we use. That sounds nice, but means it will often want a OS +# specific header instead of a mostly standard one, such as +# <linux/limits.h> instead of <limits.h>. clang-tidy: $(SRCS) $(HEADERS) clang-tidy -checks=*,-modernize-*,\ @@ -275,7 +281,8 @@ clang-tidy: $(SRCS) $(HEADERS) -readability-function-cognitive-complexity,\ -altera-struct-pack-align,\ -concurrency-mt-unsafe,\ - -readability-identifier-length \ + -readability-identifier-length,\ + -misc-include-cleaner \ -config='{CheckOptions: [{key: bugprone-suspicious-string-compare.WarnOnImplicitComparison, value: "false"}]}' \ --warnings-as-errors=* $(SRCS) -- $(filter-out -pie,$(FLAGS) $(CFLAGS) $(CPPFLAGS)) -DCLANG_TIDY_58992 -- 2.41.0

2 1

[PATCH v2 0/2] Some fixes for valgrind suppressions
by David Gibson 16 Nov '23

16 Nov '23

With the new versions of compiler and valgrind in Fedora 39, I hit some spurious test failures. Here are some workarounds. Changes since v1: * Took entirely different approach, fixing the existing suppression * Extra patch allowing optimised valgrind builds David Gibson (2): valgrind: Adjust suppression for MSG_TRUNC with NULL buffer valgrind: Don't disable optimizations for valgrind builds Makefile | 2 +- tcp.c | 9 +++++++++ test/valgrind.supp | 3 +-- 3 files changed, 11 insertions(+), 3 deletions(-) -- 2.41.0

2 4

[PATCH] cppcheck,valgrind: Don't pass NULL to recv() with MSG_TRUNC
by David Gibson 16 Nov '23

16 Nov '23

Usually, of course, it's invalid to pass a NULL buffer to recv(). However, it's acceptable when using MSG_TRUNC, because that suppresses actually writing to the buffer. So, we pass NULL in tcp_sock_consume(). Unfortunately, checker tools aren't always aware of that special case: we already have a suppression for cppcheck to cover this. valgrind-3.22.0 (present in Fedora 39) has a similar problem, generating a spurious warning here. We could generate another suppression for valgrind, however, it so happens that we already have tcp_buf_discard ready to hand. If we pass this instead of NULL it makes both cppcheck and valgrind happy. We're still using the MSG_TRUNC flag, the kernel doesn't actually have to copy data, so we should still have the performance benefits of it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tcp.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tcp.c b/tcp.c index cfcd40a..f51d27a 100644 --- a/tcp.c +++ b/tcp.c @@ -2106,8 +2106,13 @@ static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq) if (SEQ_LE(ack_seq, conn->seq_ack_from_tap)) return 0; - /* cppcheck-suppress [nullPointer, unmatchedSuppression] */ - if (recv(conn->sock, NULL, ack_seq - conn->seq_ack_from_tap, + /* Since we're using MSG_TRUNC, it's allowed to pass NULL instead of a + * real buffer. However some checker tools (including at least some + * versions of cppcheck and valgrind) aren't aware of that special case. + * We so happen to have a convenient discard buffer, so we might as well + * pass it to avoid spurious complaints from those tools. + */ + if (recv(conn->sock, tcp_buf_discard, ack_seq - conn->seq_ack_from_tap, MSG_DONTWAIT | MSG_TRUNC) < 0) return -errno; -- 2.41.0

2 3

passt: new version 2023_11_10.5ec3634 available
by Stefano Brivio 12 Nov '23

12 Nov '23

The new version with tag 2023_11_10.5ec3634 includes the following changes: 5ec3634 tap, pasta: Handle short writes to /dev/tap f0776ea tap, pasta: Handle incomplete tap sends for pasta too cf3eeba tcp: Don't use TCP_WINDOW_CLAMP 930bc3b tcp: Rename and small cleanup to tcp_clamp_window() 2c1554c test/lib/perf_report: Fix up table highlight for pasta's local flows https://passt.top/passt/log/?qt=range&q=2023_11_07.56d9f6d..2023_11_10.5ec3… Packages: - Arch Linux: https://www.archlinux.org/packages/extra/x86_64/passt/ https://archlinuxarm.org/packages/aarch64/passt https://archlinuxarm.org/packages/armv7h/passt - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/6619714/ permanent mirror: https://passt.top/builds/copr/0^20231110.g5ec3634/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Ubuntu tracker: https://launchpad.net/ubuntu/+source/passt - Void Linux: https://voidlinux.org/packages/?q=passt - Static builds: - Package for other RPM-based distributions, x86_64 only: https://passt.top/builds/latest/x86_64/passt-g5ec3634-1.x86_64.rpm - x86_64 static binaries: https://passt.top/builds/latest/x86_64/ - Debian package, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt_5ec3634-1_all.deb -- Stefano

1 0

[PATCH 0/2] Avoid bugs related to TCP_WINDOW_CLAMP
by David Gibson 10 Nov '23

10 Nov '23

The way we used TCP_WINDOW_CLAMP didn't really correspond to what it actually did. More recent kernels also have a nasty bug in the handler which we could trigger, causing TCP stalls. David Gibson (2): tcp: Rename and small cleanup to tcp_clamp_window() tcp: Don't use TCP_WINDOW_CLAMP tcp.c | 70 +++++++++--------------------------------------------- tcp_conn.h | 7 +++--- 2 files changed, 14 insertions(+), 63 deletions(-) -- 2.41.0

2 6

[PATCH 0/2] Handle incomplete tap writes for pasta
by David Gibson 10 Nov '23

10 Nov '23

passt can handle various cases where we're unable to write all the data we want to to the tap side. The same logic would make sense for pasta, however right now we don't properly report incomplete writes with the tuntap device of pasta, rather than the qemu socket of passt. I come across these as possible issues with the various stall bugs we've been investigating. It doesn't look like they're relevant there after all, but they should be correct changes nonetheless. David Gibson (2): tap, pasta: Handle incomplete tap sends for pasta too tap, pasta: Handle short writes to /dev/tap tap.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) -- 2.41.0

2 3