- dev - passt

new version 2022_10_26.f212044 available
by Stefano Brivio 27 Oct '22

27 Oct '22

The new version with tag 2022_10_26.f212044 includes the following changes: f212044 icmp: Don't discard first reply sequence for a given echo ID b062ee4 icmp: Add debugging messages for handled replies and requests 947d756 tap: Trace received (outbound) ICMP packets in debug mode, too 7402951 conf, passt.1: Don't imply --foreground with --debug https://passt.top/passt/log/?qt=range&q=2022_10_26.e4df8b0..2022_10_26.f212… Packages: - Copr (CentOS Stream, EPEL, Fedora, Mageia, openSUSE): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/4991606/ permanent mirror: https://passt.top/builds/copr/0^20221026.ge4df8b0/ - Debian package, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt_0f212044-1_all.deb - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Package for other RPM-based distributions, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt-gf212044-1.x86_64.rpm - x86_64 static binaries: https://passt.top/builds/latest/x86_64/ -- Stefano

1 2

[PATCH] test/run: Temporarily disable distribution tests
by Stefano Brivio 27 Oct '22

27 Oct '22

They're too slow to cope with current release cycles, and they haven't found bugs in months, also because clang-tidy and cppcheck would find most of them earlier. Disable them for the moment. We should pre-install gcc and make in non-x86 images, as those run on my test machine with qemu TCG, and that's the real slow-down here. Then we can re-enable them. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- test/run | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/run b/test/run index d61b2d3..cb3b6c0 100755 --- a/test/run +++ b/test/run @@ -110,12 +110,16 @@ run() { test passt_in_ns/shutdown teardown passt_in_ns + # TODO: Make those faster by at least pre-installing gcc and make on + # non-x86 images, then re-enable. +skip_distro() { setup distro test distro/debian test distro/fedora test distro/opensuse test distro/ubuntu teardown distro +} perf_finish [ ${CI} -eq 1 ] && video_stop -- 2.35.1

2 1

[PATCH 0/4] Debugging improvements, fix for 0 sequence echo reply
by Stefano Brivio 26 Oct '22

26 Oct '22

Patches 1/4 to 3/4 make debugging of the issue solved in 4/4 more viable. Stefano Brivio (4): conf, passt.1: Don't imply --foreground with --debug tap: Trace received (outbound) ICMP packets in debug mode, too icmp: Add debugging messages for handled replies and requests icmp: Don't discard first reply sequence for a given echo ID conf.c | 7 +++---- icmp.c | 46 +++++++++++++++++++++++++++++++++++++++------- icmp.h | 1 + passt.1 | 5 ++--- passt.c | 3 +++ tap.c | 2 ++ 6 files changed, 50 insertions(+), 14 deletions(-) -- 2.35.1

2 8

[PATCH] checksum: Fix calculation for ICMP checksum on IPv4
by Stefano Brivio 26 Oct '22

26 Oct '22

We need to zero out the checksum field before calculating the checksum, of course. I have no idea how this passed the "icmp" test set, looking into it. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Fixes: 67ab6171729c ("Add csum_icmp4() helper for calculating ICMP checksums") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- checksum.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/checksum.c b/checksum.c index 09d2c7c..c59869c 100644 --- a/checksum.c +++ b/checksum.c @@ -160,10 +160,13 @@ void csum_udp4(struct udphdr *udp4hr, in_addr_t saddr, in_addr_t daddr, */ void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t len) { - /* Partial checksum for ICMP header alone */ - uint32_t psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + uint32_t psum; icmp4hr->checksum = 0; + + /* Partial checksum for ICMP header alone */ + psum = sum_16b(icmp4hr, sizeof(*icmp4hr)); + icmp4hr->checksum = csum_unaligned(payload, len, psum); } -- 2.35.1

3 7

[PATCH 0/8] Add support for log file, version display, and tests
by Stefano Brivio 26 Oct '22

26 Oct '22

