- dev - passt

[PATCH 0/5] More test fixes for Fedora host
by David Gibson 18 Jun '22

18 Jun '22

Hi Stefano, Here's another batch of fixes for the passt/pasta tests. With this lot I'm now able to get the build and pasta tests passing on my Fedora workstation. The passt and distro tests still fail or crash, I'm continuing to look at those (it looks like there are some very fragile behaviours in qrap which means they're not even getting started). David Gibson (5): tests: Use more explicit netcat options for distro/fedora tests Don't abbreviate ip(8) arguments in examples and tests Tweak dhclient arguments for readability Use dhclient instead of udhcpc tests: Use nmap-ncat instead of openbsd netcat for pasta tests contrib/kata-containers/README.md | 2 +- doc/demo.sh | 8 ++--- passt.1 | 16 +++------ test/README.md | 2 +- test/demo/passt | 16 ++++----- test/demo/pasta | 24 ++++++------- test/demo/podman | 20 +++++------ test/dhcp/passt | 24 ++++++------- test/dhcp/pasta | 26 +++++++------- test/distro/fedora | 14 ++++---- test/distro/ubuntu | 4 +-- test/icmp/passt_in_ns | 4 +-- test/lib/setup | 12 +++---- test/ndp/passt | 12 +++---- test/ndp/pasta | 12 +++---- test/perf/passt_tcp | 8 ++--- test/perf/passt_udp | 8 ++--- test/perf/pasta_tcp | 6 ++-- test/perf/pasta_udp | 6 ++-- test/tcp/passt | 6 ++-- test/tcp/passt_in_ns | 22 ++++++------ test/tcp/pasta | 60 +++++++++++++++---------------- test/two_guests/basic | 26 +++++++------- test/udp/passt | 6 ++-- test/udp/passt_in_ns | 14 ++++---- test/udp/pasta | 35 +++++++++--------- 26 files changed, 193 insertions(+), 200 deletions(-) -- 2.36.1

2 11

[PATCH] tap: Add informational messages for UNIX domain socket connections
by Stefano Brivio 17 Jun '22

17 Jun '22

