- dev - passt

[PATCH 0/2] tcp: Correct handling of first ACK (or SYN-ACK) packet
by David Gibson 27 Mar '23

27 Mar '23

We have a subtle problem in the handling of the very first ack-flagged packet (either the SYN-ACK or ACK from the three way handshake). Stefano has posted a couple of versions of a patch addressing this, however I think this is a better approach. From the TCP logical point of view, that first ACK does advance the sequence number, and if we treat it as doing so, then the logic we already had in tcp_update_seqack_from_tap() is correct. David Gibson (2): tcp: Clarify allowed state for tcp_data_from_tap() tcp: Don't special case the handling of the ack of a syn tcp.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.39.2

2 3

[PATCH] tcp: Clear and set ACK_FROM_TAP_DUE also on unchanged ACK sequence from peer
by Stefano Brivio 24 Mar '23

24 Mar '23

Since commit cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer"), we don't clear ACK_FROM_TAP_DUE whenever we process an ACK segment, but, more correctly, only if we're really not waiting for a further ACK segment, that is, only if the acknowledged sequence matches what we sent. In the new function implementing this, tcp_update_seqack_from_tap(), we also reset the retransmission counter and store the updated ACK sequence. Both should be done iff forward progress is acknowledged, implied by the fact that the new ACK sequence is greater than the one we previously stored. At that point, it looked natural to also include the statements that clear and set the ACK_FROM_TAP_DUE flag inside the same conditional block: if we're not making forward progress, the need for an ACK, or lack thereof, should remain unchanged. There's a case where this isn't true, though: if a client initiates a connection, and the server doesn't send data, with the client also not sending any further data except for what's possibly sent along with the first ACK segment following SYN, ACK from the server, we'll never, in the established state of the connection, call tcp_update_seqack_from_tap() with reported forward progress. In that case, we'll never reset the initial ACK_FROM_TAP_DUE (used to trigger handshake timeouts), interpret the trigger as a the need for a retransmission (which won't actually retransmit anything), and eventually time out a perfectly healthy connection once we reach the maximum retransmission count. This is relatively simple to reproduce if we switch back to 30s iperf3 test runs, but it depends on the timing of the iperf3 client: sometimes the first ACK from the client (part of the three-way handshake) will come with data (and we'll hit the problem), sometimes data will be sent later (and we call to tcp_update_seqack_from_tap() from tcp_data_from_tap() later, avoiding the issue). A reliable reproducer is a simpler: $ strace -e accept,shutdown socat TCP-LISTEN:1111 STDIO & [2] 2202832 $ pasta --config-net -- sh -c '(sleep 30; echo a) | socat STDIN TCP:88.198.0.161:1111' accept(5, {sa_family=AF_INET, sin_port=htons(57200), sin_addr=inet_addr("127.0.0.1")}, [16]) = 6 shutdown(6, SHUT_RDWR) = 0 --- SIGTTOU {si_signo=SIGTTOU, si_code=SI_KERNEL} --- --- stopped by SIGTTOU --- 2023/03/23 16:05:06 socat[3] E write(5, 0x5645dbca9000, 2): Connection reset by peer where the socat client connects, and no data exchange happens for 30s in either direction. Here, I'm using the default gateway address to reach the socat server on the host. Fix this by clearing and setting the ACK_FROM_TAP_DUE flag regardless of reported forward progress. If we clear it when it's already unset, conn_flag() will do nothing with it. If it was set, it's always fine to reschedule the timer (as long as we're waiting for a further ACK), because we just received an ACK segment, no matter its sequence. Fixes: cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer") Reported-by: David Gibson <david(a)gibson.dropbear.id.au> Analysed-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tcp.c b/tcp.c index f156287..b225bbe 100644 --- a/tcp.c +++ b/tcp.c @@ -1610,12 +1610,12 @@ out: static void tcp_update_seqack_from_tap(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t seq) { - if (SEQ_GT(seq, conn->seq_ack_from_tap)) { - if (seq == conn->seq_to_tap) - conn_flag(c, conn, ~ACK_FROM_TAP_DUE); - else - conn_flag(c, conn, ACK_FROM_TAP_DUE); + if (seq == conn->seq_to_tap) + conn_flag(c, conn, ~ACK_FROM_TAP_DUE); + else + conn_flag(c, conn, ACK_FROM_TAP_DUE); + if (SEQ_GT(seq, conn->seq_ack_from_tap)) { conn->retrans = 0; conn->seq_ack_from_tap = seq; } -- 2.39.2

2 3

[PATCH v2] tcp: Clear ACK_FROM_TAP_DUE also on unchanged ACK sequence from peer
by Stefano Brivio 24 Mar '23

24 Mar '23

Since commit cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer"), we don't clear ACK_FROM_TAP_DUE whenever we process an ACK segment, but, more correctly, only if we're really not waiting for a further ACK segment, that is, only if the acknowledged sequence matches what we sent. In the new function implementing this, tcp_update_seqack_from_tap(), we also reset the retransmission counter and store the updated ACK sequence. Both should be done iff forward progress is acknowledged, implied by the fact that the new ACK sequence is greater than the one we previously stored. At that point, it looked natural to also include the statements that clear and set the ACK_FROM_TAP_DUE flag inside the same conditional block: if we're not making forward progress, the need for an ACK, or lack thereof, should remain unchanged. There's a case where this isn't true, though: if a tap-side client initiates a connection, and the server doesn't send any data, we'll never, in the established state of the connection, call tcp_update_seqack_from_tap() with reported forward progress. In that case, we'll never reset the initial ACK_FROM_TAP_DUE (used to trigger handshake timeouts), interpret the trigger as the need for a retransmission (which won't actually retransmit anything), and eventually time out a perfectly healthy connection once we reach the maximum retransmission count. This is relatively simple to reproduce if we switch back to 30s iperf3 test runs, but it depends on the timing of the iperf3 client: sometimes the first ACK from the client (part of the three-way handshake) will come with data (and we'll hit the problem), sometimes data will be sent later (and we call to tcp_update_seqack_from_tap() from tcp_data_from_tap() later, avoiding the issue). A reliable reproducer is a simpler: $ strace -e accept,shutdown socat TCP-LISTEN:1111 STDIO & [2] 2202832 $ pasta --config-net -- sh -c '(sleep 30; echo a) | socat STDIN TCP:88.198.0.161:1111' accept(5, {sa_family=AF_INET, sin_port=htons(57200), sin_addr=inet_addr("127.0.0.1")}, [16]) = 6 shutdown(6, SHUT_RDWR) = 0 --- SIGTTOU {si_signo=SIGTTOU, si_code=SI_KERNEL} --- --- stopped by SIGTTOU --- 2023/03/23 16:05:06 socat[3] E write(5, 0x5645dbca9000, 2): Connection reset by peer where the socat client connects, and no data exchange happens for 30s in either direction. Here, I'm using the default gateway address to reach the socat server on the host. Fix this by clearing the ACK_FROM_TAP_DUE flag regardless of reported forward progress. If we clear it when it's already unset, conn_flag() will do nothing with it. Fixes: cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer") Reported-by: David Gibson <david(a)gibson.dropbear.id.au> Analysed-by: David Gibson <david(a)gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Don't reschedule the timer if no forward progress was made. Fix a couple of things in the commit message based on David's observations tcp.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tcp.c b/tcp.c index f156287..e640a4a 100644 --- a/tcp.c +++ b/tcp.c @@ -1610,10 +1610,12 @@ out: static void tcp_update_seqack_from_tap(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t seq) { + if (seq == conn->seq_to_tap) + conn_flag(c, conn, ~ACK_FROM_TAP_DUE); + if (SEQ_GT(seq, conn->seq_ack_from_tap)) { - if (seq == conn->seq_to_tap) - conn_flag(c, conn, ~ACK_FROM_TAP_DUE); - else + /* Forward progress, but more data to acknowledge: reschedule */ + if (SEQ_LT(seq, conn->seq_to_tap)) conn_flag(c, conn, ACK_FROM_TAP_DUE); conn->retrans = 0; -- 2.39.2

1 0

passt: new version 2023_03_21.1ee2f7c available
by Stefano Brivio 22 Mar '23

22 Mar '23

The new version with tag 2023_03_21.1ee2f7c includes the following changes: 1ee2f7c tcp: Don't reset ACK_TO_TAP_DUE on any ACK, reschedule timer as needed 9ffccf7 tcp: When a connection flag it set, don't negate it for debug print 89d1494 Fix false positive if cppcheck doesn't give a false positive 34ade90 Work around weird false positives with cppcheck-2.9.1 ccf6d2a udp: Actually bind detected namespace ports in init namespace 418f75a pasta: fix tcp port forwarding in auto mode https://passt.top/passt/log/?qt=range&q=2023_03_17.dd23496..2023_03_21.1ee2… Packages: - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/5694155/ permanent mirror: https://passt.top/builds/copr/0^20230321.g1ee2f7c/ - 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-g1ee2f7c-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_1ee2f7c-1_all.deb -- Stefano

1 0

Everything Open 2023 passt presentation
by David Gibson 22 Mar '23

22 Mar '23

For anyone who might be interested, here's the presentation about passt and passt which I made at Everything Open 2023, a conference that was in Melbourne, Australia last week. Recording: watch on Youtube at: https://www.youtube.com/watch?v=QMUEtEt1i3I or download in webm format at https://mirror.linux.org.au/pub/everythingopen/2023/clarendon_room_a/Tuesda… Slides: as a Google Slides document https://docs.google.com/presentation/d/1P1lOy1hZtyCgDHltj2fwOEPDa2AGfN0vXQq… or as PDF https://drive.google.com/file/d/1b30UYxQ9F5q66ZWkO-CSRnJ-cvC1aO3S/view?usp=… Stefano, feel free to mirror those onto passt.top if you'd like. -- 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

2 1

[PATCH] pasta: fix tcp port forwarding in auto mode
by Paul Holzinger 22 Mar '23

22 Mar '23

The logic in tcp_timer() was inverted. fwd_out should expose the host ports in the ns. Therfore it must read the ports on the host and then bind them in the netns. The same for fwd_in which checks ports in the ns and then exposes them on the host. Note that this only fixes tcp ports, udp does not seems to work at all right now with the auto mode. Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- tcp.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tcp.c b/tcp.c index 0214087..0d0ad13 100644 --- a/tcp.c +++ b/tcp.c @@ -89,7 +89,7 @@ * No port translation is needed for connections initiated remotely or by the * local host: source port from socket is reused while establishing connections * to the guest. - * + * * For connections initiated by the guest, it's not possible to force the same * source port as connections are established by the host kernel: that's the * only port translation needed. @@ -173,7 +173,7 @@ * new socket is created and mapped in connection tracking table, setting * MSS and window clamping from header and option of the observed SYN segment * - * + * * Aging and timeout * ----------------- * @@ -560,7 +560,7 @@ static struct tcp6_l2_flags_buf_t { #endif struct tap_hdr taph; /* 14 2 */ struct ipv6hdr ip6h; /* 32 20 */ - struct tcphdr th /* 72 */ __attribute__ ((aligned(4))); /* 60 */ + struct tcphdr th /* 72 */ __attribute__ ((aligned(4))); /* 60 */ char opts[OPT_MSS_LEN + OPT_WS_LEN + 1]; #ifdef __AVX2__ } __attribute__ ((packed, aligned(32))) @@ -3308,14 +3308,14 @@ void tcp_timer(struct ctx *c, const struct timespec *ts) struct tcp_port_detect_arg detect_arg = { c, 0 }; struct tcp_port_rebind_arg rebind_arg = { c, 0 }; - if (c->tcp.fwd_in.mode == FWD_AUTO) { + if (c->tcp.fwd_out.mode == FWD_AUTO) { detect_arg.detect_in_ns = 0; tcp_port_detect(&detect_arg); rebind_arg.bind_in_ns = 1; NS_CALL(tcp_port_rebind, &rebind_arg); } - if (c->tcp.fwd_out.mode == FWD_AUTO) { + if (c->tcp.fwd_in.mode == FWD_AUTO) { detect_arg.detect_in_ns = 1; NS_CALL(tcp_port_detect, &detect_arg); rebind_arg.bind_in_ns = 0; -- 2.39.2

2 5

[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