- dev - passt

migrate/bidirectional debugging
by David Gibson 08 Feb '25

08 Feb '25

I've spent today trying to debug this failure. I've gathered a bunch of information, but no breakthroughs, alas. At this point I suspect a kernel bug, though I hope I'm wrong. # Background. I think these are as you described it on your system: * Most (but not every) time I run migrate/bidirectional it fails, with the "outbound" stream only getting the before migration piece * I can't reproduce if I put strace on the guest 2 passt. Possibly unlike you: * I'm able to use TRACE=1, and the problem still reproduces * I can put strace on the outer pasta and the problem still reproduces The specific anomolies I was focused on were: * The passt_2 pcap shows "and from guest 2" coming _inbound_ a bit after it (correctly) went outbound * The pasta_1 pcap doesn't seem to show "and from guest 2" in either direction # Observations * I added a hack (see other series) that let me log comments to the pcap file as ethertype 0xffff, this was so I could have debugging messages in order with the the captured packets. * I used that to bin down exactly where the bogus output "and from guest 2" was being recorded, and it's in tcp_vu_data_from_sock() * I traced back from there, and passt_2 really does seem to be getting "and from guest 2" from a recvmsg() on the socket. I see from my pcap comments that we're getting 17 bytes from recvmsg() right before capturing the inbound packet, at any rate. * As noted, I couldn't reproduce with an strace on passt_2, so I couldn't confirm that piece that way It kind of seemed like we were sendmsg()ing "and from guest 2" and it was bouncing straight back to our socket, instead of being delivered to the outer pasta. * I tried putting a dumpcap on 'lo' in the pasta namespace, thinking I might see this weird passt->passt packet. But, nothing. There are thousands of packets of the qemu migration stream, and absolutely nothing else. * I also tried dumpcap on the external interface in the pasta namspace, and I didn't see anything different from what pasta captured (although I didn't check super carefully). In particular I didn't seem to see "and from guest 2" in either direction there either * Since I couldn't strace() passt_2, I instead tried logging TCP sendmsg() and recvmsg() calls of length 17 using systemtap (script attached). At this point it gets even weirder: On a working run (achieved by adding the strace), I get this: BEGIN tcp sendmsg(-129530279294592) len=17 - ./passt -s /tmp/passt-tests-niICXS/migrate/passt_2.socket -P /tmp/passt-tests-niICXS/migrate/passt_2.pid -f --vhost-user -p /home/dwg/src/passt/test/test_logs/passt_2.pcap --trace -t 10004 -u 10004 tcp sendmsg(-129489810388608) len=17 - ./pasta -p /home/dwg/src/passt/test/test_logs/pasta_1.pcap --trace --trace -l /tmp/pasta1.log -P /tmp/passt-tests-niICXS/migrate/pasta_1.pid -t 10001,10002,10004 -T 10003 -u 10001,10002,10004 -U 10003 --map-guest-addr 169.254.1.1 --config-net /home/dwg/src/passt/test/nstool hold /tmp/passt-tests-niICXS/migrate/ns1.hold END This mostly makes sense. passt_2 sends the expected outbound packet to the namespace, then pasta_1 forwards it on to the host. I don't know why I'm not seeing the recvmsg() from the socat server, though. In the failing case, though, I get this: BEGIN tcp sendmsg(-129471392995840) len=17 - ./passt -s /tmp/passt-tests-CV71zo/migrate/passt_2.socket -P /tmp/passt-tests-CV71zo/migrate/passt_2.pid -f --vhost-user -p /home/dwg/src/passt/test/test_logs/passt_2.pcap --trace -t 10004 -u 10004 tcp recvmsg(-129476447043584) len=17 - ./pasta -p /home/dwg/src/passt/test/test_logs/pasta_1.pcap --trace --trace -l /tmp/pasta1.log -P /tmp/passt-tests-CV71zo/migrate/pasta_1.pid -t 10001,10002,10004 -T 10003 -u 10001,10002,10004 -U 10003 --map-guest-addr 169.254.1.1 --config-net /home/dwg/src/passt/test/nstool hold /tmp/passt-tests-CV71zo/migrate/ns1.hold END First event seems the same: passt_2 sending the outbound packet, as expected. The second, though, is weird: the outer pasta seems to receive the data from a socket, not from tap as we'd expect. That might explain the other symptoms, if pasta received it on its socket, it would send inwards. But... I don't see pasta sending that "and from guest 2" inbound in its packet capture. And, weirder still, although I see that recvmsg() with systemtap, I don't see it in an strace of pasta. ...and.. that's where I'm at. Attaching my systemtap script and a ball of logs. Hoping they're helpful :/. -- David Gibson (he or they) | 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 3

