- dev - passt

[PATCH 00/10] siphash: cleanups and fixes
by David Gibson 29 Sep '23

29 Sep '23

While working on unifying the hashing for the flow table, I noticed some awkwardness in the siphash functions. While looking into that I noticed some bugs. So.. here we are. David Gibson (10): siphash: Make siphash functions consistently return 64-bit results siphash: Make sip round calculations an inline function rather than macro siphash: Add siphash_feed() helper siphash: Clean up hash finalisation with posthash_final() function siphash: Fix bug in state initialisation siphash: Use more hygienic state initialiser siphash: Use specific structure for internal state siphash: Make internal helpers public siphash, checksum: Move TBAA explanation to checksum.c siphash: Use incremental rather than all-at-once siphash functions Makefile | 2 +- checksum.c | 19 ++-- inany.h | 16 +++- siphash.c | 243 --------------------------------------------------- siphash.h | 119 +++++++++++++++++++++++-- tcp.c | 37 +++----- tcp_splice.c | 1 + 7 files changed, 158 insertions(+), 279 deletions(-) delete mode 100644 siphash.c -- 2.41.0

2 16

[PATCH v2 0/2] Some static checker fixes
by David Gibson 27 Sep '23

27 Sep '23

We already had a couple of places we were working around clang-tidy issue 58992, and the flow table series adds more. I got sick of ugly inlines every time we used a syscall which returns a socket address, so wrote a patch to consolidate the workarounds in one place. However, that patch added an include of <string.h> to util.h which exposed a classic C library gotcha in packet.c, so I fixed that too. Changes since v1: * Updated missed comment to match code changes in 1/2 * Fixed more places which shadowed index(3) David Gibson (2): Avoid shadowing index(3) util: Consolidate and improve workarounds for clang-tidy issue 58992 Makefile | 2 +- icmp.c | 5 ----- packet.c | 30 +++++++++++++++--------------- packet.h | 10 +++++----- tcp.c | 22 ++++++++-------------- tcp_splice.c | 2 +- util.c | 12 ++++++------ util.h | 43 ++++++++++++++++++++++++++++++++++++++++++- 8 files changed, 78 insertions(+), 48 deletions(-) -- 2.41.0

2 3

[PATCH] tcp: Always send an ACK segment once the handshake is completed
by Stefano Brivio 25 Sep '23

25 Sep '23

The reporter is running a SMTP server behind pasta, and the client waits for the server's banner before sending any data. In turn, the server waits for our ACK after sending SYN,ACK, which never comes. If we use the ACK_IF_NEEDED indication to tcp_send_flag(), given that there's no pending data, we delay sending the ACK segment at the end of the three-way handshake until we have some data to send to the server. This was actually intended, as I thought we would lower the latency for new connections, but we can't assume that the client will start sending data first (SMTP is the typical example where this doesn't happen). And, trying out this patch with SSH (where the client starts sending data first), the reporter actually noticed we have a lower latency by forcing an ACK right away. Comparing a capture before the patch: 13:07:14.007704 IP 10.1.2.1.42056 > 10.1.2.140.1234: Flags [S], seq 1797034836, win 65535, options [mss 4096,nop,wscale 7], length 0 13:07:14.007769 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [S.], seq 2297052481, ack 1797034837, win 65480, options [mss 65480,nop,wscale 7], length 0 13:07:14.008462 IP 10.1.2.1.42056 > 10.1.2.140.1234: Flags [.], seq 1:22, ack 1, win 65535, length 21 13:07:14.008496 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [.], ack 22, win 512, length 0 13:07:14.011799 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [P.], seq 1:515, ack 22, win 512, length 514 and after: 13:10:26.165364 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [S], seq 4165939595, win 65535, options [mss 4096,nop,wscale 7], length 0 13:10:26.165391 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [S.], seq 985607380, ack 4165939596, win 65480, options [mss 65480,nop,wscale 7], length 0 13:10:26.165418 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [.], ack 1, win 512, length 0 13:10:26.165683 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [.], seq 1:22, ack 1, win 512, length 21 13:10:26.165698 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [.], ack 22, win 512, length 0 13:10:26.167107 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [P.], seq 1:515, ack 22, win 512, length 514 the latency between the initial SYN segment and the first data transmission actually decreases from 792µs to 334µs. This is not statistically relevant as we have a single measurement, but it can't be that bad, either. Reported-by: cr3bs (from IRC) 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 76b7b8d..39844eb 100644 --- a/tcp.c +++ b/tcp.c @@ -2562,7 +2562,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn, * dequeue waiting for SYN,ACK from tap -- check now. */ tcp_data_from_sock(c, conn); - tcp_send_flag(c, conn, ACK_IF_NEEDED); + tcp_send_flag(c, conn, ACK); } /** -- 2.39.2

