February 2023 - dev

Improved handling of changing DNS resolvers
by Noah Gold 14 Feb '23

14 Feb '23

Hi folks, libslirp and Passt have different approaches to sharing DNS resolvers with the guest system, each with their own benefits & drawbacks. On the libslirp project, we're discussing [1] how to support DNS failover. Passt already has support for this, but there is a drawback to its solution which prevents us from taking a similar approach: the resolvers are read exactly once, so if the host changes networks at runtime, the guest will not receive the updated resolvers and thus its connectivity will break. libslirp's current approach is to DNAT a single address exposed to the guest to one of the resolvers configured on the host. The problem here is that if that one resolver goes down, the guest can't resolve DNS names. We're considering changing so that instead of a single address, we expose a set of MAXNS addresses, and DNAT those 1:1 to the DNS resolvers registered with the host. Because the DNAT table lives on the host side, we can refresh the guest's resolvers whenever the host's resolvers change, but without the need to expire a DHCP lease (even with short leases, the guest will still lose connectivity for a time). Does this sound like an approach Passt would be open to adopting as well? Thanks. - Noah [1] https://gitlab.freedesktop.org/slirp/libslirp/-/issues/26

3 7

[libvirt PATCH] qemu: allow passt to self-daemonize
by Laine Stump 14 Feb '23

14 Feb '23

I initially had the passt process being started in an identical fashion to the slirp-helper - libvirt was daemonizing the new process and recording its pid in a pidfile. The problem with this is that, since it is daemonized immediately, any startup error in passt happens after the daemonization, and thus isn't seen by libvirt - libvirt believes that the process has started successfully and continues on its merry way. The result was that sometimes a guest would be started, but there would be no passt process for qemu to use for network traffic. Instead, we should be starting passt in the same manner we start dnsmasq - we just exec it as normal (along with a request that passt create the pidfile, which is just another option on the passt commandline) and wait for the child process to exit; passt then has a chance to parse its commandline and complete all the setup prior to daemonizing itself; if it encounters an error and exits with a non-0 code, libvirt will see the code and know about the failure. We can then grab the output from stderr, log that so the "user" has some idea of what went wrong, and then fail the guest startup. Signed-off-by: Laine Stump <laine(a)redhat.com> --- src/qemu/qemu_passt.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 0f09bf3db8..f640a69c00 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -141,24 +141,23 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); + g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; pid_t pid = (pid_t) -1; int exitstatus = 0; int cmdret = 0; - VIR_AUTOCLOSE errfd = -1; cmd = virCommandNew(PASST); virCommandClearCaps(cmd); - virCommandSetPidFile(cmd, pidfile); - virCommandSetErrorFD(cmd, &errfd); - virCommandDaemonize(cmd); + virCommandSetErrorBuffer(cmd, &errbuf); virCommandAddArgList(cmd, "--one-off", "--socket", passtSocketName, "--mac-addr", virMacAddrFormat(&net->mac, macaddr), + "--pid", pidfile, NULL); if (net->mtu) { @@ -264,7 +263,7 @@ qemuPasstStart(virDomainObj *vm, if (cmdret < 0 || exitstatus != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not start 'passt'. exitstatus: %d"), exitstatus); + _("Could not start 'passt': %s"), errbuf); goto error; } -- 2.39.1

6 14

[PATCH v2 0/9] error logging fixes
by Laine Stump 14 Feb '23

14 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. The first patch makes a simple change to the logging functions that checks a global bool that is set true after all initialization is completed. 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). Laine Stump (9): log to stderr until process is daemonized, even if a logfile is set add errexit() 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 errexit() conf.c | 471 ++++++++++++++++++++-------------------------------- isolation.c | 78 ++++----- log.c | 33 ++-- log.h | 2 + netlink.c | 3 +- passt.c | 18 +- pasta.c | 21 +-- tap.c | 30 ++-- 8 files changed, 258 insertions(+), 398 deletions(-) -- 2.39.1

4 20

[PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
by Stefano Brivio 14 Feb '23

14 Feb '23

...instead of repeatedly sending out the first one in iov. Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I just applied this, to unblock a series by David which was pending for way too long. The commit reference in Fixes: refers to a commit from said series which I'm pushing out together with this patch. Posting anyway for reviews. tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index af9bc15..716d887 100644 --- a/tap.c +++ b/tap.c @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c, { size_t i; - for (i = 0; i < n; i++) { + for (i = 0; i < n; i++, iov++) { if (write(c->fd_tap, (char *)iov->iov_base, iov->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 3

[PATCH v2] pasta: propagate exit code from child command
by Paul Holzinger 13 Feb '23

13 Feb '23

Exits codes are very useful for scripts, when the pasta child execvp() call fails with ENOENT that parent should also exit with > 0. In short the parent should always exit with the code from the child to make it useful in scripts. It is easy to test with: `pasta -- bash -c "exit 3"; echo $?` Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- Changes from v1: - Fixed comments from Stefano. pasta.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pasta.c b/pasta.c index 3f6477c..fe21c5b 100644 --- a/pasta.c +++ b/pasta.c @@ -64,9 +64,17 @@ void pasta_child_handler(int signal) if (pasta_child_pid && !waitid(P_PID, pasta_child_pid, &infop, WEXITED | WNOHANG)) { - if (infop.si_pid == pasta_child_pid) - exit(EXIT_SUCCESS); + if (infop.si_pid == pasta_child_pid) { + if (infop.si_code == CLD_EXITED) + exit(infop.si_status); + + /* If killed by a signal, si_status is the number. + * Follow common shell convention of returning it + 128. + */ + exit(infop.si_status + 128); + /* Nothing to do, detached PID namespace going away */ + } } waitid(P_ALL, 0, NULL, WEXITED | WNOHANG); -- 2.39.1

3 2

[PATCH] pasta: Wait for tap to be set up before spawning command
by Stefano Brivio 13 Feb '23

13 Feb '23

Adapted from a patch by Paul Holzinger: when pasta spawns a command, operating without a pre-existing user and network namespace, it needs to wait for the tap device to be configured and its handler ready, before the command is actually executed. Otherwise, something like: pasta --config-net nslookup passt.top usually fails as the nslookup command is issued before the network interface is ready. We can't adopt a simpler approach based on SIGSTOP and SIGCONT here: the child runs in a separate PID namespace, so it can't send SIGSTOP to itself as the kernel sees the child as init process and blocks the delivery of the signal. We could send SIGSTOP from the parent, but this wouldn't avoid the possible condition where the child isn't ready to wait for it when the parent sends it, also raised by Paul -- and SIGSTOP can't be blocked, so it can never be pending. Use SIGUSR1 instead: mask it before clone(), so that the child starts with it blocked, and can safely wait for it. Once the parent is ready, it sends SIGUSR1 to the child. If SIGUSR1 is sent before the child is waiting for it, the kernel will queue it for us, because it's blocked. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Fixes: 1392bc5ca002 ("Allow pasta to take a command to execute") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I just applied this, to keep things simpler for myself and for tests, given that Paul already had a look. I'm posting it anyway for reviews. passt.c | 3 +++ pasta.c | 14 +++++++++++++- pasta.h | 2 ++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/passt.c b/passt.c index 8b2c50d..d957e14 100644 --- a/passt.c +++ b/passt.c @@ -301,6 +301,9 @@ int main(int argc, char **argv) else write_pidfile(pidfile_fd, getpid()); + if (pasta_child_pid) + kill(pasta_child_pid, SIGUSR1); + isolate_postfork(&c); timer_init(&c, &now); diff --git a/pasta.c b/pasta.c index 528f02a..9169913 100644 --- a/pasta.c +++ b/pasta.c @@ -47,7 +47,7 @@ #include "log.h" /* PID of child, in case we created a namespace */ -static int pasta_child_pid; +int pasta_child_pid; /** * pasta_child_handler() - Exit once shell exits (if we started it), reap clones @@ -166,10 +166,16 @@ struct pasta_spawn_cmd_arg { static int pasta_spawn_cmd(void *arg) { const struct pasta_spawn_cmd_arg *a; + sigset_t set; if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) warn("Cannot set ping_group_range, ICMP requests might fail"); + /* Wait for the parent to be ready: see main() */ + sigemptyset(&set); + sigaddset(&set, SIGUSR1); + sigwaitinfo(&set, NULL); + a = (const struct pasta_spawn_cmd_arg *)arg; execvp(a->exe, a->argv); @@ -196,6 +202,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, char ns_fn_stack[NS_FN_STACK_SIZE]; char *sh_argv[] = { NULL, NULL }; char sh_arg0[PATH_MAX + 1]; + sigset_t set; c->foreground = 1; if (!c->debug) @@ -226,6 +233,11 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, arg.argv = sh_argv; } + /* Block SIGUSR1 in child, we queue it in main() when we're ready */ + sigemptyset(&set); + sigaddset(&set, SIGUSR1); + sigprocmask(SIG_BLOCK, &set, NULL); + pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack, sizeof(ns_fn_stack), CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET | diff --git a/pasta.h b/pasta.h index a8b9893..0ccb7e9 100644 --- a/pasta.h +++ b/pasta.h @@ -6,6 +6,8 @@ #ifndef PASTA_H #define PASTA_H +extern int pasta_child_pid; + void pasta_open_ns(struct ctx *c, const char *netns); void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, int argc, char *argv[]); -- 2.35.1

2 1

[PATCH] test/pasta_options: Ignore failures on shell 'exit'
by Stefano Brivio 13 Feb '23

13 Feb '23

On shell 'exit' commands, running shells from pasta, we might get: Cannot set tty process group (No such process) as some TTY devices might be unaccessible. This is harmless, but after commit "pasta: propagate exit code from child command", we'll get test failures there, at least with dash. Ignore those explicitly with a ugly workaround: we can't simply do something like: exit || : because the failure is reported by the shell itself once it exits, regardless of the command evaluation. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- This is already applied, I needed to unblock tests. Posting anyway for reviews. test/pasta_options/log_to_file | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/pasta_options/log_to_file b/test/pasta_options/log_to_file index 587bf8e..29fb2c8 100644 --- a/test/pasta_options/log_to_file +++ b/test/pasta_options/log_to_file @@ -34,12 +34,14 @@ set PORTS -t 10001,10002 -u 10001,10002 set LOG_FILE __STATEDIR__/pasta.log passt ./pasta -l __LOG_FILE__ -passt exit +passtb exit +sleep 1 check [ -s __LOG_FILE__ ] test Log truncated on creation passt ./pasta -l __LOG_FILE__ -passt exit +passtb exit +sleep 1 check [ $(cat __LOG_FILE__ | wc -l) -eq 1 ] test Maximum log size @@ -85,6 +87,7 @@ check_log_size_mountns test Timestamp consistency after rotations (no FALLOC_FL_COLLAPSE_RANGE) check tail -n +2 __LOG_FILE__ | cut -f1 -d' ' | tr -d [.:] | sort -c -passt exit +passtb exit +sleep 1 passt umount __STATEDIR__/t passt exit -- 2.35.1

2 1

[PATCH 1/2] pasta: correctly exit when execvp() fails
by Paul Holzinger 13 Feb '23

13 Feb '23

By default clone() will create a child that does not send SIGCHLD when the child exits. The caller has to specifiy the SIGNAL it should get in the flag bitmask. see clone(2) under "The child termination signal" This fixes the problem where pasta would not exit when the execvp() call failed, i.e. when the command does not exists. Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- pasta.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pasta.c b/pasta.c index 528f02a..3f6477c 100644 --- a/pasta.c +++ b/pasta.c @@ -229,7 +229,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid, pasta_child_pid = do_clone(pasta_spawn_cmd, ns_fn_stack, sizeof(ns_fn_stack), CLONE_NEWIPC | CLONE_NEWPID | CLONE_NEWNET | - CLONE_NEWUTS, + CLONE_NEWUTS | SIGCHLD, (void *)&arg); if (pasta_child_pid == -1) { -- 2.39.1

2 5

[PATCH] pasta: do not leak netlink sock into child
by Paul Holzinger 13 Feb '23

13 Feb '23

When spawning a child command with pasta command... pasta should not leak fds that it opened. Only the fds that were already open should be given to the child. Run `pasta --config-net -- ls -l /proc/self/fd` from a terminal where only stdin/out/err are open. The fd 3 was opend by ls to read the /proc/self/fd dir. But fd 5 is the netlink socket that was opend in pasta. To prevent such a leak we will open the socket with SOCK_CLOEXEC. Signed-off-by: Paul Holzinger <pholzing(a)redhat.com> --- netlink.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netlink.c b/netlink.c index 0850cbe..b8fa2a0 100644 --- a/netlink.c +++ b/netlink.c @@ -56,8 +56,8 @@ static int nl_sock_init_do(void *arg) if (arg) ns_enter((struct ctx *)arg); - if (((*s) = socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) < 0 || - bind(*s, (struct sockaddr *)&addr, sizeof(addr))) { + *s = socket(AF_NETLINK, SOCK_RAW | SOCK_CLOEXEC, NETLINK_ROUTE); + if (*s < 0 || bind(*s, (struct sockaddr *)&addr, sizeof(addr))) { *s = -1; return 0; } -- 2.39.1

2 2

[PATCH] Make assertions actually useful
by David Gibson 13 Feb '23

13 Feb '23

There are some places in passt/pasta which #include <assert.h> and make various assertions. If we hit these something has already gone wrong, but they're there so that we a useful message instead of cryptic misbehaviour if assumptions we thought were correct turn out not to be. Except.. the glibc implementation of assert() uses syscalls that aren't in our seccomp filter, so we'll get a SIGSYS before it actually prints the message. Work around this by adding our own ASSERT() implementation using our existing err() function to log the message, and an abort(). The abort() probably also won't work exactly right with seccomp, but once we've printed the message, dying with a SIGSYS works just as well as dying with a SIGABRT. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- inany.h | 6 ++---- lineread.c | 10 +++++----- tcp.c | 9 ++++----- tcp_splice.c | 3 +-- udp.c | 3 +-- util.c | 1 - util.h | 20 +++++++++++++++++++- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/inany.h b/inany.h index 52d43f2..7ff32f0 100644 --- a/inany.h +++ b/inany.h @@ -6,8 +6,6 @@ * IPv6 or IPv4 (encoded as IPv4-mapped IPv6 addresses) */ -#include <assert.h> - /** union inany_addr - Represents either an IPv4 or IPv6 address * @a6: Address as an IPv6 address, may be IPv4-mapped * @v4mapped.zero: All zero-bits for an IPv4 address @@ -63,7 +61,7 @@ static inline void inany_from_af(union inany_addr *aa, int af, const void *addr) aa->v4mapped.a4 = *((struct in_addr *)addr); } else { /* Not valid to call with other address families */ - assert(0); + ASSERT(0); } } @@ -89,6 +87,6 @@ static inline void inany_from_sockaddr(union inany_addr *aa, in_port_t *port, *port = ntohs(sa4->sin_port); } else { /* Not valid to call with other address families */ - assert(0); + ASSERT(0); } } diff --git a/lineread.c b/lineread.c index 59021e6..6ffb14e 100644 --- a/lineread.c +++ b/lineread.c @@ -16,10 +16,10 @@ #include <fcntl.h> #include <string.h> #include <stdbool.h> -#include <assert.h> #include <unistd.h> #include "lineread.h" +#include "util.h" /** * lineread_init() - Prepare for line by line file reading without allocation @@ -44,10 +44,10 @@ static int peek_line(struct lineread *lr, bool eof) char *nl; /* Sanity checks (which also document invariants) */ - assert(lr->count >= 0); - assert(lr->next_line >= 0); - assert(lr->next_line + lr->count >= lr->next_line); - assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE); + ASSERT(lr->count >= 0); + ASSERT(lr->next_line >= 0); + ASSERT(lr->next_line + lr->count >= lr->next_line); + ASSERT(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE); nl = memchr(lr->buf + lr->next_line, '\n', lr->count); diff --git a/tcp.c b/tcp.c index 4744ac5..475c23f 100644 --- a/tcp.c +++ b/tcp.c @@ -288,7 +288,6 @@ #include <sys/uio.h> #include <unistd.h> #include <time.h> -#include <assert.h> #include <linux/tcp.h> /* For struct tcp_info */ @@ -602,7 +601,7 @@ static inline struct tcp_tap_conn *conn_at_idx(int index) { if ((index < 0) || (index >= TCP_MAX_CONNS)) return NULL; - assert(!(CONN(index)->c.spliced)); + ASSERT(!(CONN(index)->c.spliced)); return CONN(index); } @@ -2809,7 +2808,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, socklen_t sl; int s; - assert(ref.r.p.tcp.tcp.listen); + ASSERT(ref.r.p.tcp.tcp.listen); if (c->tcp.conn_count >= TCP_MAX_CONNS) return; @@ -3051,7 +3050,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) struct in_addr loopback = { htonl(INADDR_LOOPBACK) }; int s; - assert(c->mode == MODE_PASTA); + ASSERT(c->mode == MODE_PASTA); s = sock_l4(c, AF_INET, IPPROTO_TCP, &loopback, NULL, port, tref.u32); if (s >= 0) @@ -3075,7 +3074,7 @@ static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port) .tcp.index = idx }; int s; - assert(c->mode == MODE_PASTA); + ASSERT(c->mode == MODE_PASTA); s = sock_l4(c, AF_INET6, IPPROTO_TCP, &in6addr_loopback, NULL, port, tref.u32); diff --git a/tcp_splice.c b/tcp_splice.c index 72b1672..1d624f1 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -46,7 +46,6 @@ #include <sys/epoll.h> #include <sys/types.h> #include <sys/socket.h> -#include <assert.h> #include "util.h" #include "passt.h" @@ -519,7 +518,7 @@ bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, union inany_addr aany; in_port_t port; - assert(c->mode == MODE_PASTA); + ASSERT(c->mode == MODE_PASTA); inany_from_sockaddr(&aany, &port, sa); a4 = inany_v4(&aany); diff --git a/udp.c b/udp.c index dbfdb61..9c63a8c 100644 --- a/udp.c +++ b/udp.c @@ -108,7 +108,6 @@ #include <sys/uio.h> #include <unistd.h> #include <time.h> -#include <assert.h> #include "checksum.h" #include "util.h" @@ -255,7 +254,7 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd) { int i; - assert(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta)); + ASSERT(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta)); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; diff --git a/util.c b/util.c index d9858f2..c5ee1c0 100644 --- a/util.c +++ b/util.c @@ -23,7 +23,6 @@ #include <time.h> #include <errno.h> #include <stdbool.h> -#include <assert.h> #include "util.h" #include "passt.h" diff --git a/util.h b/util.h index 3baec91..a367f96 100644 --- a/util.h +++ b/util.h @@ -6,6 +6,11 @@ #ifndef UTIL_H #define UTIL_H +#include <stdlib.h> +#include <stdarg.h> + +#include "log.h" + #define VERSION_BLOB \ VERSION "\n" \ "Copyright Red Hat\n" \ @@ -47,6 +52,18 @@ #define STRINGIFY(x) #x #define STR(x) STRINGIFY(x) +#define ASSERT(expr) \ + do { \ + if (!(expr)) { \ + err("ASSERTION FAILED in %s (%s:%d): %s", \ + __func__, __FILE__, __LINE__, STRINGIFY(expr)); \ + /* This may actually SIGSYS, due to seccomp, \ + * but that will still get the job done \ + */ \ + abort(); \ + } \ + } while (0) + #ifdef P_tmpdir #define TMPDIR P_tmpdir #else @@ -163,7 +180,8 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags, #include <net/if.h> #include <limits.h> -#include <stdarg.h> +#include <stdint.h> +#include <netinet/ip6.h> #include "packet.h" -- 2.39.0

2 1