[PATCH] passt-repair: Send one confirmation *per command*, not *per socket*
by Stefano Brivio 08 Feb '25

08 Feb '25

It looks like me, myself and I couldn't agree on the "simple" protocol between passt and passt-repair. The man page and passt say it's one confirmation per command, but the passt-repair implementation had one confirmation per socket instead. This caused all sort of mysterious issues with repair mode pseudo-randomly enabled, and leading to hours of fun (mostly not mine). Oops. Switch to one confirmation per command (of course). Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt-repair.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/passt-repair.c b/passt-repair.c index 322066a..614cee0 100644 --- a/passt-repair.c +++ b/passt-repair.c @@ -63,6 +63,7 @@ int main(int argc, char **argv) struct cmsghdr *cmsg; struct msghdr msg; struct iovec iov; + int op; prctl(PR_SET_DUMPABLE, 0); @@ -150,25 +151,24 @@ loop: _exit(1); } - for (i = 0; i < n; i++) { - int o = cmd; + op = cmd; - if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &o, sizeof(o))) { + for (i = 0; i < n; i++) { + if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, &op, sizeof(op))) { fprintf(stderr, - "Setting TCP_REPAIR to %i on socket %i: %s", o, + "Setting TCP_REPAIR to %i on socket %i: %s", op, fds[i], strerror(errno)); _exit(1); } /* Close _our_ copy */ close(fds[i]); + } - /* Confirm setting by echoing the command back */ - if (send(s, &cmd, sizeof(cmd), 0) < 0) { - fprintf(stderr, "Reply to command %i: %s\n", - o, strerror(errno)); - _exit(1); - } + /* Confirm setting by echoing the command back */ + if (send(s, &cmd, sizeof(cmd), 0) < 0) { + fprintf(stderr, "Reply to %i: %s\n", op, strerror(errno)); + _exit(1); } goto loop; -- 2.43.0

2 1

[PATCH v2] dhcp: Don't re-use request message for reply
by Enrique Llorente 07 Feb '25

07 Feb '25

The logic composing the DHCP reply message is reusing the request message to compose it, future long options like FQDN may exceed the request message limit making it go beyond the lower bound. This change creates a new reply message with a fixed options size of 308 and fills it in with proper fields from requests adding on top the generated options, this way the reply lower bound does not depend on the request. Signed-off-by: Enrique Llorente <ellorent(a)redhat.com> --- dhcp.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/dhcp.c b/dhcp.c index d8515aa..2a23ed4 100644 --- a/dhcp.c +++ b/dhcp.c @@ -151,9 +151,6 @@ static int fill(struct msg *m) { int i, o, offset = 0; - m->op = BOOTREPLY; - m->secs = 0; - for (o = 0; o < 255; o++) opts[o].sent = 0; @@ -291,8 +288,9 @@ int dhcp(const struct ctx *c, const struct pool *p) const struct ethhdr *eh; const struct iphdr *iph; const struct udphdr *uh; + struct msg const *m; + struct msg reply; unsigned int i; - struct msg *m; eh = packet_get(p, 0, offset, sizeof(*eh), NULL); offset += sizeof(*eh); @@ -321,6 +319,22 @@ int dhcp(const struct ctx *c, const struct pool *p) m->op != BOOTREQUEST) return -1; + reply.op = BOOTREPLY; + reply.htype = m->htype; + reply.hlen = m->hlen; + reply.hops = 0; + reply.xid = m->xid; + reply.secs = 0; + reply.flags = m->flags; + reply.ciaddr = m->ciaddr; + reply.yiaddr = c->ip4.addr; + reply.siaddr = 0; + reply.giaddr = m->giaddr; + memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr)); + memset(&reply.sname, 0, sizeof(reply.sname)); + memset(&reply.file, 0, sizeof(reply.file)); + reply.magic = m->magic; + offset += offsetof(struct msg, o); for (i = 0; i < ARRAY_SIZE(opts); i++) @@ -364,7 +378,6 @@ int dhcp(const struct ctx *c, const struct pool *p) info(" from %s", eth_ntop(m->chaddr, macstr, sizeof(macstr))); - m->yiaddr = c->ip4.addr; mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len)); memcpy(opts[1].s, &mask, sizeof(mask)); memcpy(opts[3].s, &c->ip4.guest_gw, sizeof(c->ip4.guest_gw)); @@ -401,14 +414,14 @@ int dhcp(const struct ctx *c, const struct pool *p) if (!c->no_dhcp_dns_search) opt_set_dns_search(c, sizeof(m->o)); - dlen = offsetof(struct msg, o) + fill(m); + dlen = offsetof(struct msg, o) + fill(&reply); if (m->flags & FLAG_BROADCAST) dst = in4addr_broadcast; else dst = c->ip4.addr; - tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen); + tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, &reply, dlen); return 1; } -- 2.47.0