Patches 1/8 to 4/8 add support for logging to a file, via -l/--log-file, with mandatory size limit and rotation. Patches 5/8 and 7/8 fix two minor details that came up while implementing the feature itself. Patch 6/8 adds a -v/--version option with a version string generated by the Makefile using git, if available, and includes the version string in the log header. Finally, patch 8/8 introduces related tests, using pasta. Stefano Brivio (8): Move logging functions to a new file, log.c conf: Drop duplicate, diverging optstring assignments passt.h: Include netinet/if_ether.h before struct ctx declaration log, conf: Add support for logging to file log: Add missing function comment for trace_init() conf, log, Makefile: Add versioning information util: Check return value of lseek() while reading bound ports from procfs test: Add log file tests for pasta plus corresponding layout and setup Makefile | 9 +- README.md | 2 +- conf.c | 74 +++++-- dhcp.c | 1 + dhcpv6.c | 1 + icmp.c | 1 + isolation.c | 1 + log.c | 362 +++++++++++++++++++++++++++++++++ log.h | 32 +++ ndp.c | 1 + netlink.c | 1 + packet.c | 1 + passt.1 | 18 +- passt.c | 2 + passt.h | 2 + pasta.c | 1 + pcap.c | 1 + tap.c | 1 + tcp.c | 1 + tcp_splice.c | 1 + test/lib/layout | 39 ++++ test/lib/setup | 15 +- test/lib/test | 3 + test/pasta_options/log_to_file | 90 ++++++++ test/run | 4 + udp.c | 1 + util.c | 131 +----------- util.h | 22 +- 28 files changed, 658 insertions(+), 160 deletions(-) create mode 100644 log.c create mode 100644 log.h create mode 100644 test/pasta_options/log_to_file -- 2.35.1

2 28

[PATCH] hooks: Temporarily disable demo generation in pre-push
by Stefano Brivio 26 Oct '22

26 Oct '22

