March 2023 - dev

[PATCH] tcp: Don't reset ACK_TO_TAP_DUE on any ACK, reschedule timer as needed
by Stefano Brivio 21 Mar '23

21 Mar '23

This is mostly symmetric with commit cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer"): we shouldn't reset the ACK_TO_TAP_DUE flag on any inbound ACK segment, but only once we acknowledge everything we received from the guest or the container. If we don't, a client might unnecessarily hold off further data, especially during slow start, and in general we won't converge to the usable bandwidth. This is very visible especially with traffic tests on links with non-negligible latency, such as in the reported issue. There, a public iperf3 server sometimes aborts the test due do what appears to be a low iperf3's --rcv-timeout (probably less than a second). Even if this doesn't happen, the throughput will converge to a fraction of the usable bandwidth. Clear ACK_TO_TAP_DUE if we acknowledged everything, set it if we didn't, and reschedule the timer in case the flag is still set as the timer expires. While at it, decrease the ACK timer interval to 10ms. A 50ms interval is short enough for any bandwidth-delay product I had in mind (local connections, or non-local connections with limited bandwidth), but here I am, testing 1gbps transfers to a peer with 100ms RTT. Indeed, we could eventually make the timer interval dependent on the current window and estimated bandwidth-delay product, but at least for the moment being, 10ms should be long enough to avoid any measurable syscall overhead, yet usable for any real-world application. Reported-by: Lukas Mrtvy <lukas.mrtvy(a)gmail.com> Link: https://bugs.passt.top/show_bug.cgi?id=44 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tcp.c b/tcp.c index 590b08a..f156287 100644 --- a/tcp.c +++ b/tcp.c @@ -364,7 +364,7 @@ struct tcp6_l2_head { /* For MSS6 macro: keep in sync with tcp6_l2_buf_t */ # define KERNEL_REPORTS_SND_WND(c) (0 && (c)) #endif -#define ACK_INTERVAL 50 /* ms */ +#define ACK_INTERVAL 10 /* ms */ #define SYN_TIMEOUT 10 /* s */ #define ACK_TIMEOUT 2 #define FIN_TIMEOUT 60 @@ -1730,8 +1730,12 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) iov->iov_len = tcp_l2_buf_fill_headers(c, conn, p, optlen, NULL, conn->seq_to_tap); - if (th->ack) - conn_flag(c, conn, ~ACK_TO_TAP_DUE); + if (th->ack) { + if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap)) + conn_flag(c, conn, ~ACK_TO_TAP_DUE); + else + conn_flag(c, conn, ACK_TO_TAP_DUE); + } if (th->fin) conn_flag(c, conn, ACK_FROM_TAP_DUE); @@ -2820,7 +2824,7 @@ static void tcp_timer_handler(struct ctx *c, union epoll_ref ref) if (conn->flags & ACK_TO_TAP_DUE) { tcp_send_flag(c, conn, ACK_IF_NEEDED); - conn_flag(c, conn, ~ACK_TO_TAP_DUE); + tcp_timer_ctl(c, conn); } else if (conn->flags & ACK_FROM_TAP_DUE) { if (!(conn->events & ESTABLISHED)) { debug("TCP: index %li, handshake timeout", CONN_IDX(conn)); -- 2.39.2

1 0

[PATCH] tcp: When a connection flag it set, don't negate it for debug print
by Stefano Brivio 21 Mar '23

21 Mar '23

Fix a copy and paste typo I added in commit 5474bc5485d8 ("tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()") and --debug altogether. Fixes: 5474bc5485d8 ("tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tcp.c b/tcp.c index c9c76d7..590b08a 100644 --- a/tcp.c +++ b/tcp.c @@ -758,7 +758,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, tcp_flag_str[flag_index]); } } else { - int flag_index = fls(~flag); + int flag_index = fls(flag); if (conn->flags & flag) { /* Special case: setting ACK_FROM_TAP_DUE on a -- 2.39.2

1 0

[PATCH 0/2] Fix some problems with cppcheck-2.9.1 (from Fedora 37)
by David Gibson 21 Mar '23

21 Mar '23

I updated to the latest tree and found I was getting some new cppcheck false positives. I suspect these are fixed in the latest upstream cppcheck, but they're still there for the one packaged in Fedora 37, which is a lot more convenient for me to use. Here are some workarounds for those failures. David Gibson (2): Work around weird false positives with cppcheck-2.9.1 Fix false positive if cppcheck doesn't give a false positive conf.c | 2 +- netlink.c | 2 +- tap.c | 2 +- tcp.c | 4 ++-- tcp_splice.c | 2 +- udp.c | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) -- 2.39.2

