February 2023 - dev

[PATCH] passt.1: Fix typo, improve wording in examples of port forwarding specifiers
by Stefano Brivio 16 Feb '23

16 Feb '23

Based on a patch from Laine, and reports from Laine and Yalan: fix the "22-80:32-90" example, and improve wording for the other ones: instead of using "to" to denote the end of a range, use "between ... and", so that it's clear we're *not* referring to target ports. Reported-by: Laine Stump <laine(a)redhat.com> Reported-by: Yalan Zhang <yalzhang(a)redhat.com> Fixes: da20f57f19dc ("passt, qrap: Add man pages") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt.1 | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/passt.1 b/passt.1 index 528763b..ec09c22 100644 --- a/passt.1 +++ b/passt.1 @@ -338,7 +338,7 @@ Examples: .RS .TP -t 22 -Forward local port 22 to 22 on the guest +Forward local port 22 to port 22 on the guest .TP -t 22:23 Forward local port 22 to port 23 on the guest @@ -347,10 +347,11 @@ Forward local port 22 to port 23 on the guest Forward local ports 22 and 25 to ports 22 and 25 on the guest .TP -t 22-80 -Forward local ports 22 to 80 to corresponding ports on the guest +Forward local ports between 22 and 80 to corresponding ports on the guest .TP --t 22-80-32:90 -Forward local ports 22 to 80 to corresponding ports on the guest plus 10 +-t 22-80:32-90 +Forward local ports between 22 and 80 to corresponding ports on the guest, plus +10 .TP -t 192.0.2.1/22 Forward local port 22, bound to 192.0.2.1, to port 22 on the guest @@ -359,10 +360,10 @@ Forward local port 22, bound to 192.0.2.1, to port 22 on the guest Forward local port 22, bound to 192.0.2.1 and interface eth0, to port 22 .TP -t 2000-5000,~3000-3010 -Forward local ports 2000 to 5000, but not 3000 to 3010 +Forward local ports between 2000 and 5000, but not between 3000 to 3010 .TP -t 192.0.2.1/20-30,~25 -Forward local ports 20 to 24, and 26 to 30, bound to 192.0.2.1 +Forward local ports between 20 to 24, and between 26 to 30, bound to 192.0.2.1 .TP -t ~20000-20010 Forward all ports to the guest, except for the range from 20000 to 20010 @@ -425,12 +426,13 @@ Forward local port 22 to port 23 in the target namespace Forward local ports 22 and 25 to ports 22 and 25 in the target namespace .TP -t 22-80 -Forward local ports 22 to 80 to corresponding ports in the target namespace -.TP --t 22-80-32:90 -Forward local ports 22 to 80 to corresponding ports plus 10 in the target +Forward local ports between 22 and 80 to corresponding ports in the target namespace .TP +-t 22-80:32-90 +Forward local ports between 22 and 80 to corresponding ports, plus 10, in the +target namespace +.TP -t 192.0.2.1/22 Forward local port 22, bound to 192.0.2.1, to port 22 in the target namespace .TP @@ -438,13 +440,14 @@ Forward local port 22, bound to 192.0.2.1, to port 22 in the target namespace Forward local port 22, bound to 192.0.2.1 and interface eth0, to port 22 .TP -t 2000-5000,~3000-3010 -Forward local ports 2000 to 5000, but not 3000 to 3010 +Forward local ports between 2000 and 5000, but not between 3000 to 3010 .TP -t 192.0.2.1/20-30,~25 -Forward local ports 20 to 24, and 26 to 30, bound to 192.0.2.1 +Forward local ports between 20 to 24, and between 26 to 30, bound to 192.0.2.1 .TP -t ~20000-20010 -Forward all ports to the namespace, except for the range from 20000 to 20010 +Forward all ports to the target namespace, except for the range from 20000 to +20010 .RE IPv6 bound ports are also forwarded for IPv4. -- 2.35.1

2 2

[PATCH] log: Don't duplicate messages on stderr before daemonising
by Stefano Brivio 16 Feb '23

16 Feb '23