2 1

[PATCH] passt-repair: Don't use perror(), accept ECONNRESET as termination
by Stefano Brivio 07 Feb '25

07 Feb '25

If we use glibc's perror(), we need to allow dup() and fcntl() in our seccomp profiles, which are a bit too much for this simple helper. On top of that, we would probably need a wrapper to avoid allocation for translated messages. While at it: ECONNRESET is just a close() from passt, treat it like EOF. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt-repair.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/passt-repair.c b/passt-repair.c index 3c3247b..d137a18 100644 --- a/passt-repair.c +++ b/passt-repair.c @@ -95,7 +95,7 @@ int main(int argc, char **argv) } if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) { - perror("Failed to create AF_UNIX socket"); + fprintf(stderr, "Failed to create AF_UNIX socket: %i\n", errno); _exit(1); } @@ -108,8 +108,12 @@ int main(int argc, char **argv) loop: ret = recvmsg(s, &msg, 0); if (ret < 0) { - perror("Failed to receive message"); - _exit(1); + if (errno == ECONNRESET) { + ret = 0; + } else { + fprintf(stderr, "Failed to read message: %i\n", errno); + _exit(1); + } } if (!ret) /* Done */ -- 2.43.0

2 4

[PATCH 0/2] Migration debugging hacks
by David Gibson 07 Feb '25

07 Feb '25

Here are a couple of hacky patches I was using for debugging the failures in the migrate/bidirectional test. Further details on where I'm at coming in another email. David Gibson (2): pcap comment hacks debug pcap.c | 20 ++++++++++++++++++++ pcap.h | 2 ++ tap.c | 1 + tcp_vu.c | 6 ++++++ test/lib/setup | 8 +++++--- test/migrate/bidirectional | 15 ++++++++++++++- vu_common.c | 1 + 7 files changed, 49 insertions(+), 4 deletions(-) -- 2.48.1

1 2

[PATCH] passt-repair: Dodge "structurally unreachable code" warning from Coverity
by Stefano Brivio 07 Feb '25

07 Feb '25

While main() conventionally returns int, and we need a return at the end of the function to avoid compiler warnings, turning that return into _exit() to avoid exit handlers triggers a Coverity warning. It's unreachable code anyway, so switch that single occurence back to a plain return. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt-repair.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/passt-repair.c b/passt-repair.c index 5ad5c9c..322066a 100644 --- a/passt-repair.c +++ b/passt-repair.c @@ -173,5 +173,5 @@ loop: goto loop; - _exit(0); + return 0; } -- 2.43.0

2 1

[PATCH v11 0/9] Draft state migration
by Stefano Brivio 07 Feb '25

07 Feb '25