2 1

[PATCH] dhcpv6: Properly separate domain names in search list
by Stefano Brivio 23 Sep '23

23 Sep '23

If we go over the flattened list of search domains and just replace dots and zero bytes with the length of the next label to implement the encoding specified by section 3.1 of RFC 1035, if there are multiple domains in the search list, we'll also replace separators between two domain names with the length of the first label of the second domain, plus one. Those should remain as zero bytes to separate domains, though. To distinguish between label separators and domain names separators, for simplicity, introduce a dot before the first label of every domain we copy to form the list. All dots are then replaced by label lengths, and separators (zero bytes) remain as they are. As we do this, we need to make sure we don't replace the trailing dot, if present: that's already a separator. Skip copying it, and just add separators as needed. Now that we don't copy those, though, we might end up with zero-length domains: skip them, as they're meaningless anyway. And as we might skip domains, we can't use the index 'i' to check if we're at the beginning of the option -- use 'srch' instead. This is very similar to how we prepare the list for NDP option 31, except that we don't need padding (RFC 8106, 5.2) here, and we should refactor this into common functions, but it probably makes sense to rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21) first. Reported-by: Sebastian Mitterle <smitterl(a)redhat.com> Link: https://bugs.passt.top/show_bug.cgi?id=75 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- dhcpv6.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/dhcpv6.c b/dhcpv6.c index fc42a84..58171bb 100644 --- a/dhcpv6.c +++ b/dhcpv6.c @@ -376,24 +376,34 @@ search: return offset; for (i = 0; *c->dns_search[i].n; i++) { - if (!i) { + size_t name_len = strlen(c->dns_search[i].n); + + /* We already append separators, don't duplicate if present */ + if (c->dns_search[i].n[name_len - 1] == '.') + name_len--; + + /* Skip root-only search domains */ + if (!name_len) + continue; + + if (!srch) { srch = (struct opt_dns_search *)(buf + offset); offset += sizeof(struct opt_hdr); srch->hdr.t = OPT_DNS_SEARCH; srch->hdr.l = 0; p = srch->list; - *p = 0; } - p = stpcpy(p + 1, c->dns_search[i].n); - *(p++) = 0; - srch->hdr.l += strlen(c->dns_search[i].n) + 2; - offset += strlen(c->dns_search[i].n) + 2; + *p = '.'; + p = stpncpy(p + 1, c->dns_search[i].n, name_len); + p++; + srch->hdr.l += name_len + 2; + offset += name_len + 2; } if (srch) { for (i = 0; i < srch->hdr.l; i++) { - if (srch->list[i] == '.' || !srch->list[i]) { + if (srch->list[i] == '.') { srch->list[i] = strcspn(srch->list + i + 1, "."); } -- 2.39.2

2 3

[PATCH] dhcp: Actually note down the length of options received by the client
by Stefano Brivio 23 Sep '23

23 Sep '23

