- dev - passt

[PATCH v2 0/3] tcp: Runtime checks for availability of TCP_INFO fields
by David Gibson 25 Oct '24

25 Oct '24

We have a number of Makefile tests for the presence of various fields in tcp_info. However that only tells us if they're present in the kernel headers against which we're compiling, not whether they're available in the kernel we're running on. For anything outside the earliest forms of TCP_INFO (as described in netinet/tcp.h and shared with BSD), we should verify at runtime if the kernel is returning that information. This was supposed to be leading into improved handling of lazily calling TCP_INFO, but I hit additional complications there, so I've postponed it. Might as well get these bits merged, though. v2: * Previous version removed some #ifdefs that were still needed. Simply putting them back leads to awkward duplication between the "compile time missing" and "run time missing" cases, so take a different approach which allows us to remove all of them. * Some wording changes David Gibson (3): tcp: Remove compile-time dependency on struct tcp_info version tcp: Generalise probing for tcpi_snd_wnd field tcp: Use runtime tests for TCP_INFO fields Makefile | 15 ------- tcp.c | 114 ++++++++++++++++++++++------------------------ tcp_info.h | 120 +++++++++++++++++++++++++++++++++++++++++++++++++ tcp_internal.h | 4 +- 4 files changed, 176 insertions(+), 77 deletions(-) create mode 100644 tcp_info.h -- 2.47.0

2 4

[PATCH 0/8] Take care of clang-tidy warnings with LLVM >= 16
by Stefano Brivio 25 Oct '24

25 Oct '24

So I started hitting some clang-tidy warnings with LLVM 16, some looked bogus, so I upgraded to LLVM 19, and... I got even more. This series takes care of them in different ways. Stefano Brivio (8): Makefile: Exclude qrap.c from clang-tidy checks treewide: Comply with CERT C rule ERR33-C for snprintf() treewide: Silence cert-err33-c clang-tidy warnings for fprintf() Makefile: Disable readability-math-missing-parentheses clang-tidy check treewide: Suppress clang-tidy warning if we already use O_CLOEXEC or if we can't treewide: Address cert-err33-c clang-tidy warnings for clock and timer functions udp: Take care of cert-int09-c clang-tidy warning for enum udp_iov_idx util: Don't use errno after a successful call in __daemon() Makefile | 13 ++++++++++--- arch.c | 4 +++- conf.c | 57 +++++++++++++++++++++++++++++--------------------------- log.c | 10 +++++++--- passt.c | 8 +++++--- pasta.c | 7 ++++--- pcap.c | 13 +++++++------ tap.c | 8 +++++--- tcp.c | 12 +++++++++--- udp.c | 2 +- util.c | 41 ++++++++++++++++++++++++++++++---------- util.h | 6 ++++++ 12 files changed, 118 insertions(+), 63 deletions(-) -- 2.43.0

2 19

[PATCH] tcp_splice: fcntl(2) returns the size of the pipe, if F_SETPIPE_SZ succeeds
by Stefano Brivio 25 Oct '24

25 Oct '24

Don't report bogus failures (with --trace) just because the return value is not zero. Link: https://github.com/containers/podman/issues/24219 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp_splice.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index f112cfe..93f8bce 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -320,7 +320,7 @@ static int tcp_splice_connect_finish(const struct ctx *c, } if (fcntl(conn->pipe[sidei][0], F_SETPIPE_SZ, - c->tcp.pipe_size)) { + c->tcp.pipe_size) != (int)c->tcp.pipe_size) { flow_trace(conn, "cannot set %d->%d pipe size to %zu", sidei, !sidei, c->tcp.pipe_size); @@ -672,7 +672,7 @@ static void tcp_splice_pipe_refill(const struct ctx *c) continue; if (fcntl(splice_pipe_pool[i][0], F_SETPIPE_SZ, - c->tcp.pipe_size)) { + c->tcp.pipe_size) != (int)c->tcp.pipe_size) { trace("TCP (spliced): cannot set pool pipe size to %zu", c->tcp.pipe_size); } -- 2.43.0

2 2

[PATCH] tcp_splice: splice() all we have to the writing side, not what we just read
by Stefano Brivio 25 Oct '24

25 Oct '24