v11: - drop SO_REUSEADDR setting in target: sockets closed in repair mode don't actually go to TIME_WAIT it seems - bind sockets to address too, and to the right flowside depending on whether they are for inbound or outbound connections - debug messages in flow_migrate_source() and flow_migrate_target() - error messages in tcp_flow_repair_queues() - set the same socket options (TCP_NODELAY, TCP_MAXSEG) just like for "regular" sockets, in tcp_flow_repair_socket() - connect() to HOSTFLOW(conn) flowside, not to TGTSIDE, in tcp_flow_repair_connect() - in test setup: set up guest_2 context in subshell - add migrate/bidirectional test David Gibson (4): fixup: Fix errors in modes that don't support migration migrate: Migrate guest observed addresses migrate: Hack for late migration fixups fixup: Reset SO_PEEK_OFF value after incoming migration Stefano Brivio (5): migrate: Skeleton of live migration logic Add interfaces and configuration bits for passt-repair vhost_user: Make source quit after reporting migration state migrate: Migrate TCP flows test: Add migration tests Makefile | 14 +- conf.c | 44 +++- epoll_type.h | 6 +- flow.c | 212 ++++++++++++++++ flow.h | 6 + migrate.c | 291 ++++++++++++++++++++++ migrate.h | 57 +++++ passt.1 | 11 + passt.c | 15 +- passt.h | 15 ++ repair.c | 192 +++++++++++++++ repair.h | 16 ++ tap.c | 65 +---- tcp.c | 483 +++++++++++++++++++++++++++++++++++++ tcp_conn.h | 60 +++++ test/lib/layout | 55 ++++- test/lib/setup | 128 ++++++++++ test/lib/test | 3 + test/migrate/basic | 54 +++++ test/migrate/bidirectional | 59 +++++ test/run | 7 + util.c | 62 +++++ util.h | 30 +++ vhost_user.c | 70 ++---- virtio.h | 4 - vu_common.c | 49 +--- vu_common.h | 2 +- 27 files changed, 1830 insertions(+), 180 deletions(-) create mode 100644 migrate.c create mode 100644 migrate.h create mode 100644 repair.c create mode 100644 repair.h create mode 100644 test/migrate/basic create mode 100644 test/migrate/bidirectional -- 2.43.0

1 9

[PATCH] passt-repair: Fix calculation of payload length from cmsg_len
by Stefano Brivio 07 Feb '25

07 Feb '25