2 4

[PATCH] udp: Actually bind detected namespace ports in init namespace
by Stefano Brivio 21 Mar '23

21 Mar '23

When I reworked udp_init() to move most of the port binding logic to conf_ports, I accidentally dropped this bit of automatic port detection (and binding) at start-up. On -U auto, in pasta mode, udp_sock_init_ns() binds ports in the namespace that correspond to ports bound in the init namespace, but on -u auto, nothing actually happens after port detection. Add udp_sock_init_init() to deal with this, and while at it fix the comment to udp_sock_init_ns(): the latter takes care of outbound "connections". This is currently not covered by tests, and the UDP port needs to be already bound in the namespace when pasta starts (periodic detection for UDP is a missing feature at the moment). It can be checked like this: $ unshare -rUn # echo $$ 590092 # socat -u UDP-LISTEN:5555 STDOUT $ pasta -q -u auto 590092 $ echo "test" | socat -u STDIN UDP:localhost:5555 Reported-by: Paul Holzinger <pholzing(a)redhat.com> Fixes: 3c6ae625101a ("conf, tcp, udp: Allow address specification for forwarded ports") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- udp.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/udp.c b/udp.c index b7bc4f3..45c8d40 100644 --- a/udp.c +++ b/udp.c @@ -1042,7 +1042,23 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, } /** - * udp_sock_init_ns() - Bind sockets in namespace for inbound connections + * udp_sock_init_init() - Bind sockets in init namespace for inbound connections + * @c: Execution context + */ +static void udp_sock_init_init(struct ctx *c) +{ + unsigned dst; + + for (dst = 0; dst < NUM_PORTS; dst++) { + if (!bitmap_isset(c->udp.fwd_in.f.map, dst)) + continue; + + udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, dst); + } +} + +/** + * udp_sock_init_ns() - Bind sockets in namespace for outbound connections * @arg: Execution context * * Return: 0 @@ -1110,6 +1126,7 @@ int udp_init(struct ctx *c) if (c->mode == MODE_PASTA) { udp_splice_iov_init(); + udp_sock_init_init(c); NS_CALL(udp_sock_init_ns, c); } -- 2.39.2

1 0

[PATCH] passt.1: Fix description of --mtu option
by Stefano Brivio 18 Mar '23

18 Mar '23

By default, 65520 bytes are advertised, and zero disables DHCP and NDP options. Fixes: ec2b58ea4dc4 ("conf, dhcp, ndp: Fix message about default MTU, make NDP consistent") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.1 | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/passt.1 b/passt.1 index 6136fa5..4c90634 100644 --- a/passt.1 +++ b/passt.1 @@ -135,8 +135,10 @@ background (if configured to do so). .TP .BR \-m ", " \-\-mtu " " \fImtu -Assign \fImtu\fR via DHCP (option 26) and NDP (option type 5). -By default, no MTU options will be sent. +Assign \fImtu\fR via DHCP (option 26) and NDP (option type 5). A zero value +disables assignment. +By default, the advertised MTU is 65520 bytes, that is, the maximum 802.3 MTU +minus the length of a 802.3 header, rounded to 32 bits (IPv4 words). .TP .BR \-a ", " \-\-address " " \fIaddr -- 2.39.2

2 1

[PATCH] Makefile: Enable external override for TARGET
by Stefano Brivio 18 Mar '23

18 Mar '23

A cross-architecture build might pass a target-specific CC on 'make', and not on 'make install', and this is what happens in Debian cross-qa tests. Given that we select binaries to be installed depending on the target architecture, this means we would build AVX2 binaries in any case on a x86_64 build machine. By overriding TARGET in package build rules, we can tell the Makefile about the target architecture, also for the 'install' (Makefile) target. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 6f8bd81..0dd1057 100644 --- a/Makefile +++ b/Makefile @@ -20,7 +20,7 @@ ifeq ($(RLIMIT_STACK_VAL),unlimited) RLIMIT_STACK_VAL := 1024 endif -TARGET := $(shell $(CC) -dumpmachine) +TARGET ?= $(shell $(CC) -dumpmachine) # Get 'uname -m'-like architecture description for target TARGET_ARCH := $(shell echo $(TARGET) | cut -f1 -d- | tr [A-Z] [a-z]) TARGET_ARCH := $(shell echo $(TARGET_ARCH) | sed 's/powerpc/ppc/') -- 2.39.2