The out-of-tree Podman patch needs to be rebased every second week or so, and I'm currently trying to get that upstream: https://github.com/containers/podman/pull/16141 Disable demo generation for the moment, so that I avoid wasting time with those rebases. We'll re-enable it later. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- hooks/pre-push | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/hooks/pre-push b/hooks/pre-push index ebca1f6..89717e7 100755 --- a/hooks/pre-push +++ b/hooks/pre-push @@ -21,13 +21,13 @@ BASE="/var/www/passt" BUILDS="${BASE}/builds" LATEST="${BUILDS}/latest" TEMP="${BUILDS}/temp" -AWAY="${BUILDS}/away" WEB="${TEMP}/web" TEST="${TEMP}/test" ARCH="$(uname -m)" BIN="${TEMP}/${ARCH}" +OLD_BIN="${LATEST}/${ARCH}" ssh "${USER_HOST}" "mkdir -p ${WEB} ${TEST} ${BIN}" @@ -41,12 +41,15 @@ for f in test_logs/web/*.cast test_logs/web/*.js; do scp "${f}.gz" "${USER_HOST}:${WEB}/" done +# TODO: Re-enable demos once we have pasta support in upstream Podman +skip() { ./run_demo for f in test_logs/web/*.cast test_logs/web/*.js; do gzip -fk9 "${f}" scp "${f}" "${USER_HOST}:${WEB}/" scp "${f}.gz" "${USER_HOST}:${WEB}/" done +} cd .. @@ -68,6 +71,10 @@ for pic in passt_overview pasta_overview; do done scp doc/*.html "${USER_HOST}:${WEB}/" -ssh "${USER_HOST}" "mv ${LATEST} ${AWAY}" -ssh "${USER_HOST}" "mv ${TEMP} ${LATEST}" -ssh "${USER_HOST}" "rm -rf ${AWAY}" +ssh "${USER_HOST}" "cp ${WEB}/* ${LATEST}/web/" +ssh "${USER_HOST}" "cp ${TEST}/* ${LATEST}/test/" + +ssh "${USER_HOST}" "rm -f ${OLD_BIN}/*" +ssh "${USER_HOST}" "cp ${BIN}/* ${LATEST}/${ARCH}/" + +ssh "${USER_HOST}" "rm -rf ${TEMP}" -- 2.35.1

1 0

[PATCH] conf: Don't pass leading ~ to parse_port_range() on exclusions
by Stefano Brivio 25 Oct '22

25 Oct '22

Commit 84fec4e998b6 ("Clean up parsing of port ranges") drops the strspn() call before the parsing of excluded port ranges, because now we're checking against any stray characters at every step. However, that also has the effect of passing ~ as first character to the new parse_port_range(), which makes no sense: we already checked that ~ is the first character before the call, so skip it. Alona reported this output: Invalid port specifier ~15000,~15001,~15006,~15008,~15020,~15021,~15090 while the whole specifier is indeed valid. Reported-by: Alona Paz <alkaplan(a)redhat.com> Fixes: 84fec4e998b6 ("Clean up parsing of port ranges") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I'm not introducing new tests with this yet, this is another instance where I would need a test case restarting pasta a bunch of times with several different options instead of a fixed setup. I'll reopen the discussion around: https://archives.passt.top/passt-dev/Yz%2FlXVVDgMEWweTj@yekko/#t Also, I'm posting this for review, but again, let me push this out quickly, as configuration of excluded ports is completely broken at the moment. conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/conf.c b/conf.c index ed93a60..598c711 100644 --- a/conf.c +++ b/conf.c @@ -262,6 +262,7 @@ static int conf_ports(const struct ctx *c, char optname, const char *optarg, exclude_only = false; continue; } + p++; if (parse_port_range(p, &p, &xrange)) goto bad; -- 2.35.1

2 1

[PATCH] qrap: Support JSON syntax for -device
by Andrea Bolognani 24 Oct '22

24 Oct '22

Starting with version 8.1.0, libvirt uses JSON syntax when generating the arguments to -device, so they will now look like {"driver":"virtio-scsi-pci","bus":"pci.3","addr":"0x0"} instead of virtio-scsi-pci,bus=pci.3,addr=0x0 qrap needs to parse these arguments and extract the bus number in order to figure out what address to use for the virtio-net device it adds, and the libvirt change described above has broken this parsing logic. Tweak the code so that both styles are accepted and handled correctly. Note that, when JSON is in use, qrap needs to generate its own command line options in that format as well or things will not work as expected. Signed-off-by: Andrea Bolognani <abologna(a)redhat.com> --- qrap.c | 51 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/qrap.c b/qrap.c index a9a0fc1..3c6f5b8 100644 --- a/qrap.c +++ b/qrap.c @@ -69,6 +69,8 @@ static const struct drop_arg { * @name: Device ("-device") name to insert * @template: Prefix for device specification (first part of address) * @template_post: Suffix for device specification (last part of address) + * @template_json: Device prefix for when JSON is used + * @template_json_post: Device suffix for when JSON is used * @first: First usable PCI address * @last: Last usable PCI address */ @@ -77,15 +79,29 @@ static const struct pci_dev { char *name; char *template; char *template_post; + char *template_json; + char *template_json_post; int first; int last; } pci_devs[] = { - { "pc-q35", "virtio-net-pci", - "bus=pci.", ",addr=0x0", 3, /* 2: hotplug bus */ 16 }, - { "pc-", "virtio-net-pci", - "bus=pci.0,addr=0x", "", 2, /* 1: ISA bridge */ 16 }, - { "s390-ccw", "virtio-net-ccw", - "devno=fe.0.", "", 1, 16 }, + { + "pc-q35", "virtio-net-pci", + "bus=pci.", ",addr=0x0", + "\"bus\":\"pci.", ",\"addr\":\"0x0\"", + 3, /* 2: hotplug bus */ 16 + }, + { + "pc-", "virtio-net-pci", + "bus=pci.0,addr=0x", "", + "\"bus\":\"pci.0\",\"addr=0x", "", + 2, /* 1: ISA bridge */ 16 + }, + { + "s390-ccw", "virtio-net-ccw", + "devno=fe.0.", "", + "\"devno\":\"fe.0.", "", + 1, 16 + }, { 0 }, }; @@ -115,7 +131,7 @@ void usage(const char *name) */ int main(int argc, char **argv) { - int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset, rc; + int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, has_json = 0, retry_on_reset, rc; struct timeval tv = { .tv_sec = 0, .tv_usec = (long)(500 * 1000) }; char *qemu_argv[ARG_MAX], dev_str[ARG_MAX]; struct sockaddr_un addr = { @@ -227,14 +243,22 @@ int main(int argc, char **argv) } if (!strcmp(argv[i], "-device") && i + 1 < argc) { + char *template = NULL; char *p; has_dev = 1; if ((p = strstr(argv[i + 1], dev->template))) { + template = dev->template; + } else if ((p = strstr(argv[i + 1], dev->template_json))) { + template = dev->template_json; + has_json = 1; + } + + if (template) { long n; - n = strtol(p + strlen(dev->template), NULL, 16); + n = strtol(p + strlen(template), NULL, 16); if (!errno) addr_map |= (1 << n); } @@ -254,8 +278,15 @@ int main(int argc, char **argv) if (has_dev) { qemu_argv[qemu_argc++] = "-device"; - snprintf(dev_str, ARG_MAX, "%s,%s%x%s,netdev=hostnet0,x-txburst=4096", - dev->name, dev->template, i, dev->template_post); + if (!has_json) { + snprintf(dev_str, ARG_MAX, + "%s,%s%x%s,netdev=hostnet0,x-txburst=4096", + dev->name, dev->template, i, dev->template_post); + } else { + snprintf(dev_str, ARG_MAX, + "{\"driver\":\"%s\",%s%x\"%s,\"netdev\":\"hostnet0\",\"x-txburst\":4096}", + dev->name, dev->template_json, i, dev->template_json_post); + } qemu_argv[qemu_argc++] = dev_str; } -- 2.37.3

2 3

[PATCH] util: Set NS_FN_STACK_SIZE to one eighth of ulimit-reported maximum stack size
by Stefano Brivio 24 Oct '22

24 Oct '22

...instead of one fourth. On the main() -> conf() -> nl_sock_init() call path, LTO from gcc 12 on (at least) x86_64 decides to inline... everything: nl_sock_init() is effectively part of main(), after commit 3e2eb4337bc0 ("conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()"). This means we exceed the maximum stack size, and we get SIGSEGV, under any condition, at start time, as reported by Andrea on a recent build for CentOS Stream 9. The calculation of NS_FN_STACK_SIZE, which is the stack size we reserve for clones, was previously obtained by dividing the maximum stack size by two, to avoid an explicit check on architecture (on PA-RISC, also known as hppa, the stack grows up, so we point the clone to the middle of this area), and then further divided by two to allow for any additional usage in the caller. Well, if there are essentially no function calls anymore, this is not enough. Divide it by eight, which is anyway much more than possibly needed by any clone()d callee. I think this is robust, so it's a fix in some sense. Strictly speaking, though, we have no formal guarantees that this isn't either too little or too much. What we should do, eventually: check cloned() callees, there are just thirteen of them at the moment. Note down any stack usage (they are mostly small helpers), bonus points for an automated way at build time, quadruple that or so, to allow for extreme clumsiness, and use as NS_FN_STACK_SIZE. Perhaps introduce a specific condition for hppa. Reported-by: Andrea Bolognani <abologna(a)redhat.com> Fixes: 3e2eb4337bc0 ("conf: Bind inbound ports with CAP_NET_BIND_SERVICE before isolate_user()") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- util.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util.h b/util.h index 27829b1..c498a80 100644 --- a/util.h +++ b/util.h @@ -72,7 +72,7 @@ #define IPV4_IS_LOOPBACK(addr) \ ((addr) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET) -#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 4) +#define NS_FN_STACK_SIZE (RLIMIT_STACK_VAL * 1024 / 8) #define NS_CALL(fn, arg) \ do { \ char ns_fn_stack[NS_FN_STACK_SIZE]; \ -- 2.35.1

2 4

passt crashes on CentOS Stream 9
by Andrea Bolognani 22 Oct '22

22 Oct '22

This should probably be filed on bugzilla but I can't be bothered signing up for yet another service, sorry! O:-) Short version: in a CentOS Stream 9 container, install the latest build (0^20221015.gb3f3591-1) from the official COPR, then run $ passt --runas 65534 -e -t 1234 Segmentation fault (core dumped) Doing the same thing in a CentOS Stream 8 container doesn't result in a crash, and the previous build (0^20220929.g06aa26f-1) is fine even on CentOS Stream 9. The backtrace produced by gdb doesn't look very illuminating, but maybe it will make more sense to a developer: Starting program: /usr/bin/passt --runas 65534 -e -t 1234 warning: Error disabling address space randomization: Operation not permitted [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". process 2856 is executing new program: /usr/bin/passt.avx2 warning: Could not load shared library symbols for linux-vdso.so.1. Do you need "set solib-search-path" or "set sysroot"? [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". Program received signal SIGSEGV, Segmentation fault. 0x000055663d5307ff in nl_sock_init (c=0x7fffd7fe4ed0, ns=false) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/netlink.c:78 78 { (gdb) t a a bt Thread 1 (Thread 0x7fe8763da740 (LWP 2856) "passt.avx2"): #0 0x000055663d5307ff in nl_sock_init (c=0x7fffd7fe4ed0, ns=false) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/netlink.c:78 #1 0x000055663d531296 in conf (c=<optimized out>, argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/conf.c:1547 #2 0x000055663d5262e6 in main (argc=6, argv=0x7fffd82b2a98) at /usr/src/debug/passt-0^20221015.gb3f3591-1.el9.x86_64/passt.c:243 A very interesting thing that I've noticed is that the crash doesn't occur when building from upstream sources (tag 2022_10_15.b3f3591, so it should match what's in the RPM). So I've tried looking into the compiler options used during the RPM build, and the gcc command line for passt.avx2 looks like gcc -Wall -Wextra -pedantic -std=c99 -D_XOPEN_SOURCE=700 \ -D_GNU_SOURCE -D_FORTIFY_SOURCE=2 -pie -fPIE -DPAGE_SIZE=4096 \ -DNETNS_RUN_DIR=\"/run/netns\" -DPASST_AUDIT_ARCH=AUDIT_ARCH_X86_64 \ -DRLIMIT_STACK_VAL=8192 -DARCH=\"x86_64\" \ -DVERSION=\"0^20221015.gb3f3591-1.el9.x86_64\" -DTCP_HASH_NOINLINE \ -DSIPHASH_20B_NOINLINE -DCSUM_UNALIGNED_NO_IPA -DHAS_SND_WND \ -DHAS_BYTES_ACKED -DHAS_MIN_RTT -DHAS_GETRANDOM \ -fstack-protector-strong -Ofast -mavx2 -ftree-vectorize \ -funroll-loops -flto=auto -ffat-lto-objects -fexceptions -g \ -grecord-gcc-switches -pipe -Wall -Werror=format-security \ -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS \ -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 \ -fstack-protector-strong \ -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64-v2 \ -mtune=generic -fasynchronous-unwind-tables \ -fstack-clash-protection -fcf-protection arch.c arp.c checksum.c \ conf.c dhcp.c dhcpv6.c icmp.c igmp.c isolation.c lineread.c log.c \ mld.c ndp.c netlink.c packet.c passt.c pasta.c pcap.c siphash.c \ tap.c tcp.c tcp_splice.c udp.c util.c -o passt.avx2 -Wl,-z,relro \ -Wl,--as-needed -Wl,-z,now \ -specs=/usr/lib/rpm/redhat/redhat-hardened-ld \ -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 I tried making educated guesses at which ones among those could cause trouble, and pretty quickly landed on the LTO stuff. Indeed, dropping -flto=auto -ffat-lto-objects from the command results in a working binary, and adding %global _lto_cflags %nil to the top of the spec file produces a working RPM. Of course disabling LTO is a workaround, not a solution, especially considering that the previous version didn't have any problem with it, but hopefully there's enough information in here to allow the developers to track down and resolve the underlying issue :) -- Andrea Bolognani / Red Hat / Virtualization

2 4