...namely, as connections are discarded or accepted. This was quite useful to debug an issue with libvirtd failing to start qemu (because passt refused the new connection) as a previous qemu instance was still active. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tap.c b/tap.c index 04ceade..23414db 100644 --- a/tap.c +++ b/tap.c @@ -856,12 +856,19 @@ static void tap_sock_unix_new(struct ctx *c) { struct epoll_event ev = { 0 }; int v = INT_MAX / 2; + struct ucred ucred; + socklen_t len; + + len = sizeof(ucred); /* Another client is already connected: accept and close right away. */ if (c->fd_tap != -1) { int discard = accept4(c->fd_tap_listen, NULL, NULL, SOCK_NONBLOCK); + if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("discardingq connection from PID %i", ucred.pid); + if (discard != -1) close(discard); @@ -870,6 +877,9 @@ static void tap_sock_unix_new(struct ctx *c) c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, 0); + if (!getsockopt(c->fd_tap, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("accepted connection from PID %i", ucred.pid); + if (!c->low_rmem && setsockopt(c->fd_tap, SOL_SOCKET, SO_RCVBUF, &v, sizeof(v))) trace("tap: failed to set SO_RCVBUF to %i", v); -- 2.35.1

2 1

[PATCH] conf: In conf_runas(), on static builds, group information is also unused
by Stefano Brivio 16 Jun '22

16 Jun '22

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/conf.c b/conf.c index d615cf5..3a5d650 100644 --- a/conf.c +++ b/conf.c @@ -868,6 +868,7 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid) (void)ubuf; (void)gbuf; (void)pw; + (void)gr; return -EINVAL; #else -- 2.35.1

1 0

[PATCH] qrap: Add probe retry on connection reset from passt for KubeVirt integration
by Stefano Brivio 16 Jun '22

16 Jun '22

KubeVirt uses libvirt to start qrap in its current draft integration (https://github.com/kubevirt/kubevirt/pull/7849/), and libvirtd starts qrap three times every time a new virtual machine is created: once on domain creation, and twice on domain start (for "probing") and to finally start it for real. Very often, a subsequent invocation of qrap happen before the previously running instance of qemu terminates, which means that passt will refuse the new connection as the old one is still active. Introduce a single retry with a 100ms delay to work around this. This should be checked again once native libvirt support is there, and that point qrap will have no reason to exist anymore. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- qrap.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/qrap.c b/qrap.c index 17cc472..303e981 100644 --- a/qrap.c +++ b/qrap.c @@ -25,6 +25,7 @@ #include <net/if_arp.h> #include <netinet/in.h> #include <netinet/if_ether.h> +#include <time.h> #include "util.h" #include "passt.h" @@ -112,7 +113,7 @@ void usage(const char *name) int main(int argc, char **argv) { struct timeval tv = { .tv_sec = 0, .tv_usec = (long)(500 * 1000) }; - int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0; + int i, s, qemu_argc = 0, addr_map = 0, has_dev = 0, retry_on_reset; char *qemu_argv[ARG_MAX], dev_str[ARG_MAX]; struct sockaddr_un addr = { .sun_family = AF_UNIX, @@ -233,6 +234,9 @@ int main(int argc, char **argv) valid_args: for (i = 1; i < UNIX_SOCK_MAX; i++) { + retry_on_reset = 1; + +retry: s = socket(AF_UNIX, SOCK_STREAM, 0); if (s < 0) { perror("socket"); @@ -254,6 +258,34 @@ valid_args: else break; + /* FIXME: in a KubeVirt environment, libvirtd invokes qrap three + * times in a strict sequence when a virtual machine needs to + * be started, namely, when: + * - the domain XML is saved + * - the domain is started (for "probing") + * - the virtual machine is started for real + * and it often happens that the qemu process is still running + * when qrap is invoked again, so passt will refuse the new + * connection because the previous one is still active. This + * overlap seems to be anywhere between 0 and 3ms. + * + * If we get a connection reset, retry, just once, after 100ms, + * to allow for the previous qemu instance to terminate and, in + * turn, for the connection to passt to be closed. + * + * This should be fixed in libvirt instead. It probably makes + * sense to check this behaviour once native libvirt support is + * there, and this implies native qemu support too, so at that + * point qrap will have no reason to exist anymore -- that is, + * this FIXME will probably remain until the tool itself is + * obsoleted. + */ + if (retry_on_reset && errno == ECONNRESET) { + retry_on_reset = 0; + usleep(100 * 1000); + goto retry; + } + fprintf(stderr, "Probe of %s failed\n", addr.sun_path); close(s); -- 2.35.1

1 0

passt & mbuto
by David Gibson 16 Jun '22

16 Jun '22

Hi again, I realized I wasn't quite right when I said that qrap problems where what was currently stopping me running the passt (not pasta) tests. I did hit qrap issues somewhere, but the current stumbling block is that mbuto looks for udhcpc to put into the guest image, which I can't easily put onto my host system. Now, in the short term, once my patch to remove usage of udhcpc from the passt/pasta tests is applied, we could just remove udhcpc from the mbuto profile as well. However, that raises a wider scope issue: The passt testing profile for mbuto appliances is in the mbuto tree, not the passt tree. That doesn't realy make sense, since it means any change to what we need for the passt tests requires a synchronized change with mbuto. Particularly for a pretty young and project like passt, that's not really tenable. Plus, slurping an external tool from some random URL in the tests is just kinda ugly. I'm not immediately sure how best to to address this: * We could make mbuto take the profiles as some sort of external file, so they can be provided by the user, rather than built into the mbuto repository. * We could just fork a copy of mbuto into the passt tree, making local modifications for the profile, and only manually updating it to match upstream mbuto changes. * We could use an entirely different and more established tool for building our testing guest images in passt (e.g. supermin, buildroot or just picking a standard distro guest image) -- 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 4

[PATCH] Makefile: Suppress unusedStructMember Cppcheck warning in dhcp.c
by Stefano Brivio 14 Jun '22

14 Jun '22

New from Cppcheck 2.8: all the fields of struct msg that are not directly manipulated are now reported as unused, which is kind of correct as those fields are used as a blob "copied" from request to response and not as separate fields. However, keeping the message composition explicit is probably desirable, and adding inline suppressions makes the whole thing rather unreadable, so just suppress unusedStructMember warnings for dhcp.c, while also adding a suppression for unmatched suppressions to keep earlier versions of Cppcheck happy. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 9f2ec3a..b0de1ec 100644 --- a/Makefile +++ b/Makefile @@ -259,8 +259,10 @@ cppcheck: $(wildcard *.c) $(wildcard *.h) --suppress=localtimeCalled:pcap.c \ --suppress=unusedStructMember:pcap.c \ --suppress=funcArgNamesDifferent:util.h \ + --suppress=unusedStructMember:dhcp.c \ \ --suppress=unmatchedSuppression:conf.c \ + --suppress=unmatchedSuppression:dhcp.c \ --suppress=unmatchedSuppression:passt.c \ --suppress=unmatchedSuppression:pcap.c \ --suppress=unmatchedSuppression:qrap.c \ -- 2.35.1

1 0

Bugzilla complains about invalid content-type
by Carl Winbäck 09 Jun '22

09 Jun '22

Hi all! When I try to interact with Bugzilla, this error pops up at various locations: “Software error: Invalid Content-Type 'subtype' parameter at Bugzilla/BugMail.pm line 499. For help, please send mail to this site's webmaster, giving this error message and the time and date of the error.” [URL: https://bugs.passt.top/process_bug.cgi] How to reproduce: * Go to a ticket, e.g. https://bugs.passt.top/show_bug.cgi?id=6 * Write a comment in the Additional Comments textarea. * Click on the button labeled “Save Changes” Despite the error message, my comment was saved sucessfully. The above error message also showed when I (sucessfully) attempted to add my self to the CC list of this ticket. Best regards, Carl Winbäck

2 3

[PATCH] tcp: Work around gcc 12 bogus warning in tcp_rtt_dst_check()
by Stefano Brivio 25 May '22

25 May '22

gcc 12.1.x (e.g. current OpenSUSE Tumbleweed, x86_64 only, gcc-12-1.4.x86_64) reports: tcp.c: In function ‘tcp_send_flag’: tcp.c:1014:9: warning: writing 16 bytes into a region of size 0 [-Wstringop-overflow=] 1014 | memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6)); | ^ tcp.c:559:24: note: at offset -16 into destination object ‘low_rtt_dst’ of size 128 559 | static struct in6_addr low_rtt_dst[LOW_RTT_TABLE_SIZE]; | but 'hole' can't be -1, because the low_rtt_dst table is guaranteed to have a hole: if we happened to write to the last entry, we'll go back to index 0 and clear that one. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tcp.c b/tcp.c index e68409a..53af3db 100644 --- a/tcp.c +++ b/tcp.c @@ -1011,6 +1011,12 @@ static void tcp_rtt_dst_check(const struct tcp_conn *conn, hole = i; } + /* Keep gcc 12 happy: this won't actually happen because the table is + * guaranteed to have a hole, see the second memcpy() below. + */ + if (hole == -1) + return; + memcpy(low_rtt_dst + hole++, &conn->a.a6, sizeof(conn->a.a6)); if (hole == LOW_RTT_TABLE_SIZE) hole = 0; -- 2.35.1

2 2

[PATCH] conf: Fix one Coverity CID 258163 warning, work around another one
by Stefano Brivio 24 May '22

24 May '22

In conf_runas(), Coverity reports that we might dereference uid and gid despite possibly being NULL (CWE-476) because of the check after the first sscanf(). They can't be NULL, but I actually wanted to check that UID and GID are non-zero (the user could otherwise pass --runas root:root and defy the whole mechanism). Later on, we have the same type of warning for 'gr': it's compared against NULL, so it might be NULL, which is actually the case: but in that case, we don't dereference it, because we'll return -ENOENT right away. Rewrite the clause to silence the warning. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/conf.c b/conf.c index ddad9a3..d615cf5 100644 --- a/conf.c +++ b/conf.c @@ -857,7 +857,7 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid) struct group *gr; /* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */ - if (sscanf(opt, "%u:%u", uid, gid) == 2 && uid && gid) + if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid) return 0; *uid = strtol(opt, &endptr, 0); @@ -874,12 +874,10 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid) /* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */ if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:" "%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) { - pw = getpwnam(ubuf); - if (!pw || !(*uid = pw->pw_uid)) + if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid)) return -ENOENT; - gr = getgrnam(gbuf); - if (!gr || !(*gid = gr->gr_gid)) + if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid)) return -ENOENT; return 0; -- 2.35.1