2 1

passt: new version 2023_03_17.dd23496 available
by Stefano Brivio 17 Mar '23

17 Mar '23

The new version with tag 2023_03_17.dd23496 includes the following changes: dd23496 fedora: Refresh SELinux labels in scriptlets, require -selinux package 87a6550 Makefile: Enable external override for TARGET 7727804 passt.1: Fix description of --mtu option 4e6178f log: Avoid time_t/__syscall_slong_t format mismatch with long int on X32 ABI https://passt.top/passt/log/?qt=range&q=2023_03_10.70c0765..2023_03_17.dd23… Packages: - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/5653690/ permanent mirror: https://passt.top/builds/copr/0^20230317.gdd23496/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Ubuntu tracker: https://packages.ubuntu.com/lunar/passt - Static builds: - Package for other RPM-based distributions, x86_64 only: https://passt.top/builds/latest/x86_64/passt-gdd23496-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_0dd23496-1_all.deb -- Stefano

1 0

[PATCH] fedora: Refresh SELinux labels in scriptlets, require -selinux package
by Stefano Brivio 16 Mar '23

16 Mar '23

Instead of: https://fedoraproject.org/wiki/SELinux_Policy_Modules_Packaging_Draft follow this: https://fedoraproject.org/wiki/PackagingDrafts/SELinux_Independent_Policy which seems to make more sense and fixes the issue that, on a fresh install, without a reboot, the file contexts for the binaries are not actually updated. In detail: - labels are refreshed using the selinux_relabel_pre and selinux_relabel_post on install, upgrade, and uninstall - use the selinux_modules_install and selinux_modules_uninstall macros, instead of calling 'semodule' directly (no functional changes in our case) - require the -selinux package on SELinux-enabled environments and if the current system policy is "targeted" Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- contrib/fedora/passt.spec | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec index 51cad90..f6aa117 100644 --- a/contrib/fedora/passt.spec +++ b/contrib/fedora/passt.spec @@ -8,6 +8,7 @@ # Author: Stefano Brivio <sbrivio(a)redhat.com> %global git_hash {{{ git_head }}} +%global selinuxtype targeted Name: passt Version: {{{ git_version }}} @@ -19,6 +20,7 @@ URL: https://passt.top/ Source: https://passt.top/passt/snapshot/passt-%{git_hash}.tar.xz BuildRequires: gcc, make, checkpolicy, selinux-policy-devel +Requires: (%{name}-selinux = %{version}-%{release} if selinux-policy-%{selinuxtype}) %description passt implements a translation layer between a Layer-2 network interface and @@ -66,13 +68,21 @@ install -p -m 644 -D pasta.pp %{buildroot}%{_datadir}/selinux/packages/%{name}/p install -p -m 644 -D pasta.if %{buildroot}%{_datadir}/selinux/devel/include/contrib/pasta.if popd +%pre selinux +%selinux_relabel_pre -s %{selinuxtype} + %post selinux -semodule -i %{_datadir}/selinux/packages/%{name}/passt.pp 2>/dev/null || : -semodule -i %{_datadir}/selinux/packages/%{name}/pasta.pp 2>/dev/null || : +%selinux_modules_install -s %{selinuxtype} %{_datadir}/selinux/packages/%{name}/passt.pp +%selinux_modules_install -s %{selinuxtype} %{_datadir}/selinux/packages/%{name}/pasta.pp + +%postun selinux +if [ $1 -eq 0 ]; then + %selinux_modules_uninstall -s %{selinuxtype} passt + %selinux_modules_uninstall -s %{selinuxtype} pasta +fi -%preun selinux -semodule -r passt 2>/dev/null || : -semodule -r pasta 2>/dev/null || : +%posttrans selinux +%selinux_relabel_post -s %{selinuxtype} %files %license LICENSES/{AGPL-3.0-or-later.txt,BSD-3-Clause.txt} -- 2.39.2

1 0

[PATCH] log: Avoid time_t/__syscall_slong_t format mismatch with long int on X32 ABI
by Stefano Brivio 14 Mar '23

14 Mar '23