There's no inverse function for CMSG_LEN(), so we need to loop over SCM_MAX_FD (253) possible input values. The previous calculation is clearly wrong, as not every int takes CMSG_LEN(sizeof(int)) in cmsg data. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- passt-repair.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/passt-repair.c b/passt-repair.c index d137a18..5ad5c9c 100644 --- a/passt-repair.c +++ b/passt-repair.c @@ -57,7 +57,7 @@ int main(int argc, char **argv) char buf[CMSG_SPACE(sizeof(int) * SCM_MAX_FD)] __attribute__ ((aligned(__alignof__(struct cmsghdr)))); struct sockaddr_un a = { AF_UNIX, "" }; - int fds[SCM_MAX_FD], s, ret, i, n; + int fds[SCM_MAX_FD], s, ret, i, n = 0; struct sock_fprog prog; int8_t cmd = INT8_MAX; struct cmsghdr *cmsg; @@ -127,7 +127,21 @@ loop: _exit(1); } - n = cmsg->cmsg_len / CMSG_LEN(sizeof(int)); + /* No inverse formula for CMSG_LEN(x), and building one with CMSG_LEN(0) + * works but there's no guarantee it does. Search the whole domain. + */ + for (i = 1; i < SCM_MAX_FD; i++) { + if (CMSG_LEN(sizeof(int) * i) == cmsg->cmsg_len) { + n = i; + break; + } + } + if (!n) { + fprintf(stderr, "Invalid ancillary data length %zu from peer\n", + cmsg->cmsg_len); + _exit(1); + } + memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n); if (cmd != TCP_REPAIR_ON && cmd != TCP_REPAIR_OFF && -- 2.43.0

1 0

[PATCH] conf, passt.1: Un-deprecate --host-lo-to-ns-lo
by Stefano Brivio 06 Feb '25

06 Feb '25

It was established behaviour, and it's now the third report about it: users ask how to achieve the same functionality, and we don't have a better answer yet. The idea behind declaring it deprecated to start with, I guess, was that we would eventually replace it by more flexible and generic configuration options, which is still planned. But there's nothing preventing us to alias this in the future to a particular configuration. So, stop scaring users off, and un-deprecate this. Link: https://archives.passt.top/passt-dev/20240925102009.62b9a0ce@elisabeth/ Link: https://github.com/rootless-containers/rootlesskit/pull/482#issuecomment-25… Link: https://github.com/moby/moby/issues/48838 Link: https://github.com/containers/podman/discussions/25243 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 3 +-- passt.1 | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/conf.c b/conf.c index dcfc1d6..db7950e 100644 --- a/conf.c +++ b/conf.c @@ -966,8 +966,7 @@ pasta_opts: " -U, --udp-ns SPEC UDP port forwarding to init namespace\n" " SPEC is as described above\n" " default: auto\n" - " --host-lo-to-ns-lo DEPRECATED:\n" - " Translate host-loopback forwards to\n" + " --host-lo-to-ns-lo Translate host-loopback forwards to\n" " namespace loopback\n" " --userns NSPATH Target user namespace to join\n" " --netns PATH|NAME Target network namespace to join\n" diff --git a/passt.1 b/passt.1 index 63a3a01..395c0c6 100644 --- a/passt.1 +++ b/passt.1 @@ -633,7 +633,7 @@ Configure UDP port forwarding from target namespace to init namespace. Default is \fBauto\fR. .TP -.BR \-\-host-lo-to-ns-lo " " (DEPRECATED) +.BR \-\-host-lo-to-ns-lo If specified, connections forwarded with \fB\-t\fR and \fB\-u\fR from the host's loopback address will appear on the loopback address in the guest as well. Without this option such forwarded packets will appear -- 2.43.0

2 1

[PATCH v10 00/10] Draft state migration
by David Gibson 06 Feb '25

06 Feb '25

Changes from v9: * Assorted minor cleanups * cppcheck and clang-tidy fixes * Set SO_PEEK_OFF properly on the target * Migrate addr_seen and friends Minor changes, I've just folded into the existing patches. More major things are separate patches. Those marked "fixup" I think should be folded into the previous patch before merge, but are separate now to make it clearer what I'm doing. David Gibson (5): debug: Add tcpdump to mbuto.img fixup: Fix errors in modes that don't support migration migrate: Migrate guest observed addresses migrate: Hack for late migration fixups fixup: Reset SO_PEEK_OFF value after incoming migration Stefano Brivio (5): migrate: Skeleton of live migration logic Add interfaces and configuration bits for passt-repair vhost_user: Make source quit after reporting migration state migrate: Migrate TCP flows test: Add migrate/basic tests Makefile | 14 +- conf.c | 44 ++++- epoll_type.h | 6 +- flow.c | 201 +++++++++++++++++++ flow.h | 6 + migrate.c | 291 ++++++++++++++++++++++++++++ migrate.h | 57 ++++++ passt.1 | 11 ++ passt.c | 15 +- passt.h | 15 ++ repair.c | 192 +++++++++++++++++++ repair.h | 16 ++ tap.c | 65 +------ tcp.c | 466 +++++++++++++++++++++++++++++++++++++++++++++ tcp_conn.h | 60 ++++++ test/lib/layout | 55 +++++- test/lib/setup | 127 ++++++++++++ test/lib/test | 3 + test/migrate/basic | 54 ++++++ test/passt.mbuto | 3 +- test/run | 4 + util.c | 62 ++++++ util.h | 30 +++ vhost_user.c | 70 ++----- virtio.h | 4 - vu_common.c | 49 +---- vu_common.h | 2 +- 27 files changed, 1741 insertions(+), 181 deletions(-) create mode 100644 migrate.c create mode 100644 migrate.h create mode 100644 repair.c create mode 100644 repair.h create mode 100644 test/migrate/basic -- 2.48.1

2 11