Now that logging functions force printing messages to stderr before passt forks to background, we'll have duplicate messages when running from an interactive terminal, or if --stderr is passed, because at some point we set LOG_PERROR in our __openlog() wrapper. We could defer setting LOG_PERROR, but that would change option semantics in other, unexpected ways. We could force calling passt_vsyslog() as long as the mask is set to LOG_EMERG, but that complicates the logic in logging functions even further. Go the easy way for now: don't force printing to stderr with LOG_EMERG if LOG_PERROR is already set. We should seriously consider a rework of those logging functions at this point. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- This applies on top of Laine's series. log.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/log.c b/log.c index 785bc36..f23e8ef 100644 --- a/log.c +++ b/log.c @@ -44,6 +44,8 @@ static char log_header[BUFSIZ]; /* File header, written back on cuts */ static time_t log_start; /* Start timestamp */ int log_trace; /* --trace mode enabled */ +#define BEFORE_DAEMON (setlogmask(0) == LOG_MASK(LOG_EMERG)) + #define logfn(name, level, doexit) \ void name(const char *format, ...) { \ struct timespec tp; \ @@ -56,8 +58,7 @@ void name(const char *format, ...) { \ tp.tv_nsec / (100L * 1000)); \ } \ \ - if ((LOG_MASK(LOG_PRI(level)) & log_mask) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) { \ + if ((LOG_MASK(LOG_PRI(level)) & log_mask) || BEFORE_DAEMON) { \ va_start(args, format); \ if (log_file != -1) \ logfile_write(level, format, args); \ @@ -67,7 +68,7 @@ void name(const char *format, ...) { \ } \ \ if ((setlogmask(0) & LOG_MASK(LOG_DEBUG) && log_file == -1) || \ - setlogmask(0) == LOG_MASK(LOG_EMERG)) { \ + (BEFORE_DAEMON && !(log_opt & LOG_PERROR))) { \ va_start(args, format); \ (void)vfprintf(stderr, format, args); \ va_end(args); \ -- 2.35.1

2 1

[PATCH] conf, tcp, udp: Exit if we fail to bind sockets for all given ports
by Stefano Brivio 16 Feb '23

16 Feb '23