On X32 (ILP32 using AMD64 system call ABI) and glibc, struct timespec::tv_nsec is __syscall_slong_t and not a long int, see also https://sourceware.org/bugzilla/show_bug.cgi?id=16437 and timespec(3type). Fine, we could cast that down to long and be done with it. But it turns out that also time_t (not guaranteed to be equivalent to any type) is a long long int, and there we can't downcast. To keep it simple, cast both to long long int, and change formats to %lli, to avoid format warnings from gcc. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- log.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/log.c b/log.c index d2f08fa..d7411ba 100644 --- a/log.c +++ b/log.c @@ -53,9 +53,9 @@ void name(const char *format, ...) { \ \ if (setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) { \ clock_gettime(CLOCK_REALTIME, &tp); \ - fprintf(stderr, "%li.%04li: ", \ - tp.tv_sec - log_start, \ - tp.tv_nsec / (100L * 1000)); \ + fprintf(stderr, "%lli.%04lli: ", \ + (long long int)tp.tv_sec - log_start, \ + (long long int)tp.tv_nsec / (100L * 1000)); \ } \ \ if ((LOG_MASK(LOG_PRI(level)) & log_mask) || BEFORE_DAEMON) { \ @@ -225,8 +225,9 @@ static void logfile_rotate_fallocate(int fd, struct timespec *ts) return; n = snprintf(buf, BUFSIZ, - "%s - log truncated at %li.%04li", log_header, - ts->tv_sec - log_start, ts->tv_nsec / (100L * 1000)); + "%s - log truncated at %lli.%04lli", log_header, + (long long int)(ts->tv_sec - log_start), + (long long int)(ts->tv_nsec / (100L * 1000))); /* Avoid partial lines by padding the header with spaces */ nl = memchr(buf + n + 1, '\n', BUFSIZ - n - 1); @@ -256,9 +257,9 @@ static void logfile_rotate_move(int fd, struct timespec *ts) char buf[BUFSIZ], *nl; header_len = snprintf(buf, BUFSIZ, - "%s - log truncated at %li.%04li\n", log_header, - ts->tv_sec - log_start, - ts->tv_nsec / (100L * 1000)); + "%s - log truncated at %lli.%04lli\n", log_header, + (long long int)(ts->tv_sec - log_start), + (long long int)(ts->tv_nsec / (100L * 1000))); if (lseek(fd, 0, SEEK_SET) == -1) return; if (write(fd, buf, header_len) == -1) @@ -349,8 +350,9 @@ void logfile_write(int pri, const char *format, va_list ap) if (clock_gettime(CLOCK_REALTIME, &ts)) return; - n = snprintf(buf, BUFSIZ, "%li.%04li: %s", - ts.tv_sec - log_start, ts.tv_nsec / (100L * 1000), + n = snprintf(buf, BUFSIZ, "%lli.%04lli: %s", + (long long int)(ts.tv_sec - log_start), + (long long int)(ts.tv_nsec / (100L * 1000)), logfile_prefix[pri]); n += vsnprintf(buf + n, BUFSIZ - n, format, ap); -- 2.39.2

2 1

[PATCH] fedora: Install SELinux interface files to shared include directory
by Stefano Brivio 10 Mar '23

10 Mar '23

Link: https://github.com/fedora-selinux/selinux-policy/pull/1613 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- contrib/fedora/passt.spec | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contrib/fedora/passt.spec b/contrib/fedora/passt.spec index 7f67bee..62713a8 100644 --- a/contrib/fedora/passt.spec +++ b/contrib/fedora/passt.spec @@ -61,7 +61,9 @@ ln -sr %{buildroot}%{_mandir}/man1/pasta.1 %{buildroot}%{_mandir}/man1/pasta.avx pushd contrib/selinux make -f %{_datadir}/selinux/devel/Makefile install -p -m 644 -D passt.pp %{buildroot}%{_datadir}/selinux/packages/%{name}/passt.pp +install -p -m 644 -D passt.if %{buildroot}%{_datadir}/selinux/devel/include/contrib/passt.if install -p -m 644 -D pasta.pp %{buildroot}%{_datadir}/selinux/packages/%{name}/pasta.pp +install -p -m 644 -D pasta.if %{buildroot}%{_datadir}/selinux/devel/include/contrib/pasta.if popd %post selinux @@ -93,7 +95,9 @@ semodule -r pasta 2>/dev/null || : %files selinux %dir %{_datadir}/selinux/packages/%{name} %{_datadir}/selinux/packages/%{name}/passt.pp +%{_datadir}/selinux/devel/include/passt.if %{_datadir}/selinux/packages/%{name}/pasta.pp +%{_datadir}/selinux/devel/include/pasta.if %changelog {{{ passt_git_changelog }}} -- 2.39.2

1 0