In tcp_splice_sock_handler(), we try to calculate how much we can move from the pipe to the writing socket: if we just read some bytes, we'll use that amount, but if we haven't, we just try to empty the pipe. However, if we just read something, that doesn't mean that that's all the data we have on the pipe, as it's obvious from this sequence, where: pasta: epoll event on connected spliced TCP socket 54 (events: 0x00000001) Flow 0 (TCP connection (spliced)): 98304 from read-side call Flow 0 (TCP connection (spliced)): 33615 from write-side call (passed 98304) Flow 0 (TCP connection (spliced)): -1 from read-side call Flow 0 (TCP connection (spliced)): -1 from write-side call (passed 524288) Flow 0 (TCP connection (spliced)): event at tcp_splice_sock_handler:580 Flow 0 (TCP connection (spliced)): OUT_WAIT_0 we first pile up 98304 - 33615 = 64689 pending bytes, that we read but couldn't write, as the receiver buffer is full, and we set the corresponding OUT_WAIT flag. Then: pasta: epoll event on connected spliced TCP socket 54 (events: 0x00000001) Flow 0 (TCP connection (spliced)): 32768 from read-side call Flow 0 (TCP connection (spliced)): -1 from write-side call (passed 32768) Flow 0 (TCP connection (spliced)): event at tcp_splice_sock_handler:580 we splice() 32768 more bytes from our receiving side to the pipe. At some point: pasta: epoll event on connected spliced TCP socket 49 (events: 0x00000004) Flow 0 (TCP connection (spliced)): event at tcp_splice_sock_handler:489 Flow 0 (TCP connection (spliced)): ~OUT_WAIT_0 Flow 0 (TCP connection (spliced)): 1320 from read-side call Flow 0 (TCP connection (spliced)): 1320 from write-side call (passed 1320) the receiver is signalling to us that it's ready for more data (EPOLLOUT). We reset the OUT_WAIT flag, read 1320 more bytes from our receiving socket into the pipe, and that's what we write to the receiver, forgetting about the pending 97457 bytes we had, which the receiver might never get (not the same 97547 bytes: we'll actually send 1320 of those). This condition is rather hard to reproduce, and it was observed with Podman pulling container images via HTTPS. In the traces above, the client is side 0 (the initiating peer), and the server is sending the whole data. Instead of splicing from pipe to socket the amount of data we just read, we need to splice all the pending data we piled up until that point. We could do that using 'read' and 'written' counters, but there's actually no need, as the kernel also keeps track of how much data is available on the pipe. So, to make this simple and more robust, just give the whole pipe size as length to splice(). The kernel knows what to do with it. Later in the function, we used 'to_write' for an optimisation meant to reduce wakeups which retries right away to splice() in both directions if we couldn't write to the receiver the whole amount of pending data. Calculate a 'pending' value instead, only if we reach that point. Now that we check for the actual amount of pending data in that optimisation, we need to make sure we don't compare a zero or negative 'written' value: if we met that, it means that the receiver signalled end-of-file, an error, or to try again later. In those three cases, the optimisation doesn't make any sense, so skip it. Reported-by: Ed Santiago <santiago(a)redhat.com> Reported-by: Paul Holzinger <pholzing(a)redhat.com> Analysed-by: Paul Holzinger <pholzing(a)redhat.com> Link: https://github.com/containers/podman/issues/24219 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp_splice.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/tcp_splice.c b/tcp_splice.c index 9f5cc27..f112cfe 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -503,7 +503,7 @@ swap: lowat_act_flag = RCVLOWAT_ACT(fromsidei); while (1) { - ssize_t readlen, to_write = 0, written; + ssize_t readlen, written, pending; int more = 0; retry: @@ -518,14 +518,11 @@ retry: if (errno != EAGAIN) goto close; - - to_write = c->tcp.pipe_size; } else if (!readlen) { eof = 1; - to_write = c->tcp.pipe_size; } else { never_read = 0; - to_write += readlen; + if (readlen >= (long)c->tcp.pipe_size * 90 / 100) more = SPLICE_F_MORE; @@ -535,10 +532,10 @@ retry: eintr: written = splice(conn->pipe[fromsidei][0], NULL, - conn->s[!fromsidei], NULL, to_write, + conn->s[!fromsidei], NULL, c->tcp.pipe_size, SPLICE_F_MOVE | more | SPLICE_F_NONBLOCK); flow_trace(conn, "%zi from write-side call (passed %zi)", - written, to_write); + written, c->tcp.pipe_size); /* Most common case: skip updating counters. */ if (readlen > 0 && readlen == written) { @@ -584,10 +581,9 @@ eintr: if (never_read && written == (long)(c->tcp.pipe_size)) goto retry; - if (!never_read && written < to_write) { - to_write -= written; + pending = conn->read[fromsidei] - conn->written[fromsidei]; + if (!never_read && written > 0 && written < pending) goto retry; - } if (eof) break; -- 2.43.0

2 1

[PATCH 0/2] tcp: Runtime checks for availability of TCP_INFO fields
by David Gibson 24 Oct '24

24 Oct '24

We have a number of Makefile tests for the presence of various fields in tcp_info. However that only tells us if they're present in the kernel headers against which we're compiling, not whether they're available in the kernel we're running on. For anything outside the earliest forms of TCP_INFO (as described in netinet/tcp.h and shared with BSD), we should verify at runtime if the kernel is returning that information. This was supposed to be leading into improved handling of lazily calling TCP_INFO, but I hit additional complications there, so I've postponed it. Might as well get these bits merged, though. David Gibson (2): tcp: Generalise probing for tcpi_snd_wnd field tcp: Use runtime tests for TCP_INFO fields tcp.c | 97 +++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 40 deletions(-) -- 2.47.0

2 4

[PATCH v4 0/2] tcp: unify IPv4 and IPv6 tap queues
by Jon Maloy 21 Oct '24

21 Oct '24

This should save us some memory and code. --- v2: - Setting pointers to pre-set IP and MAC headers on the fly instead of copying them. - Merged patch #2 and #3 from v1 v3: - Changes based on feedback from team v4: - Rebased to latest version of master branch Jon Maloy (2): tcp: set ip and eth headers in l2 tap queues on the fly tcp: unify l2 TCPv4 and TCPv6 queues and structures tcp.c | 6 +- tcp_buf.c | 272 ++++++++++++++++++------------------------------------ tcp_buf.h | 3 +- 3 files changed, 91 insertions(+), 190 deletions(-) -- 2.45.2

3 5

[PATCH] tcp: Use structures to construct initial TCP options
by David Gibson 21 Oct '24

21 Oct '24

As a rule, we prefer constructing packets with matching C structures, rather than building them byte by byte. However, one case we still build byte by byte is the TCP options we include in SYN packets (in fact the only time we generate TCP options on the tap interface). Rework this to use a structure and initialisers which make it a bit clearer what's going on. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tcp.c | 17 +++---------- tcp_buf.c | 2 +- tcp_internal.h | 68 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 67 insertions(+), 20 deletions(-) diff --git a/tcp.c b/tcp.c index b2155ab..0d22e07 100644 --- a/tcp.c +++ b/tcp.c @@ -1232,7 +1232,7 @@ static void tcp_update_seqack_from_tap(const struct ctx *c, * 1 otherwise */ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, - int flags, struct tcphdr *th, char *data, + int flags, struct tcphdr *th, struct tcp_syn_opts *opts, size_t *optlen) { struct tcp_info tinfo = { 0 }; @@ -1258,12 +1258,6 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, if (flags & SYN) { int mss; - /* Options: MSS, NOP and window scale (8 bytes) */ - *optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN; - - *data++ = OPT_MSS; - *data++ = OPT_MSS_LEN; - if (c->mtu == -1) { mss = tinfo.tcpi_snd_mss; } else { @@ -1279,16 +1273,11 @@ int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, else if (mss > PAGE_SIZE) mss = ROUND_DOWN(mss, PAGE_SIZE); } - *(uint16_t *)data = htons(MIN(USHRT_MAX, mss)); - - data += OPT_MSS_LEN - 2; conn->ws_to_tap = MIN(MAX_WS, tinfo.tcpi_snd_wscale); - *data++ = OPT_NOP; - *data++ = OPT_WS; - *data++ = OPT_WS_LEN; - *data++ = conn->ws_to_tap; + *opts = TCP_SYN_OPTS(mss, conn->ws_to_tap); + *optlen = sizeof(*opts); } else if (!(flags & RST)) { flags |= ACK; } diff --git a/tcp_buf.c b/tcp_buf.c index 238827b..44df0e4 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -282,7 +282,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) seq = conn->seq_to_tap; ret = tcp_prepare_flags(c, conn, flags, &payload->th, - payload->opts, &optlen); + &payload->opts, &optlen); if (ret <= 0) { if (CONN_V4(conn)) tcp4_flags_used--; diff --git a/tcp_internal.h b/tcp_internal.h index 2f74ffe..1ab8ce2 100644 --- a/tcp_internal.h +++ b/tcp_internal.h @@ -33,9 +33,7 @@ #define OPT_EOL 0 #define OPT_NOP 1 #define OPT_MSS 2 -#define OPT_MSS_LEN 4 #define OPT_WS 3 -#define OPT_WS_LEN 3 #define OPT_SACKP 4 #define OPT_SACK 5 #define OPT_TS 8 @@ -77,6 +75,65 @@ struct tcp_payload_t { } __attribute__ ((packed, aligned(__alignof__(unsigned int)))); #endif +/** struct tcp_opt_nop - TCP NOP option + * @kind: Option kind (OPT_NOP = 1) + */ +struct tcp_opt_nop { + uint8_t kind; +} __attribute__ ((packed)); +#define TCP_OPT_NOP ((struct tcp_opt_nop){ .kind = OPT_NOP, }) + +/** struct tcp_opt_mss - TCP MSS option + * @kind: Option kind (OPT_MSS == 2) + * @len: Option length (4) + * @mss: Maximum Segment Size + */ +struct tcp_opt_mss { + uint8_t kind; + uint8_t len; + uint16_t mss; +} __attribute__ ((packed)); +#define TCP_OPT_MSS(mss_) \ + ((struct tcp_opt_mss) { \ + .kind = OPT_MSS, \ + .len = sizeof(struct tcp_opt_mss), \ + .mss = htons(mss_), \ + }) + +/** struct tcp_opt_ws - TCP Window Scaling option + * @kind: Option kind (OPT_WS == 3) + * @len: Option length (3) + * @shift: Window scaling shift + */ +struct tcp_opt_ws { + uint8_t kind; + uint8_t len; + uint8_t shift; +} __attribute__ ((packed)); +#define TCP_OPT_WS(shift_) \ + ((struct tcp_opt_ws) { \ + .kind = OPT_WS, \ + .len = sizeof(struct tcp_opt_ws), \ + .shift = (shift_), \ + }) + +/** struct tcp_syn_opts - TCP options we apply to SYN packets + * @mss: Maximum Segment Size (MSS) option + * @nop: NOP opt (for alignment) + * @ws: Window Scaling (WS) option + */ +struct tcp_syn_opts { + struct tcp_opt_mss mss; + struct tcp_opt_nop nop; + struct tcp_opt_ws ws; +} __attribute__ ((packed)); +#define TCP_SYN_OPTS(mss_, ws_) \ + ((struct tcp_syn_opts){ \ + .mss = TCP_OPT_MSS(mss_), \ + .nop = TCP_OPT_NOP, \ + .ws = TCP_OPT_WS(ws_), \ + }) + /** * struct tcp_flags_t - TCP header and data to send zero-length * segments (flags) @@ -85,7 +142,7 @@ struct tcp_payload_t { */ struct tcp_flags_t { struct tcphdr th; - char opts[OPT_MSS_LEN + OPT_WS_LEN + 1]; + struct tcp_syn_opts opts; #ifdef __AVX2__ } __attribute__ ((packed, aligned(32))); #else @@ -124,7 +181,8 @@ size_t tcp_l2_buf_fill_headers(const struct tcp_tap_conn *conn, bool no_tcp_csum); int tcp_update_seqack_wnd(const struct ctx *c, struct tcp_tap_conn *conn, bool force_seq, struct tcp_info *tinfo); -int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, int flags, - struct tcphdr *th, char *data, size_t *optlen); +int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, + int flags, struct tcphdr *th, struct tcp_syn_opts *opts, + size_t *optlen); #endif /* TCP_INTERNAL_H */ -- 2.47.0

2 1

[PATCH v3 0/4] Don't expose container loopback services to the host
by David Gibson 21 Oct '24

21 Oct '24

podman issue #24045 pointed out that pasta's spliced forwarding logic can expose services within the namespace bound only to 127.0.0.1 or ::1 to the host. However, the namespace probably expects those to only be accessible to itself, so that's probably not what we want. This changes our forwarding logic so as not to do this. Note that the podman tests will currently fail with this series, I've submitted podman PR https://github.com/containers/podman/pull/24064 to fix that. Link: https://github.com/containers/podman/issues/24045 Changes since v2: * Add new field do structure comment Changes since v1: * Add --host-lo-to-ns-lo option to preserve the old behaviour * Clarify the new behaviour in the man page * Add some extra patches making some other corrections to the man page David Gibson (4): passt.1: Mark --stderr as deprecated more prominently passt.1: Clarify and update "Handling of local addresses" section test: Clarify test for spliced inbound transfers fwd: Direct inbound spliced forwards to the guest's external address conf.c | 9 ++++++ fwd.c | 31 +++++++++++++----- passt.1 | 75 +++++++++++++++++++++++++++----------------- passt.h | 2 ++ test/passt_in_ns/tcp | 8 ++--- test/passt_in_ns/udp | 4 +-- test/pasta/tcp | 16 +++++----- test/pasta/udp | 8 ++--- 8 files changed, 98 insertions(+), 55 deletions(-) -- 2.46.2

2 15

[PATCH v5 0/7] Don't expose container loopback services to the host
by David Gibson 18 Oct '24

18 Oct '24

podman issue #24045 pointed out that pasta's spliced forwarding logic can expose services within the namespace bound only to 127.0.0.1 or ::1 to the host. However, the namespace probably expects those to only be accessible to itself, so that's probably not what we want. This changes our forwarding logic so as not to do this. Note that the podman tests will currently fail with this series, I've submitted podman PR https://github.com/containers/podman/pull/24064 to fix that. Link: https://github.com/containers/podman/issues/24045 Changes since v4: * Handled a few cases where we need to wait for DAD which were missed in v4. * Fix a grammar error in comment Changes since v3: * Added several extra patches working around failures on Debian, because its dhclient-script doesn't wait for DAD to complete Changes since v2: * Add new field do structure comment Changes since v1: * Add --host-lo-to-ns-lo option to preserve the old behaviour * Clarify the new behaviour in the man page * Add some extra patches making some other corrections to the man page David Gibson (7): arp: Fix a handful of small warts test: Explicitly wait for DAD to complete on SLAAC addresses test: Wait for DAD on DHCPv6 addresses passt.1: Mark --stderr as deprecated more prominently passt.1: Clarify and update "Handling of local addresses" section test: Clarify test for spliced inbound transfers fwd: Direct inbound spliced forwards to the guest's external address arp.c | 8 ++--- conf.c | 9 ++++++ fwd.c | 31 +++++++++++++----- passt.1 | 75 ++++++++++++++++++++++++++----------------- passt.h | 2 ++ test/passt/dhcp | 2 ++ test/passt/ndp | 4 ++- test/passt_in_ns/dhcp | 2 ++ test/passt_in_ns/tcp | 8 ++--- test/passt_in_ns/udp | 4 +-- test/pasta/dhcp | 2 ++ test/pasta/ndp | 3 +- test/pasta/tcp | 16 ++++----- test/pasta/udp | 8 ++--- test/perf/passt_tcp | 2 ++ test/two_guests/basic | 6 +++- 16 files changed, 119 insertions(+), 63 deletions(-) -- 2.47.0

2 8

[PATCH v4 0/7] Don't expose container loopback services to the host
by David Gibson 18 Oct '24

18 Oct '24

podman issue #24045 pointed out that pasta's spliced forwarding logic can expose services within the namespace bound only to 127.0.0.1 or ::1 to the host. However, the namespace probably expects those to only be accessible to itself, so that's probably not what we want. This changes our forwarding logic so as not to do this. Note that the podman tests will currently fail with this series, I've submitted podman PR https://github.com/containers/podman/pull/24064 to fix that. Link: https://github.com/containers/podman/issues/24045 Changes since v3: * Added several extra patches working around failures on Debian, because its dhclient-script doesn't wait for DAD to complete Changes since v2: * Add new field do structure comment Changes since v1: * Add --host-lo-to-ns-lo option to preserve the old behaviour * Clarify the new behaviour in the man page * Add some extra patches making some other corrections to the man page David Gibson (7): arp: Fix a handful of small warts test: Explicitly wait for DAD to complete on SLAAC addresses test: Wait for DAD on DHCPv6 addresses passt.1: Mark --stderr as deprecated more prominently passt.1: Clarify and update "Handling of local addresses" section test: Clarify test for spliced inbound transfers fwd: Direct inbound spliced forwards to the guest's external address arp.c | 8 ++--- conf.c | 9 ++++++ fwd.c | 31 +++++++++++++----- passt.1 | 75 ++++++++++++++++++++++++++----------------- passt.h | 2 ++ test/passt/dhcp | 2 ++ test/passt/ndp | 4 ++- test/passt_in_ns/tcp | 8 ++--- test/passt_in_ns/udp | 4 +-- test/pasta/dhcp | 2 ++ test/pasta/ndp | 3 +- test/pasta/tcp | 16 ++++----- test/pasta/udp | 8 ++--- test/two_guests/basic | 6 +++- 14 files changed, 115 insertions(+), 63 deletions(-) -- 2.47.0

2 11