passt supports ranges of forwarded ports as well as 'all' for TCP and UDP, so it might be convenient to proceed if we fail to bind only some of the desired ports. But if we fail to bind even a single port for a given specification, we're clearly, unexpectedly, conflicting with another network service. In that case, report failure and exit. Reported-by: Yalan Zhang <yalzhang(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 47 ++++++++++++++++++++++++++++++++++------------- tcp.c | 25 ++++++++++++++++++------- tcp.h | 4 ++-- udp.c | 16 +++++++++++++--- udp.h | 4 ++-- 5 files changed, 69 insertions(+), 27 deletions(-) diff --git a/conf.c b/conf.c index f175405..675d961 100644 --- a/conf.c +++ b/conf.c @@ -179,9 +179,9 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, { char addr_buf[sizeof(struct in6_addr)] = { 0 }, *addr = addr_buf; char buf[BUFSIZ], *spec, *ifname = NULL, *p; + bool exclude_only = true, bound_one = false; uint8_t exclude[PORT_BITMAP_SIZE] = { 0 }; sa_family_t af = AF_UNSPEC; - bool exclude_only = true; if (!strcmp(optarg, "none")) { if (fwd->mode) @@ -215,12 +215,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, memset(fwd->map, 0xff, PORT_EPHEMERAL_MIN / 8); for (i = 0; i < PORT_EPHEMERAL_MIN; i++) { - if (optname == 't') - tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i); - else if (optname == 'u') - udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, i); + if (optname == 't') { + if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i)) + bound_one = true; + } else if (optname == 'u') { + if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL, + i)) + bound_one = true; + } } + if (!bound_one) + goto bind_fail; + return; } @@ -293,12 +300,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, bitmap_set(fwd->map, i); - if (optname == 't') - tcp_sock_init(c, af, addr, ifname, i); - else if (optname == 'u') - udp_sock_init(c, 0, af, addr, ifname, i); + if (optname == 't') { + if (!tcp_sock_init(c, af, addr, ifname, i)) + bound_one = true; + } else if (optname == 'u') { + if (!udp_sock_init(c, 0, af, addr, ifname, i)) + bound_one = true; + } } + if (!bound_one) + goto bind_fail; + return; } @@ -339,13 +352,19 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg, fwd->delta[i] = mapped_range.first - orig_range.first; - if (optname == 't') - tcp_sock_init(c, af, addr, ifname, i); - else if (optname == 'u') - udp_sock_init(c, 0, af, addr, ifname, i); + if (optname == 't') { + if (!tcp_sock_init(c, af, addr, ifname, i)) + bound_one = true; + } else if (optname == 'u') { + if (udp_sock_init(c, 0, af, addr, ifname, i)) + bound_one = true; + } } } while ((p = next_chunk(p, ','))); + if (!bound_one) + goto bind_fail; + return; bad: die("Invalid port specifier %s", optarg); @@ -353,6 +372,8 @@ overlap: die("Overlapping port specifier %s", optarg); mode_conflict: die("Port forwarding mode '%s' conflicts with previous mode", optarg); +bind_fail: + die("Failed to bind any port for '-%c %s', exiting", optname, optarg); } /** diff --git a/tcp.c b/tcp.c index f028e01..32ce1e9 100644 --- a/tcp.c +++ b/tcp.c @@ -2916,20 +2916,31 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port, * @addr: Pointer to address for binding, NULL if not configured * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order + * + * Return: 0 on (partial) success, -1 on (complete) failure */ -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, - const char *ifname, in_port_t port) +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, + const char *ifname, in_port_t port) { + int ret = 0; + if (af == AF_UNSPEC && c->ifi4 && c->ifi6) /* Attempt to get a dual stack socket */ if (tcp_sock_init_af(c, AF_UNSPEC, port, addr, ifname) >= 0) - return; + return 0; /* Otherwise create a socket per IP version */ - if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) - tcp_sock_init_af(c, AF_INET, port, addr, ifname); - if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) - tcp_sock_init_af(c, AF_INET6, port, addr, ifname); + if ((af == AF_INET || af == AF_UNSPEC) && c->ifi4) { + if (tcp_sock_init_af(c, AF_INET, port, addr, ifname) < 0) + ret = -1; + } + + if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) { + if (tcp_sock_init_af(c, AF_INET6, port, addr, ifname) < 0) + ret = -1; + } + + return ret; } /** diff --git a/tcp.h b/tcp.h index 236038f..5527c5b 100644 --- a/tcp.h +++ b/tcp.h @@ -17,8 +17,8 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); int tcp_tap_handler(struct ctx *c, int af, const void *addr, const struct pool *p, const struct timespec *now); -void tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, - const char *ifname, in_port_t port); +int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, + const char *ifname, in_port_t port); int tcp_init(struct ctx *c); void tcp_timer(struct ctx *c, const struct timespec *ts); void tcp_defer_handler(struct ctx *c); diff --git a/udp.c b/udp.c index 2e9b7e6..c913d27 100644 --- a/udp.c +++ b/udp.c @@ -955,12 +955,14 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr, * @addr: Pointer to address for binding, NULL if not configured * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order + * + * Return: 0 on (partial) success, -1 on (complete) failure */ -void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, - const void *addr, const char *ifname, in_port_t port) +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, + const void *addr, const char *ifname, in_port_t port) { union udp_epoll_ref uref = { .u32 = 0 }; - int s; + int s, ret = 0; if (ns) { uref.udp.port = (in_port_t)(port + @@ -989,6 +991,9 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, ifname, port, uref.u32); udp_splice_ns[V4][port].sock = s; } + + if (s < 0) + ret = -1; } if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) { @@ -1009,7 +1014,12 @@ void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, ifname, port, uref.u32); udp_splice_ns[V6][port].sock = s; } + + if (s < 0) + ret = -1; } + + return ret; } /** diff --git a/udp.h b/udp.h index 68082ea..0e81be8 100644 --- a/udp.h +++ b/udp.h @@ -12,8 +12,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); int udp_tap_handler(struct ctx *c, int af, const void *addr, const struct pool *p, const struct timespec *now); -void udp_sock_init(const struct ctx *c, int ns, sa_family_t af, - const void *addr, const char *ifname, in_port_t port); +int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, + const void *addr, const char *ifname, in_port_t port); int udp_init(struct ctx *c); void udp_timer(struct ctx *c, const struct timespec *ts); void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, -- 2.35.1