It turns out we never used 'clen' until commit 1f24d3efb499 ("dhcp: support BOOTP clients"), and we always ignored option 55 (Parameter Request List), while, according to RFC 2132, we MUST try to insert the requested options in the order requested by the client. The commit mentioned above made this visible because now every client is reported as sending a DHCPREQUEST as an old BOOTP client, based on the lack of option 53 (that is, zero length). Fixes: b439984641ed ("merd: ARP and DHCP handlers, connection tracking fixes") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- dhcp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dhcp.c b/dhcp.c index c1ac95e..46c258e 100644 --- a/dhcp.c +++ b/dhcp.c @@ -317,6 +317,7 @@ int dhcp(const struct ctx *c, const struct pool *p) return -1; memcpy(&opts[*type].c, val, *olen); + opts[*type].clen = *olen; opt_off += *olen + 2; } -- 2.39.2

2 1

[PATCH 0/2] Some static checker fixes
by David Gibson 20 Sep '23

20 Sep '23

We already had a couple of places we were working around clang-tidy issue 58992, and the flow table series adds more. I got sick of ugly inlines every time we used a syscall which returns a socket address, so wrote a patch to consolidate the workarounds in one place. However, that patch added an include of <string.h> to util.h which exposed a classic C library gotcha in packet.c, so I fixed that too. David Gibson (2): packet: Avoid shadowing index(3) util: Consolidate and improve workarounds for clang-tidy issue 58992 Makefile | 2 +- icmp.c | 5 ----- packet.c | 28 ++++++++++++++-------------- packet.h | 10 +++++----- tcp.c | 8 +------- util.h | 41 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 62 insertions(+), 32 deletions(-) -- 2.41.0

2 7

[PATCH] passt: introduce --fd-is-tap to allow passing TAP file descriptor
by Nikolay Edigaryev 19 Sep '23

19 Sep '23