2 1

[PATCH v3] udp: Ignore bogus -Wstringop-overread for write() from gcc 12.1
by Stefano Brivio 19 May '22

19 May '22

With current OpenSUSE Tumbleweed on aarch64 (gcc-12-1.3.aarch64) and on x86_64 (gcc-12-1.4.x86_64), but curiously not on armv7hl (gcc-12-1.3.armv7hl), gcc warns about using the _pointer_ to the 802.3 header to write the whole frame to the tap descriptor: reading between 62 and 4294967357 bytes from a region of size 14 which is bogus: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=103483 Probably declaring udp_sock_fill_data_v{4,6}() as noinline would "fix" this, but that's on the data path, so I'd rather not. Use a gcc pragma instead. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: LLVM 13.0.1 doesn't know about -Wstringop-overread, and clang-tidy warns about the unknown diagnostic pragma: make it conditional v3: gcc < 11 doesn't know about it either, but it doesn't implement __has_warning udp.c | 4 ++++ util.h | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/udp.c b/udp.c index f425d14..98e3eaa 100644 --- a/udp.c +++ b/udp.c @@ -714,8 +714,10 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n, b->uh.len = htons(udp4_l2_mh_sock[n].msg_len + sizeof(b->uh)); if (c->mode == MODE_PASTA) { + PRAGMA_STRINGOP_OVERREAD_IGNORE if (write(c->fd_tap, &b->eh, sizeof(b->eh) + ip_len) < 0) debug("tap write: %s", strerror(errno)); + PRAGMA_STRINGOP_OVERREAD_IGNORE_POP pcap((char *)&b->eh, sizeof(b->eh) + ip_len); return; @@ -813,8 +815,10 @@ static void udp_sock_fill_data_v6(const struct ctx *c, int n, b->ip6h.hop_limit = 255; if (c->mode == MODE_PASTA) { + PRAGMA_STRINGOP_OVERREAD_IGNORE if (write(c->fd_tap, &b->eh, sizeof(b->eh) + ip_len) < 0) debug("tap write: %s", strerror(errno)); + PRAGMA_STRINGOP_OVERREAD_IGNORE_POP pcap((char *)&b->eh, sizeof(b->eh) + ip_len); return; diff --git a/util.h b/util.h index f85a87a..6f2c702 100644 --- a/util.h +++ b/util.h @@ -102,6 +102,29 @@ enum { (void *)(arg)); \ } while (0) + +#ifdef __has_warning +# if __has_warning("-Wstringop-overread") +# define PRAGMA_STRINGOP_OVERREAD_IGNORE \ + _Pragma("GCC diagnostic ignored \"-Wstringop-overread\"") +# define PRAGMA_STRINGOP_OVERREAD_IGNORE_POP \ + _Pragma("GCC diagnostic pop") +# else +# define PRAGMA_STRINGOP_OVERREAD_IGNORE +# define PRAGMA_STRINGOP_OVERREAD_IGNORE_POP +# endif +#else +# if defined(__GNUC__) && __GNUC__ >= 11 +# define PRAGMA_STRINGOP_OVERREAD_IGNORE \ + _Pragma("GCC diagnostic ignored \"-Wstringop-overread\"") +# define PRAGMA_STRINGOP_OVERREAD_IGNORE_POP \ + _Pragma("GCC diagnostic pop") +# else +# define PRAGMA_STRINGOP_OVERREAD_IGNORE +# define PRAGMA_STRINGOP_OVERREAD_IGNORE_POP +# endif +#endif + #if __BYTE_ORDER == __BIG_ENDIAN #define L2_BUF_ETH_IP4_INIT \ { \ -- 2.35.1

1 0