2 1

[PATCH] dhcp: Fix netmask calculation for option 1 from prefix length
by Stefano Brivio 16 Feb '23

16 Feb '23

Similar to the conf_print() fix from commit 4129764ecaeb ("conf: Fix mask calculation from prefix_len in conf_print()"): to calculate an IPv4 netmask from the prefix length, we need to left shift 32 all-one bits by 32 minus the prefix length -- not by the prefix length itself. Reported-by: Yalan Zhang <yalzhang(a)redhat.com> Fixes: dd09cceaee21 ("Minor improvements to IPv4 netmask handling") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- dhcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dhcp.c b/dhcp.c index 6088886..edab780 100644 --- a/dhcp.c +++ b/dhcp.c @@ -335,7 +335,7 @@ int dhcp(const struct ctx *c, const struct pool *p) m->chaddr[3], m->chaddr[4], m->chaddr[5]); m->yiaddr = c->ip4.addr; - mask.s_addr = htonl(0xffffffff << c->ip4.prefix_len); + mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len)); memcpy(opts[1].s, &mask, sizeof(mask)); memcpy(opts[3].s, &c->ip4.gw, sizeof(c->ip4.gw)); memcpy(opts[54].s, &c->ip4.gw, sizeof(c->ip4.gw)); -- 2.35.1

2 1

[PATCH] tap: Use single counter for iov elements in tap_send_frames_pasta()
by Stefano Brivio 16 Feb '23

16 Feb '23

David points out that using multiple counters to go over the iov array, namely 'i' and 'iov', makes mistakes easier. We can't just use 'iov', unless we reserve an element with zero iov_len at the end, which isn't really justified. Simply use 'i' to iterate over the array. Link: https://archives.passt.top/passt-dev/Y+mfenvLn3VJ7Dg5@yekko/ Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tap.c b/tap.c index 02da84d..e4c5b5b 100644 --- a/tap.c +++ b/tap.c @@ -316,13 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c, { size_t i; - for (i = 0; i < n; i++, iov++) { - if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) { + for (i = 0; i < n; i++) { + if (write(c->fd_tap, (char *)iov[i].iov_base, + iov[i].iov_len) < 0) { debug("tap write: %s", strerror(errno)); if (errno != EAGAIN && errno != EWOULDBLOCK) tap_handler(c, c->fd_tap, EPOLLERR, NULL); i--; - iov--; } } } -- 2.35.1

2 1

[PATCH v3 0/9] error logging fixes
by Laine Stump 15 Feb '23

15 Feb '23

There are two topics covered here: 1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails - everything after the point that the logfile is opened is sent only to the logfile. The first patch makes a simple change to the logging functions that uses the value of the system logmask to decide if it should force writing messages to stderr even when a logfile has been specified. Change from "v2": I'm using Stefano's suggestion of "abusing" logmask, rather than adding a static bool to keep track. 2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged), and replace calls to err() followed by exit() with a single call to the new function die(). Change from "v2": I changed the name of the "log and exit" function from "errexit()" to "die()" at the suggestion of Dave Gibson (Stefano concurred). Although it seems a bit more violent, it does make moot many/most of Stefano's nits about needing to split lines to eliminate > 80 characters (I did address all the rest of the things he pointed out, though) NB: Yes, this says it is v3, and the previous version I sent was v2, and there *was no v1* - this is because I didn't realize that git-publish is automatically incrementing the version number every time I run it, and I had done a test-drive sending the patches to my personal address prior to sending them to the list - *that* was v1. Laine Stump (9): log to stderr until process is daemonized, even if a logfile is set add die() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to die() conf.c | 468 ++++++++++++++++++++-------------------------------- isolation.c | 67 +++----- log.c | 24 +-- log.h | 1 + netlink.c | 3 +- passt.c | 29 ++-- pasta.c | 20 +-- tap.c | 30 ++-- 8 files changed, 244 insertions(+), 398 deletions(-) -- 2.39.1