Problem: I have a Cloud Hypervisor virtual machine that needs both (1) an internet access without fiddling with iptables/Netfilter and (2) VM <-> host access (to be able to provision this VM over SSH) without dealing with passt port forwarding it doesn't seem to be possible to map the whole IP address, yet the users expect an IP instead of IP:port combination. Requirement #1 is why I've choosen passt and it's pretty much satisfied (thank you for this great piece of software!). Requirement #2 implies some kind of bridge interface on the host with one TAP interface for the VM and the other for the passt. However, only pasta can accept TAP interface FD's in it's -F/--fd, which is OK, but it also configures unneeded namespacing, which in turn results in unneeded complexity and performance overhead due to the need of involving veth pairs to break away from the pasta namespace to the host for the requirement #2 to be satisfied. I've also considered proxying the UNIX domain socket communication to/from a TAP interface in my own Golang code, but it incurs significant performance overhead. On the other hand passt seems to already can do everything I need, it just needs some guidance on which type of FD it's dealing with. Solution: introduce --fd-is-tap command-line flag to tell passt which type of FD it's being passed to and force it to use appropriate system calls and offset calculation. This patch also clarifies the -F/--fd description for pasta to note that we're expecting a TAP device and not a UNIX domain socket. --- README.md | 4 ++++ conf.c | 14 +++++++++++++- passt.c | 2 ++ passt.h | 1 + tap.c | 8 +++++--- tap.h | 4 ++-- 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6d00313..a78288f 100644 --- a/README.md +++ b/README.md @@ -381,6 +381,10 @@ descriptor that's already opened. This approach, compared to using a _tap_ device, doesn't require any security capabilities, as we don't need to create any interface. +However, if you already have a _tap_ device opened by other means, you can +specify `--fd-is-tap` command-line option and _passt_ will treat the file +descriptor passed in `-F`/`--fd` option as a pre-opened TAP device. + _pasta_ runs out of the box with any recent (post-3.8) Linux kernel. ## Services diff --git a/conf.c b/conf.c index 0ad6e23..d622fdf 100644 --- a/conf.c +++ b/conf.c @@ -803,7 +803,12 @@ static void print_usage(const char *name, int status) UNIX_SOCK_PATH, 1); } - info( " -F, --fd FD Use FD as pre-opened connected socket"); + if (strstr(name, "pasta")) { + info( " -F, --fd FD Use FD as pre-opened TAP device"); + } else { + info( " -F, --fd FD Use FD as pre-opened and connected UNIX domain socket"); + info( " --fd-is-tap Treat FD as pre-opened TAP device instead of connected UNIX domain socket"); + } info( " -p, --pcap FILE Log tap-facing traffic to pcap file"); info( " -P, --pid FILE Write own PID to the given file"); info( " -m, --mtu MTU Assign MTU via DHCP/NDP"); @@ -1232,6 +1237,7 @@ void conf(struct ctx *c, int argc, char **argv) {"config-net", no_argument, NULL, 17 }, {"no-copy-routes", no_argument, NULL, 18 }, {"no-copy-addrs", no_argument, NULL, 19 }, + {"fd-is-tap", no_argument, NULL, 20 }, { 0 }, }; struct get_bound_ports_ns_arg ns_ports_arg = { .c = c }; @@ -1411,6 +1417,12 @@ void conf(struct ctx *c, int argc, char **argv) warn("--no-copy-addrs will be dropped soon"); c->no_copy_addrs = copy_addrs_opt = true; break; + case 20: + if (c->mode != MODE_PASST) + die("--fd-is-tap is for passt mode only"); + + c->fd_tap_is_socket = false; + break; case 'd': if (c->debug) die("Multiple --debug options given"); diff --git a/passt.c b/passt.c index 8ddd9b3..b7276ff 100644 --- a/passt.c +++ b/passt.c @@ -195,9 +195,11 @@ int main(int argc, char **argv) } c.mode = MODE_PASTA; + c.fd_tap_is_socket = false; log_name = "pasta"; } else if (strstr(name, "passt")) { c.mode = MODE_PASST; + c.fd_tap_is_socket = true; log_name = "passt"; } else { exit(EXIT_FAILURE); diff --git a/passt.h b/passt.h index 282bd1a..2079cd0 100644 --- a/passt.h +++ b/passt.h @@ -264,6 +264,7 @@ struct ctx { int epollfd; int fd_tap_listen; int fd_tap; + bool fd_tap_is_socket; unsigned char mac[ETH_ALEN]; unsigned char mac_guest[ETH_ALEN]; diff --git a/tap.c b/tap.c index 93db989..12b66ca 100644 --- a/tap.c +++ b/tap.c @@ -76,7 +76,7 @@ int tap_send(const struct ctx *c, const void *data, size_t len) { pcap(data, len); - if (c->mode == MODE_PASST) { + if (c->fd_tap_is_socket) { int flags = MSG_NOSIGNAL | MSG_DONTWAIT; uint32_t vnet_len = htonl(len); @@ -421,7 +421,7 @@ void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n) if (!n) return; - if (c->mode == MODE_PASST) + if (c->fd_tap_is_socket) m = tap_send_frames_passt(c, iov, n); else m = tap_send_frames_pasta(c, iov, n); @@ -1176,6 +1176,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events) } c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, 0); + c->fd_tap_is_socket = true; if (!getsockopt(c->fd_tap, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) info("accepted connection from PID %i", ucred.pid); @@ -1225,6 +1226,7 @@ static int tap_ns_tun(void *arg) die("Tap device opened but no network interface found"); c->fd_tap = fd; + c->fd_tap_is_socket = false; return 0; } @@ -1273,7 +1275,7 @@ void tap_sock_init(struct ctx *c) ASSERT(c->one_off); ref.fd = c->fd_tap; - if (c->mode == MODE_PASST) + if (c->fd_tap_is_socket) ref.type = EPOLL_TYPE_TAP_PASST; else ref.type = EPOLL_TYPE_TAP_PASTA; diff --git a/tap.h b/tap.h index 021fb7c..3626e49 100644 --- a/tap.h +++ b/tap.h @@ -20,7 +20,7 @@ struct tap_hdr { static inline size_t tap_hdr_len_(const struct ctx *c) { - if (c->mode == MODE_PASST) + if (c->fd_tap_is_socket) return sizeof(struct tap_hdr); else return sizeof(struct ethhdr); @@ -52,7 +52,7 @@ static inline void *tap_iov_base(const struct ctx *c, struct tap_hdr *taph) static inline size_t tap_iov_len(const struct ctx *c, struct tap_hdr *taph, size_t plen) { - if (c->mode == MODE_PASST) + if (c->fd_tap_is_socket) taph->vnet_len = htonl(plen + sizeof(taph->eh)); return plen + tap_hdr_len_(c); } -- 2.39.2 (Apple Git-144)