1 9

Problem with IPv4 performance tests
by David Gibson 15 Feb '23

15 Feb '23

Hi, I've been running the tests for the first time since getting back from holiday. I seem to be getting a consistent failure on the first batch of IPv4 throughput tests, although the IPv6 tests before it worked fine: === perf/passt_tcp > passt: throughput and latency Throughput in Gbps, latency in µs, one thread at 4.8 GHz, 8 streams MTU: | 256B | 576B | 1280B | 1500B | 9000B | 65520B | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv6: guest to host | - | - | 6.5 | 7.6 | 20.7 | 28.1 | TCP RR latency over IPv6: guest to host | - | - | - | - | - | 74 | TCP CRR latency over IPv6: guest to host | - | - | - | - | - | 227 | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv4: guest to host | 0 | 0 | 0 | 0 | 0 | 0 | TCP RR latency over IPv4: guest to host | - | - | - | - | - | I haven't seen a failure like this before, has anyone else seen something like this? I'm not getting any messages from the iperf client at all - like it has no connectivity whatsoever, but the non-perf tests are all passing. Haven't done much troubleshooting yet. -- 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 2

[PATCH] Makefile: Explict int type in FALLOC_FL_COLLAPSE_RANGE probe
by Stefano Brivio 15 Feb '23

15 Feb '23

From: Florian Weimer <fweimer(a)redhat.com> Future compilers will not support implicit ints by default, causing the probe to always fail. Link: https://bugs.passt.top/show_bug.cgi?id=42 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I'm just posting Florian's patch here for ease of review -- I just applied this by the way. Thanks Florian for the report and for the patch! Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index dd222a8..080c748 100644 --- a/Makefile +++ b/Makefile @@ -101,7 +101,7 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec FLAGS += -fstack-protector-strong endif -C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nx = FALLOC_FL_COLLAPSE_RANGE; +C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nint x = FALLOC_FL_COLLAPSE_RANGE; ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) EXTRA_SYSCALLS += fallocate endif -- 2.35.1

2 1

[PATCH v2 0/5] Cleanups to tcp socket pool handling
by David Gibson 14 Feb '23

14 Feb '23

This is a bit of a diversion from what I'm notionally working on at the moment. While thinking about what we'd need to use the IP_TRANSPARENT socket option to broaden the cases where we can "splice", I noticed some inelegancies in how we handle the pool of pre-opened sockets in the TCP code, and well, here we are. This makes a number of cleanups to the handling of these pools. Most notably, tcp_splice_connect() now has simpler semantics: it now always runs in the init namespace, and is always given a pre-opened socket (which could be in the guest ns). Changes since v1: * Rebased * Improved wording of some commit messages David Gibson (5): tcp: Make a helper to refill each socket pool tcp: Split init and ns cases for tcp_sock_refill() tcp: Move socket pool declarations around tcp: Split pool lookup from creating new sockets in tcp_conn_new_sock() tcp: Improve handling of fallback if socket pool is empty on new splice tcp.c | 138 ++++++++++++++++++------------------------------- tcp.h | 2 - tcp_conn.h | 12 ++++- tcp_splice.c | 141 ++++++++++++++++++++++++++------------------------- 4 files changed, 132 insertions(+), 161 deletions(-) -- 2.39.1

2 6

[PATCH 0/2] Update locations for some downloaded images
by David Gibson 14 Feb '23

14 Feb '23

We're not running the distro tests by default at the moment, but the Makefile still downloads and prepares the images for them. I haven't actually done the downloads for some time, because I kept the images around, but I finally happened to do a full clean and needed to re-download. At this point I discovered some of the things we're trying to download have moved, so some URLs need updating. David Gibson (2): test: Update location for Debian ppc64 images test: Fedora 32-35 have moved to the archives test/Makefile | 13 ++++++++----- test/distro/debian | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) -- 2.39.1

2 3