3 5

[PATCH] util: Fix licensing information display in --version
by Stefano Brivio 10 Sep '23

10 Sep '23

The regular expression I used when relicensing to GPLv2+ missed this. Fixes: ca2749e1bd52 ("passt: Relicense to GPL 2.0, or any later version") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util.h b/util.h index e4db33a..195023f 100644 --- a/util.h +++ b/util.h @@ -15,8 +15,8 @@ #define VERSION_BLOB \ VERSION "\n" \ "Copyright Red Hat\n" \ - "GNU Affero GPL version 3 or later " \ - "<https://www.gnu.org/licenses/agpl-3.0.html>\n" \ + "GNU General Public License, version 2 or later\n" \ + " <https://www.gnu.org/licenses/old-licenses/gpl-2.0.html>\n" \ "This is free software: you are free to change and redistribute it.\n" \ "There is NO WARRANTY, to the extent permitted by law.\n\n" -- 2.39.2

2 1

passt: new version 2023_09_08.05627dc available
by Stefano Brivio 08 Sep '23

08 Sep '23

The new version with tag 2023_09_08.05627dc includes the following changes: 05627dc util: Fix licensing information display in --version 46f915d tcp: Correct handling of FIN,ACK followed by SYN b3f2210 tcp: Consolidate paths where we initiate reset on tap interface f984003 tcp: Correctly handle RST followed rapidly by SYN 60d3915 tcp: Return consumed packet count from tcp_data_from_tap() 5fb376d tcp: Never hash match closed connections 805dd10 tcp: Remove some redundant packet_get() operations 7b56117 udp, tap: Correctly advance through packets in udp_tap_handler() 043a70b tcp, tap: Correctly advance through packets in tcp_tap_handler() https://passt.top/passt/log/?qt=range&q=2023_09_07.ee58f37..2023_09_08.0562… 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/6387922/ permanent mirror: https://passt.top/builds/copr/0^20230908.g05627dc/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Ubuntu tracker: https://packages.ubuntu.com/lunar/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-g46f915d-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_46f915d-1_all.deb -- Stefano

1 0

[PATCH 0/8] Fix a number of bugs with handling of TCP packets from tap
by David Gibson 08 Sep '23

08 Sep '23

In the course of investigating bug 68, I discovered a number of pretty serious bugs in how we handle various cases in tcp_tap_handler() and tcp_data_from_tap(). This series fixes a number of them. Note that while I'm pretty sure the bugs fixed here are real, I haven't yet positively traced how they lead to the symptoms in bug 68 - I'm still waiting on the results from some special instrumentation to track that down. Link: https://bugs.passt.top/show_bug.cgi?id=68 David Gibson (8): tcp, tap: Correctly advance through packets in tcp_tap_handler() udp, tap: Correctly advance through packets in udp_tap_handler() tcp: Remove some redundant packet_get() operations tcp: Never hash match closed connections tcp: Return consumed packet count from tcp_data_from_tap() tcp: Correctly handle RST followed rapidly by SYN tcp: Consolidate paths where we initiate reset on tap interface tcp: Correct handling of FIN,ACK followed by SYN tap.c | 29 ++++++++++-------- tcp.c | 98 +++++++++++++++++++++++++++++++---------------------------- tcp.h | 2 +- udp.c | 15 ++++----- udp.h | 2 +- 5 files changed, 78 insertions(+), 68 deletions(-) -- 2.41.0

2 9