On Tue, Jan 28, 2025 at 12:15:26AM +0100, Stefano Brivio wrote:

That's the only field in flows with different storage sizes depending on the architecture: it's usually 4-byte wide on 32-bit architectures, except for arc and x32 where it's 8 bytes, and 8-byte wide on 64-bit machines.

As discussed on the call, I think there are broader problems with transferring timestamps than just the structure size. So I'm hoping we can work out how to not transfer them at all and avoid this change.

By keeping flow entries the same size across architectures, we avoid having to expand or shrink table entries upon migration.

Signed-off-by: Stefano Brivio --- icmp_flow.h | 6 +++++- udp_flow.h | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/icmp_flow.h b/icmp_flow.h index fb93801..da7e255 100644 --- a/icmp_flow.h +++ b/icmp_flow.h @@ -13,6 +13,7 @@ * @seq: Last sequence number sent to tap, host order, -1: not sent yet * @sock: "ping" socket * @ts: Last associated activity from tap, seconds + * @ts_storage: Pad @ts to 64-bit storage to keep state migration sane */ struct icmp_ping_flow { /* Must be first element */ @@ -20,7 +21,10 @@ struct icmp_ping_flow {

int seq; int sock; - time_t ts; + union { + time_t ts; + uint64_t ts_storage; + }; };

bool icmp_ping_timer(const struct ctx *c, const struct icmp_ping_flow *pingf, diff --git a/udp_flow.h b/udp_flow.h index 9a1b059..9cb79a0 100644 --- a/udp_flow.h +++ b/udp_flow.h @@ -12,6 +12,7 @@ * @f: Generic flow information * @closed: Flow is already closed * @ts: Activity timestamp + * @ts_storage: Pad @ts to 64-bit storage to keep state migration sane * @s: Socket fd (or -1) for each side of the flow */ struct udp_flow { @@ -19,7 +20,10 @@ struct udp_flow { struct flow_common f;

bool closed :1; - time_t ts; + union { + time_t ts; + uint64_t ts_storage; + }; int s[SIDES]; };

-- 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

On Tue, Jan 28, 2025 at 12:15:30AM +0100, Stefano Brivio wrote:

These are symmetric to write_remainder() and write_all_buf() and almost a copy and paste of them, with the most notable differences being reversed reads/writes and a couple of better-safe-than-sorry asserts to keep Coverity happy.

So, there's one thing that needs to be not quite symmetric for the read() version: we need to handle EOF. At present, I believe these will enter an infinite loop on EOF, which is not a graceful failure mode.

I'll use them in the next patch. At least for the moment, they're going to be used for vhost-user mode only, so I'm not unconditionally enabling readv() in the seccomp profile: the caller has to ensure it's there.

Signed-off-by: Stefano Brivio --- util.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 2 ++ 2 files changed, 72 insertions(+)

diff --git a/util.c b/util.c index 11973c4..085937b 100644 --- a/util.c +++ b/util.c @@ -606,6 +606,76 @@ int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip) return 0; }

+/** + * read_all_buf() - Fill a whole buffer from a file descriptor + * @fd: File descriptor + * @buf: Pointer to base of buffer + * @len: Length of buffer + * + * Return: 0 on success, -1 on error (with errno set) + * + * #syscalls read + */ +int read_all_buf(int fd, void *buf, size_t len) +{ + size_t left = len; + char *p = buf; + + while (left) { + ssize_t rc; + + ASSERT(left <= len); + + do + rc = read(fd, p, left); + while ((rc < 0) && errno == EINTR); + + if (rc < 0) + return -1; + + p += rc; + left -= rc; + } + return 0; +} + +/** + * read_remainder() - Read the tail of an IO vector from a file descriptor + * @fd: File descriptor + * @iov: IO vector + * @cnt: Number of entries in @iov + * @skip: Number of bytes of the vector to skip reading + * + * Return: 0 on success, -1 on error (with errno set) + * + * Note: mode-specific seccomp profiles need to enable readv() to use this. + */ +int read_remainder(int fd, struct iovec *iov, size_t cnt, size_t skip) +{ + size_t i = 0, offset; + + while ((i += iov_skip_bytes(iov + i, cnt - i, skip, &offset)) < cnt) { + ssize_t rc; + + if (offset) { + ASSERT(offset < iov[i].iov_len); + /* Read the remainder of the partially read buffer */ + if (read_all_buf(fd, (char *)iov[i].iov_base + offset, + iov[i].iov_len - offset) < 0) + return -1; + i++; + } + + /* Fill as many of the remaining buffers as we can */ + rc = readv(fd, &iov[i], cnt - i); + if (rc < 0) + return -1; + + skip = rc; + } + return 0; +} + /** sockaddr_ntop() - Convert a socket address to text format * @sa: Socket address * @dst: output buffer, minimum SOCKADDR_STRLEN bytes diff --git a/util.h b/util.h index d02333d..73a7a33 100644 --- a/util.h +++ b/util.h @@ -203,6 +203,8 @@ int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_all_buf(int fd, const void *buf, size_t len); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +int read_all_buf(int fd, void *buf, size_t len); +int read_remainder(int fd, struct iovec *iov, size_t cnt, size_t skip); void close_open_files(int argc, char **argv); bool snprintf_check(char *str, size_t size, const char *format, ...);

-- 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

On Tue, Jan 28, 2025 at 12:15:31AM +0100, Stefano Brivio wrote:

Add two sets (source or target) of three functions each for passt in vhost-user mode, triggered by activity on the file descriptor passed via VHOST_USER_PROTOCOL_F_DEVICE_STATE:

- migrate_source_pre() and migrate_target_pre() are called to prepare for migration, before data is transferred

- migrate_source() sends, and migrate_target() receives migration data

- migrate_source_post() and migrate_target_post() are responsible for any post-migration task

Callbacks are added to these functions with arrays of function pointers in migrate.c. Migration handlers are versioned.

Versioned descriptions of data sections will be added to the data_versions array, which points to versioned iovec arrays. Version 1 is currently empty and will be filled in in subsequent patches.

The source announces the data version to be used and informs the peer about endianness, and the size of void *, time_t, flow entries and flow hash table entries.

The target checks if the version of the source is still supported. If it's not, it aborts the migration.

Signed-off-by: Stefano Brivio --- Makefile | 12 +-- migrate.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++++++ migrate.h | 90 ++++++++++++++++++ passt.c | 2 +- vu_common.c | 122 ++++++++++++++++--------- vu_common.h | 2 +- 6 files changed, 438 insertions(+), 49 deletions(-) create mode 100644 migrate.c create mode 100644 migrate.h

diff --git a/Makefile b/Makefile index 464eef1..1383875 100644 --- a/Makefile +++ b/Makefile @@ -38,8 +38,8 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)

PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \ - ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \ - tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c tap.c \ + tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) @@ -48,10 +48,10 @@ MANPAGES = passt.1 pasta.1 qrap.1

PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \ flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h isolation.h \ - lineread.h log.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h \ - siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \ - tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \ - virtio.h vu_common.h + lineread.h log.h migrate.h ndp.h netlink.h packet.h passt.h pasta.h \ + pcap.h pif.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \ + tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \ + vhost_user.h virtio.h vu_common.h HEADERS = $(PASST_HEADERS) seccomp.h

C := \#include \nint main(){int a=getrandom(0, 0, 0);} diff --git a/migrate.c b/migrate.c new file mode 100644 index 0000000..bee9653 --- /dev/null +++ b/migrate.c @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * migrate.c - Migration sections, layout, and routines + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#include +#include + +#include "util.h" +#include "ip.h" +#include "passt.h" +#include "inany.h" +#include "flow.h" +#include "flow_table.h" + +#include "migrate.h" + +/* Current version of migration data */ +#define MIGRATE_VERSION 1 + +/* Magic as we see it and as seen with reverse endianness */ +#define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 +#define MIGRATE_MAGIC_SWAPPED 0xB0D1B10B1B1DBBB1

As noted, I'm hoping we can get rid of "either endian" migration. But if this stays, we should define it using __bswap_constant_32() to avoid embarrassing mistakes.

+ +/* Migration header to send from source */ +static union migrate_header header = { + .magic = MIGRATE_MAGIC, + .version = htonl_constant(MIGRATE_VERSION), + .time_t_size = htonl_constant(sizeof(time_t)), + .flow_size = htonl_constant(sizeof(union flow)), + .flow_sidx_size = htonl_constant(sizeof(struct flow_sidx)), + .voidp_size = htonl_constant(sizeof(void *)), +}; + +/* Data sections for version 1 */ +static struct iovec sections_v1[] = { + { &header, sizeof(header) }, +}; + +/* Set of data versions */ +static struct migrate_data data_versions[] = { + { + 1, sections_v1, + }, + { 0 }, +}; + +/* Handlers to call in source before sending data */ +struct migrate_handler handlers_source_pre[] = { + { 0 }, +}; + +/* Handlers to call in source after sending data */ +struct migrate_handler handlers_source_post[] = { + { 0 }, +}; + +/* Handlers to call in target before receiving data with version 1 */ +struct migrate_handler handlers_target_pre_v1[] = { + { 0 }, +}; + +/* Handlers to call in target after receiving data with version 1 */ +struct migrate_handler handlers_target_post_v1[] = { + { 0 }, +}; + +/* Versioned sets of migration handlers */ +struct migrate_target_handlers target_handlers[] = { + { + 1, + handlers_target_pre_v1, + handlers_target_post_v1, + }, + { 0 }, +}; + +/** + * migrate_source_pre() - Pre-migration tasks as source + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_source_pre(struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_pre; h->fn; h++) { + int rc; + + if ((rc = h->fn(m, h->data))) + return rc; + } + + return 0; +} + +/** + * migrate_source() - Perform migration as source: send state to hypervisor + * @fd: Descriptor for state transfer + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_source(int fd, const struct migrate_meta *m) +{ + static struct migrate_data *d; + unsigned count; + int rc; + + for (d = data_versions; d->v != MIGRATE_VERSION; d++);

Should ASSERT() if we don't find the version within the array.

+ for (count = 0; d->sections[count].iov_len; count++); + + debug("Writing %u migration sections", count - 1 /* minus header */); + rc = write_remainder(fd, d->sections, count, 0); + if (rc < 0) + return errno; + + return 0; +} + +/** + * migrate_source_post() - Post-migration tasks as source + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +void migrate_source_post(struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_post; h->fn; h++) + h->fn(m, h->data);

Is there actually anything we might need to do on the source after a successful migration, other than exit?

+} + +/** + * migrate_target_read_header() - Set metadata in target from source header + * @fd: Descriptor for state transfer + * @m: Migration metadata, filled on return + * + * Return: 0 on success, error code on failure

We nearly always use negative error codes. Why not here?

+ */ +int migrate_target_read_header(int fd, struct migrate_meta *m) +{ + static struct migrate_data *d; + union migrate_header h; + + if (read_all_buf(fd, &h, sizeof(h))) + return errno; + + debug("Source magic: 0x%016" PRIx64 ", sizeof(void *): %u, version: %u", + h.magic, ntohl(h.voidp_size), ntohl(h.version)); + + for (d = data_versions; d->v != ntohl(h.version); d++); + if (!d->v) + return ENOTSUP;

This is too late. The loop doesn't check it, so you've already overrun the data_versions table if the version wasn't in there. Easier to use an ARRAY_SIZE() limit in the loop, I think.

+ m->v = d->v; + + if (h.magic == MIGRATE_MAGIC) + m->bswap = false; + else if (h.magic == MIGRATE_MAGIC_SWAPPED) + m->bswap = true; + else + return ENOTSUP; + + if (ntohl(h.voidp_size) == 4) + m->source_64b = false; + else if (ntohl(h.voidp_size) == 8) + m->source_64b = true; + else + return ENOTSUP; + + if (ntohl(h.time_t_size) == 4) + m->time_64b = false; + else if (ntohl(h.time_t_size) == 8) + m->time_64b = true; + else + return ENOTSUP; + + m->flow_size = ntohl(h.flow_size); + m->flow_sidx_size = ntohl(h.flow_sidx_size); + + return 0; +} + +/** + * migrate_target_pre() - Pre-migration tasks as target + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_target_pre(struct migrate_meta *m) +{ + struct migrate_target_handlers *th; + struct migrate_handler *h; + + for (th = target_handlers; th->v != m->v && th->v; th++); + + for (h = th->pre; h->fn; h++) { + int rc; + + if ((rc = h->fn(m, h->data))) + return rc; + } + + return 0; +} + +/** + * migrate_target() - Perform migration as target: receive state from hypervisor + * @fd: Descriptor for state transfer + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + * + * #syscalls:vu readv + */ +int migrate_target(int fd, const struct migrate_meta *m) +{ + static struct migrate_data *d; + unsigned cnt; + int rc; + + for (d = data_versions; d->v != m->v && d->v; d++); + + for (cnt = 0; d->sections[cnt + 1 /* skip header */].iov_len; cnt++); + + debug("Reading %u migration sections", cnt); + rc = read_remainder(fd, d->sections + 1, cnt, 0); + if (rc < 0) + return errno; + + return 0; +} + +/** + * migrate_target_post() - Post-migration tasks as target + * @m: Migration metadata + */ +void migrate_target_post(struct migrate_meta *m) +{ + struct migrate_target_handlers *th; + struct migrate_handler *h; + + for (th = target_handlers; th->v != m->v && th->v; th++); + + for (h = th->post; h->fn; h++) + h->fn(m, h->data); +} diff --git a/migrate.h b/migrate.h new file mode 100644 index 0000000..5582f75 --- /dev/null +++ b/migrate.h @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef MIGRATE_H +#define MIGRATE_H + +/** + * struct migrate_meta - Migration metadata + * @v: Chosen migration data version, host order + * @bswap: Source has opposite endianness + * @peer_64b: Source uses 64-bit void * + * @time_64b: Source uses 64-bit time_t + * @flow_size: Size of union flow in source + * @flow_sidx_size: Size of struct flow_sidx in source + */ +struct migrate_meta { + uint32_t v; + bool bswap; + bool source_64b; + bool time_64b; + size_t flow_size; + size_t flow_sidx_size; +}; + +/** + * union migrate_header - Migration header from source + * @magic: 0xB1BB1D1B0BB1D1B0, host order + * @version: Source sends highest known, target aborts if unsupported + * @voidp_size: sizeof(void *), network order + * @time_t_size: sizeof(time_t), network order + * @flow_size: sizeof(union flow), network order + * @flow_sidx_size: sizeof(struct flow_sidx_t), network order + * @unused: Go figure + */ +union migrate_header { + struct { + uint64_t magic; + uint32_t version; + uint32_t voidp_size; + uint32_t time_t_size; + uint32_t flow_size; + uint32_t flow_sidx_size; + }; + uint8_t unused[65536];

So, having looked at this, I no longer think padding the header to 64kiB is a good idea. The structure means we're basically stuck always having that chunky header. Instead, I think the header should be absolutely minimal: basically magic and version only. v1 (and maybe others) can add a "metadata" or whatever section for additional information like this they need.

+}; + +/** + * struct migrate_data - Data sections for given source version + * @v: Source version this applies to, host order + * @sections: Array of data sections, NULL-terminated + */ +struct migrate_data { + uint32_t v; + struct iovec *sections; +}; + +/** + * struct migrate_handler - Function to handle a specific data section + * @fn: Function pointer taking pointer to data section + * @data: Associated data section + */ +struct migrate_handler { + int (*fn)(struct migrate_meta *m, void *data); + void *data; +}; + +/** + * struct migrate_target_handlers - Versioned sets of migration target handlers + * @v: Source version this applies to, host order + * @pre: Set of functions to execute in target before data copy + * @post: Set of functions to execute in target after data copy + */ +struct migrate_target_handlers { + uint32_t v; + struct migrate_handler *pre; + struct migrate_handler *post; +}; + +int migrate_source_pre(struct migrate_meta *m); +int migrate_source(int fd, const struct migrate_meta *m); +void migrate_source_post(struct migrate_meta *m); + +int migrate_target_read_header(int fd, struct migrate_meta *m); +int migrate_target_pre(struct migrate_meta *m); +int migrate_target(int fd, const struct migrate_meta *m); +void migrate_target_post(struct migrate_meta *m); + +#endif /* MIGRATE_H */ diff --git a/passt.c b/passt.c index b1c8ab6..184d4e5 100644 --- a/passt.c +++ b/passt.c @@ -358,7 +358,7 @@ loop: vu_kick_cb(c.vdev, ref, &now); break; case EPOLL_TYPE_VHOST_MIGRATION: - vu_migrate(c.vdev, eventmask); + vu_migrate(&c, eventmask); break; default: /* Can't happen */ diff --git a/vu_common.c b/vu_common.c index f43d8ac..0c67bd0 100644 --- a/vu_common.c +++ b/vu_common.c @@ -5,6 +5,7 @@ * common_vu.c - vhost-user common UDP and TCP functions */

+#include #include #include #include @@ -17,6 +18,7 @@ #include "vhost_user.h" #include "pcap.h" #include "vu_common.h" +#include "migrate.h"

#define VU_MAX_TX_BUFFER_NB 2

@@ -305,50 +307,88 @@ err: }

/** - * vu_migrate() - Send/receive passt insternal state to/from QEMU - * @vdev: vhost-user device + * vu_migrate_source() - Migration as source, send state to hypervisor + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_source(int fd) +{ + struct migrate_meta m; + int rc; + + if ((rc = migrate_source_pre(&m))) { + err("Source pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Saving backend state"); + + rc = migrate_source(fd, &m); + if (rc) + err("Source migration failed: %s", strerror_(rc)); + else + migrate_source_post(&m); + + return rc;

After a successful source migration shouldn't we exit, or at least quiesce ourselves so we don't accidentally mess with anything the target is now doing?

+} + +/** + * vu_migrate_target() - Migration as target, receive state from hypervisor + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_target(int fd) +{ + struct migrate_meta m; + int rc; + + rc = migrate_target_read_header(fd, &m); + if (rc) { + err("Migration header check failed: %s, abort", strerror_(rc)); + return rc; + } + + if ((rc = migrate_target_pre(&m))) { + err("Target pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Loading backend state"); + + rc = migrate_target(fd, &m); + if (rc) + err("Target migration failed: %s", strerror_(rc)); + else + migrate_target_post(&m); + + return rc; +} + +/** + * vu_migrate() - Send/receive passt internal state to/from QEMU + * @c: Execution context * @events: epoll events */ -void vu_migrate(struct vu_dev *vdev, uint32_t events) +void vu_migrate(struct ctx *c, uint32_t events) { - int ret; + struct vu_dev *vdev = c->vdev; + int rc = EIO;

- /* TODO: collect/set passt internal state - * and use vdev->device_state_fd to send/receive it - */ debug("vu_migrate fd %d events %x", vdev->device_state_fd, events); - if (events & EPOLLOUT) { - debug("Saving backend state"); - - /* send some stuff */ - ret = write(vdev->device_state_fd, "PASST", 6); - /* value to be returned by VHOST_USER_CHECK_DEVICE_STATE */ - vdev->device_state_result = ret == -1 ? -1 : 0; - /* Closing the file descriptor signals the end of transfer */ - epoll_ctl(vdev->context->epollfd, EPOLL_CTL_DEL, - vdev->device_state_fd, NULL); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; - } else if (events & EPOLLIN) { - char buf[6]; - - debug("Loading backend state"); - /* read some stuff */ - ret = read(vdev->device_state_fd, buf, sizeof(buf)); - /* value to be returned by VHOST_USER_CHECK_DEVICE_STATE */ - if (ret != sizeof(buf)) { - vdev->device_state_result = -1; - } else { - ret = strncmp(buf, "PASST", sizeof(buf)); - vdev->device_state_result = ret == 0 ? 0 : -1; - } - } else if (events & EPOLLHUP) { - debug("Closing migration channel"); - - /* The end of file signals the end of the transfer. */ - epoll_ctl(vdev->context->epollfd, EPOLL_CTL_DEL, - vdev->device_state_fd, NULL); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; - } + + if (events & EPOLLOUT) + rc = vu_migrate_source(vdev->device_state_fd); + else if (events & EPOLLIN) + rc = vu_migrate_target(vdev->device_state_fd); + + /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ + + vdev->device_state_result = rc; + + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, vdev->device_state_fd, NULL); + debug("Closing migration channel"); + close(vdev->device_state_fd); + vdev->device_state_fd = -1; } diff --git a/vu_common.h b/vu_common.h index d56c021..69c4006 100644 --- a/vu_common.h +++ b/vu_common.h @@ -57,5 +57,5 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq, void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, const struct timespec *now); int vu_send_single(const struct ctx *c, const void *buf, size_t size); -void vu_migrate(struct vu_dev *vdev, uint32_t events); +void vu_migrate(struct ctx *c, uint32_t events); #endif /* VU_COMMON_H */

-- 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

On Tue, Jan 28, 2025 at 07:50:01AM +0100, Stefano Brivio wrote:

On Tue, 28 Jan 2025 12:40:12 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 12:15:31AM +0100, Stefano Brivio wrote:

...

Add two sets (source or target) of three functions each for passt in vhost-user mode, triggered by activity on the file descriptor passed via VHOST_USER_PROTOCOL_F_DEVICE_STATE:

- migrate_source_pre() and migrate_target_pre() are called to prepare for migration, before data is transferred

- migrate_source() sends, and migrate_target() receives migration data

- migrate_source_post() and migrate_target_post() are responsible for any post-migration task

Callbacks are added to these functions with arrays of function pointers in migrate.c. Migration handlers are versioned.

Versioned descriptions of data sections will be added to the data_versions array, which points to versioned iovec arrays. Version 1 is currently empty and will be filled in in subsequent patches.

The source announces the data version to be used and informs the peer about endianness, and the size of void *, time_t, flow entries and flow hash table entries.

The target checks if the version of the source is still supported. If it's not, it aborts the migration.

Signed-off-by: Stefano Brivio --- Makefile | 12 +-- migrate.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++++++ migrate.h | 90 ++++++++++++++++++ passt.c | 2 +- vu_common.c | 122 ++++++++++++++++--------- vu_common.h | 2 +- 6 files changed, 438 insertions(+), 49 deletions(-) create mode 100644 migrate.c create mode 100644 migrate.h

diff --git a/Makefile b/Makefile index 464eef1..1383875 100644 --- a/Makefile +++ b/Makefile @@ -38,8 +38,8 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)

PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \ - ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \ - tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c tap.c \ + tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) @@ -48,10 +48,10 @@ MANPAGES = passt.1 pasta.1 qrap.1

PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \ flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h isolation.h \ - lineread.h log.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h \ - siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \ - tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \ - virtio.h vu_common.h + lineread.h log.h migrate.h ndp.h netlink.h packet.h passt.h pasta.h \ + pcap.h pif.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \ + tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \ + vhost_user.h virtio.h vu_common.h HEADERS = $(PASST_HEADERS) seccomp.h

C := \#include \nint main(){int a=getrandom(0, 0, 0);} diff --git a/migrate.c b/migrate.c new file mode 100644 index 0000000..bee9653 --- /dev/null +++ b/migrate.c @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * migrate.c - Migration sections, layout, and routines + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#include +#include + +#include "util.h" +#include "ip.h" +#include "passt.h" +#include "inany.h" +#include "flow.h" +#include "flow_table.h" + +#include "migrate.h" + +/* Current version of migration data */ +#define MIGRATE_VERSION 1 + +/* Magic as we see it and as seen with reverse endianness */ +#define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 +#define MIGRATE_MAGIC_SWAPPED 0xB0D1B10B1B1DBBB1

As noted, I'm hoping we can get rid of "either endian" migration. But if this stays, we should define it using __bswap_constant_32() to avoid embarrassing mistakes.

Those always give me issues on musl,

What sort of issues? We're already using them, and have fallback versions defined in util.h

so I'd rather test things on big-endian and realise it's actually 0xB0D1B1B01B1DBBB1 (0x0b bitswap).

Feel free to post a different proposal if tested.

...

...

+ +/* Migration header to send from source */ +static union migrate_header header = { + .magic = MIGRATE_MAGIC, + .version = htonl_constant(MIGRATE_VERSION), + .time_t_size = htonl_constant(sizeof(time_t)), + .flow_size = htonl_constant(sizeof(union flow)), + .flow_sidx_size = htonl_constant(sizeof(struct flow_sidx)), + .voidp_size = htonl_constant(sizeof(void *)), +}; + +/* Data sections for version 1 */ +static struct iovec sections_v1[] = { + { &header, sizeof(header) }, +}; + +/* Set of data versions */ +static struct migrate_data data_versions[] = { + { + 1, sections_v1, + }, + { 0 }, +}; + +/* Handlers to call in source before sending data */ +struct migrate_handler handlers_source_pre[] = { + { 0 }, +}; + +/* Handlers to call in source after sending data */ +struct migrate_handler handlers_source_post[] = { + { 0 }, +}; + +/* Handlers to call in target before receiving data with version 1 */ +struct migrate_handler handlers_target_pre_v1[] = { + { 0 }, +}; + +/* Handlers to call in target after receiving data with version 1 */ +struct migrate_handler handlers_target_post_v1[] = { + { 0 }, +}; + +/* Versioned sets of migration handlers */ +struct migrate_target_handlers target_handlers[] = { + { + 1, + handlers_target_pre_v1, + handlers_target_post_v1, + }, + { 0 }, +}; + +/** + * migrate_source_pre() - Pre-migration tasks as source + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_source_pre(struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_pre; h->fn; h++) { + int rc; + + if ((rc = h->fn(m, h->data))) + return rc; + } + + return 0; +} + +/** + * migrate_source() - Perform migration as source: send state to hypervisor + * @fd: Descriptor for state transfer + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_source(int fd, const struct migrate_meta *m) +{ + static struct migrate_data *d; + unsigned count; + int rc; + + for (d = data_versions; d->v != MIGRATE_VERSION; d++);

Should ASSERT() if we don't find the version within the array.

This looks a bit unnecessary, MIGRATE_VERSION is defined just above... it's just a readability killer to me.

...

...

+ for (count = 0; d->sections[count].iov_len; count++); + + debug("Writing %u migration sections", count - 1 /* minus header */); + rc = write_remainder(fd, d->sections, count, 0); + if (rc < 0) + return errno; + + return 0; +} + +/** + * migrate_source_post() - Post-migration tasks as source + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +void migrate_source_post(struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_post; h->fn; h++) + h->fn(m, h->data);

Is there actually anything we might need to do on the source after a successful migration, other than exit?

We might want to log a couple of things, which would warrant these handlers.

But let's say we need to do something *similar* to "updating the network" such as the RARP announcement that QEMU is requesting (this is

IIUC, that's on the target end, not the source end...

intended for OVN-Kubernetes, so go figure), or that we need a workaround for a kernel issue with implicit close() with TCP_REPAIR on... I would leave this in for completeness.

...but sure, point taken.

...

...

+} + +/** + * migrate_target_read_header() - Set metadata in target from source header + * @fd: Descriptor for state transfer + * @m: Migration metadata, filled on return + * + * Return: 0 on success, error code on failure

We nearly always use negative error codes. Why not here?

Because the reply to VHOST_USER_SET_DEVICE_STATE_FD is unsigned:

https://qemu-project.gitlab.io/qemu/interop/vhost-user.html#front-end-messag...

and I want to keep this consistent/untranslated.

Ok.

...

...

+ */ +int migrate_target_read_header(int fd, struct migrate_meta *m) +{ + static struct migrate_data *d; + union migrate_header h; + + if (read_all_buf(fd, &h, sizeof(h))) + return errno; + + debug("Source magic: 0x%016" PRIx64 ", sizeof(void *): %u, version: %u", + h.magic, ntohl(h.voidp_size), ntohl(h.version)); + + for (d = data_versions; d->v != ntohl(h.version); d++); + if (!d->v) + return ENOTSUP;

This is too late. The loop doesn't check it, so you've already overrun the data_versions table if the version wasn't in there.

Ah, yes, I forgot the '&& d->v' part (see migrate_target()).

...

Easier to use an ARRAY_SIZE() limit in the loop, I think.

I'd rather keep that as a one-liner, and NULL-terminate the arrays.

...

...

+ m->v = d->v; + + if (h.magic == MIGRATE_MAGIC) + m->bswap = false; + else if (h.magic == MIGRATE_MAGIC_SWAPPED) + m->bswap = true; + else + return ENOTSUP; + + if (ntohl(h.voidp_size) == 4) + m->source_64b = false; + else if (ntohl(h.voidp_size) == 8) + m->source_64b = true; + else + return ENOTSUP; + + if (ntohl(h.time_t_size) == 4) + m->time_64b = false; + else if (ntohl(h.time_t_size) == 8) + m->time_64b = true; + else + return ENOTSUP; + + m->flow_size = ntohl(h.flow_size); + m->flow_sidx_size = ntohl(h.flow_sidx_size); + + return 0; +} + +/** + * migrate_target_pre() - Pre-migration tasks as target + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_target_pre(struct migrate_meta *m) +{ + struct migrate_target_handlers *th; + struct migrate_handler *h; + + for (th = target_handlers; th->v != m->v && th->v; th++); + + for (h = th->pre; h->fn; h++) { + int rc; + + if ((rc = h->fn(m, h->data))) + return rc; + } + + return 0; +} + +/** + * migrate_target() - Perform migration as target: receive state from hypervisor + * @fd: Descriptor for state transfer + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + * + * #syscalls:vu readv + */ +int migrate_target(int fd, const struct migrate_meta *m) +{ + static struct migrate_data *d; + unsigned cnt; + int rc; + + for (d = data_versions; d->v != m->v && d->v; d++); + + for (cnt = 0; d->sections[cnt + 1 /* skip header */].iov_len; cnt++); + + debug("Reading %u migration sections", cnt); + rc = read_remainder(fd, d->sections + 1, cnt, 0); + if (rc < 0) + return errno; + + return 0; +} + +/** + * migrate_target_post() - Post-migration tasks as target + * @m: Migration metadata + */ +void migrate_target_post(struct migrate_meta *m) +{ + struct migrate_target_handlers *th; + struct migrate_handler *h; + + for (th = target_handlers; th->v != m->v && th->v; th++); + + for (h = th->post; h->fn; h++) + h->fn(m, h->data); +} diff --git a/migrate.h b/migrate.h new file mode 100644 index 0000000..5582f75 --- /dev/null +++ b/migrate.h @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef MIGRATE_H +#define MIGRATE_H + +/** + * struct migrate_meta - Migration metadata + * @v: Chosen migration data version, host order + * @bswap: Source has opposite endianness + * @peer_64b: Source uses 64-bit void * + * @time_64b: Source uses 64-bit time_t + * @flow_size: Size of union flow in source + * @flow_sidx_size: Size of struct flow_sidx in source + */ +struct migrate_meta { + uint32_t v; + bool bswap; + bool source_64b; + bool time_64b; + size_t flow_size; + size_t flow_sidx_size; +}; + +/** + * union migrate_header - Migration header from source + * @magic: 0xB1BB1D1B0BB1D1B0, host order + * @version: Source sends highest known, target aborts if unsupported + * @voidp_size: sizeof(void *), network order + * @time_t_size: sizeof(time_t), network order + * @flow_size: sizeof(union flow), network order + * @flow_sidx_size: sizeof(struct flow_sidx_t), network order + * @unused: Go figure + */ +union migrate_header { + struct { + uint64_t magic; + uint32_t version; + uint32_t voidp_size; + uint32_t time_t_size; + uint32_t flow_size; + uint32_t flow_sidx_size; + }; + uint8_t unused[65536];

So, having looked at this, I no longer think padding the header to 64kiB is a good idea. The structure means we're basically stuck always having that chunky header. Instead, I think the header should be absolutely minimal: basically magic and version only. v1 (and maybe others) can add a "metadata" or whatever section for additional information like this they need.

The header is processed by the target in a separate, preliminary step, though.

That's why I added metadata right in the header: if the target needs to abort the migration because, say, the size of a flow entry is too big to handle for a particular version, then we should know that before migrate_target_pre().

Ah, yes, I missed that, we'd need a more complex design to do additional transfers and checks before making the target_pre callbacks.

As long as we check the version first, we can always shrink the header later on.

*thinks*.. I guess so, though it's kind of awkward; a future version would have to read the "header of the header", check the version, then if it's the old one, read the remainder of the 64kiB block. I still think we should clearly separate the part that we're committing to being in every future version (which I think should just be magic and version), from the stuff that's just v1.

But having 64 KiB reserved looks more robust because it's a safe place to add this kind of metadata.

Note that 64 KiB is typically transferred in a single read/write from/to the vhost-user back-end.

Ok, but it also has to go over the qemu migration channel, which will often be a physical link, not a super-fast local/virtual one, and may be bandwidth capped as well. I'm not actually certain if 64kiB is likely to be a problem there, but it *is* large compared to the state blobs of most qemu devices (usually only a few hundred bytes).

...

...

+}; + +/** + * struct migrate_data - Data sections for given source version + * @v: Source version this applies to, host order + * @sections: Array of data sections, NULL-terminated + */ +struct migrate_data { + uint32_t v; + struct iovec *sections; +}; + +/** + * struct migrate_handler - Function to handle a specific data section + * @fn: Function pointer taking pointer to data section + * @data: Associated data section + */ +struct migrate_handler { + int (*fn)(struct migrate_meta *m, void *data); + void *data; +}; + +/** + * struct migrate_target_handlers - Versioned sets of migration target handlers + * @v: Source version this applies to, host order + * @pre: Set of functions to execute in target before data copy + * @post: Set of functions to execute in target after data copy + */ +struct migrate_target_handlers { + uint32_t v; + struct migrate_handler *pre; + struct migrate_handler *post; +}; + +int migrate_source_pre(struct migrate_meta *m); +int migrate_source(int fd, const struct migrate_meta *m); +void migrate_source_post(struct migrate_meta *m); + +int migrate_target_read_header(int fd, struct migrate_meta *m); +int migrate_target_pre(struct migrate_meta *m); +int migrate_target(int fd, const struct migrate_meta *m); +void migrate_target_post(struct migrate_meta *m); + +#endif /* MIGRATE_H */ diff --git a/passt.c b/passt.c index b1c8ab6..184d4e5 100644 --- a/passt.c +++ b/passt.c @@ -358,7 +358,7 @@ loop: vu_kick_cb(c.vdev, ref, &now); break; case EPOLL_TYPE_VHOST_MIGRATION: - vu_migrate(c.vdev, eventmask); + vu_migrate(&c, eventmask); break; default: /* Can't happen */ diff --git a/vu_common.c b/vu_common.c index f43d8ac..0c67bd0 100644 --- a/vu_common.c +++ b/vu_common.c @@ -5,6 +5,7 @@ * common_vu.c - vhost-user common UDP and TCP functions */

+#include #include #include #include @@ -17,6 +18,7 @@ #include "vhost_user.h" #include "pcap.h" #include "vu_common.h" +#include "migrate.h"

#define VU_MAX_TX_BUFFER_NB 2

@@ -305,50 +307,88 @@ err: }

/** - * vu_migrate() - Send/receive passt insternal state to/from QEMU - * @vdev: vhost-user device + * vu_migrate_source() - Migration as source, send state to hypervisor + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_source(int fd) +{ + struct migrate_meta m; + int rc; + + if ((rc = migrate_source_pre(&m))) { + err("Source pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Saving backend state"); + + rc = migrate_source(fd, &m); + if (rc) + err("Source migration failed: %s", strerror_(rc)); + else + migrate_source_post(&m); + + return rc;

After a successful source migration shouldn't we exit, or at least quiesce ourselves so we don't accidentally mess with anything the target is now doing?

Maybe, yes. Pending TCP connections should be safe because with TCP_REPAIR they're already quiesced, but we don't close listening sockets (yet).

Perhaps a reasonable approach for the moment would be to declare a single migrate_source_post handler logging a info() message and exiting.

Seems sensible for now.

...

...

+} + +/** + * vu_migrate_target() - Migration as target, receive state from hypervisor + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_target(int fd) +{ + struct migrate_meta m; + int rc; + + rc = migrate_target_read_header(fd, &m); + if (rc) { + err("Migration header check failed: %s, abort", strerror_(rc)); + return rc; + } + + if ((rc = migrate_target_pre(&m))) { + err("Target pre-migration failed: %s, abort", strerror_(rc)); + return rc; + } + + debug("Loading backend state"); + + rc = migrate_target(fd, &m); + if (rc) + err("Target migration failed: %s", strerror_(rc)); + else + migrate_target_post(&m); + + return rc; +} + +/** + * vu_migrate() - Send/receive passt internal state to/from QEMU + * @c: Execution context * @events: epoll events */ -void vu_migrate(struct vu_dev *vdev, uint32_t events) +void vu_migrate(struct ctx *c, uint32_t events) { - int ret; + struct vu_dev *vdev = c->vdev; + int rc = EIO;

- /* TODO: collect/set passt internal state - * and use vdev->device_state_fd to send/receive it - */ debug("vu_migrate fd %d events %x", vdev->device_state_fd, events); - if (events & EPOLLOUT) { - debug("Saving backend state"); - - /* send some stuff */ - ret = write(vdev->device_state_fd, "PASST", 6); - /* value to be returned by VHOST_USER_CHECK_DEVICE_STATE */ - vdev->device_state_result = ret == -1 ? -1 : 0; - /* Closing the file descriptor signals the end of transfer */ - epoll_ctl(vdev->context->epollfd, EPOLL_CTL_DEL, - vdev->device_state_fd, NULL); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; - } else if (events & EPOLLIN) { - char buf[6]; - - debug("Loading backend state"); - /* read some stuff */ - ret = read(vdev->device_state_fd, buf, sizeof(buf)); - /* value to be returned by VHOST_USER_CHECK_DEVICE_STATE */ - if (ret != sizeof(buf)) { - vdev->device_state_result = -1; - } else { - ret = strncmp(buf, "PASST", sizeof(buf)); - vdev->device_state_result = ret == 0 ? 0 : -1; - } - } else if (events & EPOLLHUP) { - debug("Closing migration channel"); - - /* The end of file signals the end of the transfer. */ - epoll_ctl(vdev->context->epollfd, EPOLL_CTL_DEL, - vdev->device_state_fd, NULL); - close(vdev->device_state_fd); - vdev->device_state_fd = -1; - } + + if (events & EPOLLOUT) + rc = vu_migrate_source(vdev->device_state_fd); + else if (events & EPOLLIN) + rc = vu_migrate_target(vdev->device_state_fd); + + /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ + + vdev->device_state_result = rc; + + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, vdev->device_state_fd, NULL); + debug("Closing migration channel"); + close(vdev->device_state_fd); + vdev->device_state_fd = -1; } diff --git a/vu_common.h b/vu_common.h index d56c021..69c4006 100644 --- a/vu_common.h +++ b/vu_common.h @@ -57,5 +57,5 @@ void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq, void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, const struct timespec *now); int vu_send_single(const struct ctx *c, const void *buf, size_t size); -void vu_migrate(struct vu_dev *vdev, uint32_t events); +void vu_migrate(struct ctx *c, uint32_t events); #endif /* VU_COMMON_H */

-- 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

On Wed, Jan 29, 2025 at 08:33:50AM +0100, Stefano Brivio wrote:

On Wed, 29 Jan 2025 12:16:58 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 07:50:01AM +0100, Stefano Brivio wrote:

...

On Tue, 28 Jan 2025 12:40:12 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 12:15:31AM +0100, Stefano Brivio wrote:

...

Add two sets (source or target) of three functions each for passt in vhost-user mode, triggered by activity on the file descriptor passed via VHOST_USER_PROTOCOL_F_DEVICE_STATE:

- migrate_source_pre() and migrate_target_pre() are called to prepare for migration, before data is transferred

- migrate_source() sends, and migrate_target() receives migration data

- migrate_source_post() and migrate_target_post() are responsible for any post-migration task

Callbacks are added to these functions with arrays of function pointers in migrate.c. Migration handlers are versioned.

Versioned descriptions of data sections will be added to the data_versions array, which points to versioned iovec arrays. Version 1 is currently empty and will be filled in in subsequent patches.

The source announces the data version to be used and informs the peer about endianness, and the size of void *, time_t, flow entries and flow hash table entries.

The target checks if the version of the source is still supported. If it's not, it aborts the migration.

Signed-off-by: Stefano Brivio --- Makefile | 12 +-- migrate.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++++++ migrate.h | 90 ++++++++++++++++++ passt.c | 2 +- vu_common.c | 122 ++++++++++++++++--------- vu_common.h | 2 +- 6 files changed, 438 insertions(+), 49 deletions(-) create mode 100644 migrate.c create mode 100644 migrate.h

diff --git a/Makefile b/Makefile index 464eef1..1383875 100644 --- a/Makefile +++ b/Makefile @@ -38,8 +38,8 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS)

PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \ - ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \ - tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c tap.c \ + tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) @@ -48,10 +48,10 @@ MANPAGES = passt.1 pasta.1 qrap.1

PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \ flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h isolation.h \ - lineread.h log.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h \ - siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \ - tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \ - virtio.h vu_common.h + lineread.h log.h migrate.h ndp.h netlink.h packet.h passt.h pasta.h \ + pcap.h pif.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \ + tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \ + vhost_user.h virtio.h vu_common.h HEADERS = $(PASST_HEADERS) seccomp.h

C := \#include \nint main(){int a=getrandom(0, 0, 0);} diff --git a/migrate.c b/migrate.c new file mode 100644 index 0000000..bee9653 --- /dev/null +++ b/migrate.c @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * PASTA - Pack A Subtle Tap Abstraction + * for network namespace/tap device mode + * + * migrate.c - Migration sections, layout, and routines + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#include +#include + +#include "util.h" +#include "ip.h" +#include "passt.h" +#include "inany.h" +#include "flow.h" +#include "flow_table.h" + +#include "migrate.h" + +/* Current version of migration data */ +#define MIGRATE_VERSION 1 + +/* Magic as we see it and as seen with reverse endianness */ +#define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 +#define MIGRATE_MAGIC_SWAPPED 0xB0D1B10B1B1DBBB1

As noted, I'm hoping we can get rid of "either endian" migration. But if this stays, we should define it using __bswap_constant_32() to avoid embarrassing mistakes.

Those always give me issues on musl,

What sort of issues? We're already using them, and have fallback versions defined in util.h

The very issues that brought me to introduce those fallback versions, so I'm instinctively reluctant to use them.

Actually, I think it's even clearer to have this spelt out (I always need to stop for a moment and think: what happens when I cross the 32-bit boundary?).

Oh, yes, we'd need to add a __bswap_constant_64() for this. [snip]

...

...

...

...

+/** + * union migrate_header - Migration header from source + * @magic: 0xB1BB1D1B0BB1D1B0, host order + * @version: Source sends highest known, target aborts if unsupported + * @voidp_size: sizeof(void *), network order + * @time_t_size: sizeof(time_t), network order + * @flow_size: sizeof(union flow), network order + * @flow_sidx_size: sizeof(struct flow_sidx_t), network order + * @unused: Go figure + */ +union migrate_header { + struct { + uint64_t magic; + uint32_t version; + uint32_t voidp_size; + uint32_t time_t_size; + uint32_t flow_size; + uint32_t flow_sidx_size; + }; + uint8_t unused[65536];

So, having looked at this, I no longer think padding the header to 64kiB is a good idea. The structure means we're basically stuck always having that chunky header. Instead, I think the header should be absolutely minimal: basically magic and version only. v1 (and maybe others) can add a "metadata" or whatever section for additional information like this they need.

The header is processed by the target in a separate, preliminary step, though.

That's why I added metadata right in the header: if the target needs to abort the migration because, say, the size of a flow entry is too big to handle for a particular version, then we should know that before migrate_target_pre().

Ah, yes, I missed that, we'd need a more complex design to do additional transfers and checks before making the target_pre callbacks.

...

As long as we check the version first, we can always shrink the header later on.

*thinks*.. I guess so, though it's kind of awkward; a future version would have to read the "header of the header", check the version, then if it's the old one, read the remainder of the 64kiB block.

I still think we should clearly separate the part that we're committing to being in every future version (which I think should just be magic and version), from the stuff that's just v1.

Sure, I can add a comment.

...

...

But having 64 KiB reserved looks more robust because it's a safe place to add this kind of metadata.

Note that 64 KiB is typically transferred in a single read/write from/to the vhost-user back-end.

Ok, but it also has to go over the qemu migration channel, which will often be a physical link, not a super-fast local/virtual one, and may be bandwidth capped as well. I'm not actually certain if 64kiB is likely to be a problem there, but it *is* large compared to the state blobs of most qemu devices (usually only a few hundred bytes).

Even if we transfer just what we need of a flow, it's still something well in excess of 50 bytes each. 100k flows would be 5 megs.

Just transferring the in-use flows would be higher priority than being selective about what we send within each flow. It's both easier to do and a bigger win in most cases. That would dramatically reduce the size sent here.

Well, anyway, let's cut this down to 4k, which should be enough, so that it's not a topic anymore.

I still think it's ugly, but whatever. -- 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

On Thu, Jan 30, 2025 at 05:55:22AM +0100, Stefano Brivio wrote:

On Thu, 30 Jan 2025 11:48:19 +1100 David Gibson wrote:

...

On Wed, Jan 29, 2025 at 08:33:50AM +0100, Stefano Brivio wrote:

...

On Wed, 29 Jan 2025 12:16:58 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 07:50:01AM +0100, Stefano Brivio wrote:

...

On Tue, 28 Jan 2025 12:40:12 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 12:15:31AM +0100, Stefano Brivio wrote: > Add two sets (source or target) of three functions each for passt in > vhost-user mode, triggered by activity on the file descriptor passed > via VHOST_USER_PROTOCOL_F_DEVICE_STATE: > > - migrate_source_pre() and migrate_target_pre() are called to prepare > for migration, before data is transferred > > - migrate_source() sends, and migrate_target() receives migration data > > - migrate_source_post() and migrate_target_post() are responsible for > any post-migration task > > Callbacks are added to these functions with arrays of function > pointers in migrate.c. Migration handlers are versioned. > > Versioned descriptions of data sections will be added to the > data_versions array, which points to versioned iovec arrays. Version > 1 is currently empty and will be filled in in subsequent patches. > > The source announces the data version to be used and informs the peer > about endianness, and the size of void *, time_t, flow entries and > flow hash table entries. > > The target checks if the version of the source is still supported. If > it's not, it aborts the migration. > > Signed-off-by: Stefano Brivio > --- > Makefile | 12 +-- > migrate.c | 259 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > migrate.h | 90 ++++++++++++++++++ > passt.c | 2 +- > vu_common.c | 122 ++++++++++++++++--------- > vu_common.h | 2 +- > 6 files changed, 438 insertions(+), 49 deletions(-) > create mode 100644 migrate.c > create mode 100644 migrate.h > > diff --git a/Makefile b/Makefile > index 464eef1..1383875 100644 > --- a/Makefile > +++ b/Makefile > @@ -38,8 +38,8 @@ FLAGS += -DDUAL_STACK_SOCKETS=$(DUAL_STACK_SOCKETS) > > PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ > icmp.c igmp.c inany.c iov.c ip.c isolation.c lineread.c log.c mld.c \ > - ndp.c netlink.c packet.c passt.c pasta.c pcap.c pif.c tap.c tcp.c \ > - tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ > + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c tap.c \ > + tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ > vhost_user.c virtio.c vu_common.c > QRAP_SRCS = qrap.c > SRCS = $(PASST_SRCS) $(QRAP_SRCS) > @@ -48,10 +48,10 @@ MANPAGES = passt.1 pasta.1 qrap.1 > > PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h fwd.h \ > flow_table.h icmp.h icmp_flow.h inany.h iov.h ip.h isolation.h \ > - lineread.h log.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h \ > - siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h tcp_splice.h \ > - tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h vhost_user.h \ > - virtio.h vu_common.h > + lineread.h log.h migrate.h ndp.h netlink.h packet.h passt.h pasta.h \ > + pcap.h pif.h siphash.h tap.h tcp.h tcp_buf.h tcp_conn.h tcp_internal.h \ > + tcp_splice.h tcp_vu.h udp.h udp_flow.h udp_internal.h udp_vu.h util.h \ > + vhost_user.h virtio.h vu_common.h > HEADERS = $(PASST_HEADERS) seccomp.h > > C := \#include \nint main(){int a=getrandom(0, 0, 0);} > diff --git a/migrate.c b/migrate.c > new file mode 100644 > index 0000000..bee9653 > --- /dev/null > +++ b/migrate.c > @@ -0,0 +1,259 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +/* PASST - Plug A Simple Socket Transport > + * for qemu/UNIX domain socket mode > + * > + * PASTA - Pack A Subtle Tap Abstraction > + * for network namespace/tap device mode > + * > + * migrate.c - Migration sections, layout, and routines > + * > + * Copyright (c) 2025 Red Hat GmbH > + * Author: Stefano Brivio > + */ > + > +#include > +#include > + > +#include "util.h" > +#include "ip.h" > +#include "passt.h" > +#include "inany.h" > +#include "flow.h" > +#include "flow_table.h" > + > +#include "migrate.h" > + > +/* Current version of migration data */ > +#define MIGRATE_VERSION 1 > + > +/* Magic as we see it and as seen with reverse endianness */ > +#define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 > +#define MIGRATE_MAGIC_SWAPPED 0xB0D1B10B1B1DBBB1

As noted, I'm hoping we can get rid of "either endian" migration. But if this stays, we should define it using __bswap_constant_32() to avoid embarrassing mistakes.

Those always give me issues on musl,

What sort of issues? We're already using them, and have fallback versions defined in util.h

The very issues that brought me to introduce those fallback versions, so I'm instinctively reluctant to use them.

Actually, I think it's even clearer to have this spelt out (I always need to stop for a moment and think: what happens when I cross the 32-bit boundary?).

Oh, yes, we'd need to add a __bswap_constant_64() for this.

...which doesn't exist on musl. On current Alpine Edge:

util.h:131:34: error: implicit declaration of function '__bswap_constant_64' [-Wimplicit-function-declaration] 131 | #define htonll_constant(x) (__bswap_constant_64(x)) | ^~~~~~~~~~~~~~~~~~~

...so rather than adding an implementation for this single usage, which makes it actually less clear to me, I would keep it like it is.

Very well.

...

[snip]

...

...

...

...

> +/** > + * union migrate_header - Migration header from source > + * @magic: 0xB1BB1D1B0BB1D1B0, host order > + * @version: Source sends highest known, target aborts if unsupported > + * @voidp_size: sizeof(void *), network order > + * @time_t_size: sizeof(time_t), network order > + * @flow_size: sizeof(union flow), network order > + * @flow_sidx_size: sizeof(struct flow_sidx_t), network order > + * @unused: Go figure > + */ > +union migrate_header { > + struct { > + uint64_t magic; > + uint32_t version; > + uint32_t voidp_size; > + uint32_t time_t_size; > + uint32_t flow_size; > + uint32_t flow_sidx_size; > + }; > + uint8_t unused[65536];

So, having looked at this, I no longer think padding the header to 64kiB is a good idea. The structure means we're basically stuck always having that chunky header. Instead, I think the header should be absolutely minimal: basically magic and version only. v1 (and maybe others) can add a "metadata" or whatever section for additional information like this they need.

The header is processed by the target in a separate, preliminary step, though.

That's why I added metadata right in the header: if the target needs to abort the migration because, say, the size of a flow entry is too big to handle for a particular version, then we should know that before migrate_target_pre().

Ah, yes, I missed that, we'd need a more complex design to do additional transfers and checks before making the target_pre callbacks.

...

As long as we check the version first, we can always shrink the header later on.

*thinks*.. I guess so, though it's kind of awkward; a future version would have to read the "header of the header", check the version, then if it's the old one, read the remainder of the 64kiB block.

I still think we should clearly separate the part that we're committing to being in every future version (which I think should just be magic and version), from the stuff that's just v1.

Sure, I can add a comment.

...

...

But having 64 KiB reserved looks more robust because it's a safe place to add this kind of metadata.

Note that 64 KiB is typically transferred in a single read/write from/to the vhost-user back-end.

Ok, but it also has to go over the qemu migration channel, which will often be a physical link, not a super-fast local/virtual one, and may be bandwidth capped as well. I'm not actually certain if 64kiB is likely to be a problem there, but it *is* large compared to the state blobs of most qemu devices (usually only a few hundred bytes).

Even if we transfer just what we need of a flow, it's still something well in excess of 50 bytes each. 100k flows would be 5 megs.

Just transferring the in-use flows would be higher priority than being selective about what we send within each flow.

Well, of course, I meant that we would only transfer used flows at that point, because it's not about transferring the flow table as a whole, with none of the advantages and disadvantages of it.

But still one can have 128k flows at the moment.

Right, but in the present draft you pay that cost whether or not you're actually using the flows. Unfortunately a busy server with heaps of active connections is exactly the case that's likely to be most sensitve to additional downtime, but there's not really any getting around that. A machine with a lot of state will need either high downtime or high migration bandwidth. But, I'm really hoping we can move relatively quickly to a model where a guest with only a handful of connections _doesn't_ have to pay that 128k flow cost - and can consequently migrate ok even with quite constrained migration bandwidth. In that scenario the size of the header could become significant.

...

It's both easier to do and a bigger win in most cases. That would dramatically reduce the size sent here.

Yep, feel free.

It's on my queue for the next few days.

...

...

Well, anyway, let's cut this down to 4k, which should be enough, so that it's not a topic anymore.

I still think it's ugly, but whatever.

Same here.

-- 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

On Tue, 28 Jan 2025 12:51:59 +1100 David Gibson wrote:

On Tue, Jan 28, 2025 at 12:15:32AM +0100, Stefano Brivio wrote:

...

A privileged helper to set/clear TCP_REPAIR on sockets on behalf of passt. Not used yet.

Signed-off-by: Stefano Brivio --- Makefile | 10 +++-- passt-repair.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 passt-repair.c

diff --git a/Makefile b/Makefile index 1383875..1b71cb0 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,8 @@ PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c -SRCS = $(PASST_SRCS) $(QRAP_SRCS) +PASST_REPAIR_SRCS = passt-repair.c +SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS)

MANPAGES = passt.1 pasta.1 qrap.1

@@ -72,9 +73,9 @@ mandir ?= $(datarootdir)/man man1dir ?= $(mandir)/man1

ifeq ($(TARGET_ARCH),x86_64) -BIN := passt passt.avx2 pasta pasta.avx2 qrap +BIN := passt passt.avx2 pasta pasta.avx2 qrap passt-repair else -BIN := passt pasta qrap +BIN := passt pasta qrap passt-repair endif

all: $(BIN) $(MANPAGES) docs @@ -101,6 +102,9 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% qrap: $(QRAP_SRCS) passt.h $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)

+passt-repair: $(PASST_REPAIR_SRCS) + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS) + valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ rt_sigreturn getpid gettid kill clock_gettime mmap \ mmap2 munmap open unlink gettimeofday futex statx \ diff --git a/passt-repair.c b/passt-repair.c new file mode 100644 index 0000000..e9b9609 --- /dev/null +++ b/passt-repair.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * passt-repair.c - Privileged helper to set/clear TCP_REPAIR on sockets + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + * + * Connect to passt via UNIX domain socket, receive sockets via SCM_RIGHTS along + * with commands mapping to TCP_REPAIR values, and switch repair mode on or + * off. Reply by echoing the command. Exit if the command is INT_MAX. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */ + +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 cmd, fds[SCM_MAX_FD], s, ret, i; + struct cmsghdr *cmsg; + struct msghdr msg; + struct iovec iov; + + iov = (struct iovec){ &cmd, sizeof(cmd) };

I mean, local to local, it's *probably* fine, but still a network protocol not defined in terms of explicit width fields makes me nervous. I'd prefer to see the cmd being a packed structure with fixed width elements.

It actually is, because: struct { int cmd; }; is a packet structure with fixed width elements. Any architecture we build for (at least the ones I'm aware of) has a 32-bit int. We can make it uint32_t if it makes you feel better.

I also think we should do some sort of basic magic / version exchange. I don't see any reason we'd need to extend the protocol, but I'd rather have the option if we have to.

passt-repair will be packaged and distributed together with passt, though. Versions must match. And latency here might matter more than in the rest of the migration process.

Plus checking a magic number should make things less damaging and more debuggable if you were to point the repair helper at an entirely unrelated unix socket instead of passt's repair socket.

Maybe, yes, even though I don't really see good chances for that mistake to happen. Feel free to post a proposal, of course.

...

+ msg = (struct msghdr){ NULL, 0, &iov, 1, buf, sizeof(buf), 0 }; + cmsg = CMSG_FIRSTHDR(&msg); + + if (argc != 2) { + fprintf(stderr, "Usage: %s PATH\n", argv[0]); + return -1; + } + + ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[1]); + if (ret <= 0 || ret >= (int)sizeof(a.sun_path)) { + fprintf(stderr, "Invalid socket path: %s\n", argv[1]); + return -1; + } + + if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0) {

Hmm.. would a datagram socket better suit our needs here?

We need a connection though, so that passt knows when the helper is ready to get messages. It could be done with a synchronisation datagram but it looks more complicated to handle. By the way, with a connection, we could probably just close() the socket here instead of having a "quit" command. If you're referring to the fact we don't keep message boundaries, so we would in theory need to add short read handling to the recvmsg() below: I'd rather switch cmd to a single byte instead. You can't transfer less than that.

...

+ perror("Failed to create AF_UNIX socket"); + return -1; + } + + if (connect(s, (struct sockaddr *)&a, sizeof(a))) { + fprintf(stderr, "Failed to connect to %s: %s\n", argv[1], + strerror(errno)); + return -1; + } + + while (1) { + int n; + + if (recvmsg(s, &msg, 0) < 0) { + perror("Failed to receive message"); + return -1; + } + + if (!cmsg || + cmsg->cmsg_len < CMSG_LEN(sizeof(int)) || + cmsg->cmsg_len > CMSG_LEN(sizeof(int) * SCM_MAX_FD) || + cmsg->cmsg_type != SCM_RIGHTS) + return -1; + + n = cmsg->cmsg_len / CMSG_LEN(sizeof(int)); + memcpy(fds, CMSG_DATA(cmsg), sizeof(int) * n); + + switch (cmd) { + case INT_MAX: + return 0; + case TCP_REPAIR_ON: + case TCP_REPAIR_OFF: + case TCP_REPAIR_OFF_NO_WP: + for (i = 0; i < n; i++) { + if (setsockopt(fds[i], SOL_TCP, TCP_REPAIR, + &cmd, sizeof(int))) { + perror("Setting TCP_REPAIR"); + return -1;

We probably want this to report errors back to passt, rather than just dying in this case. That way if for some weird reason one socket can't be placed in repair mode, we can still migrate all the other connections.

We implicitly report the error in the sense that we close the connection and passt will abort the migration. If you look at the handling of TCP_REPAIR in do_tcp_setsockopt(), you'll see that it either always fails (EPERM), or always succeeds. I mean, it's straightforward to implement, and we can just reply with a different command. But it's probably more meaningful and fitting to abort altogether. Besides, if we have to report exactly on which socket we failed, we won't be able to switch to a single-byte command protocol.

...

+ } + } + + /* Confirm setting by echoing the command back */ + if (send(s, &cmd, sizeof(int), 0) < 0) { + fprintf(stderr, "Reply to command %i: %s\n", + cmd, strerror(errno)); + return -1; + } + + break; + default: + fprintf(stderr, "Unsupported command 0x%04x\n", cmd); + return -1; + } + } +}

-- Stefano

On Wed, 29 Jan 2025 12:29:27 +1100 David Gibson wrote:

On Tue, Jan 28, 2025 at 07:51:31AM +0100, Stefano Brivio wrote:

...

On Tue, 28 Jan 2025 12:51:59 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 12:15:32AM +0100, Stefano Brivio wrote:

...

A privileged helper to set/clear TCP_REPAIR on sockets on behalf of passt. Not used yet.

Signed-off-by: Stefano Brivio --- Makefile | 10 +++-- passt-repair.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 passt-repair.c

diff --git a/Makefile b/Makefile index 1383875..1b71cb0 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,8 @@ PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c -SRCS = $(PASST_SRCS) $(QRAP_SRCS) +PASST_REPAIR_SRCS = passt-repair.c +SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS)

MANPAGES = passt.1 pasta.1 qrap.1

@@ -72,9 +73,9 @@ mandir ?= $(datarootdir)/man man1dir ?= $(mandir)/man1

ifeq ($(TARGET_ARCH),x86_64) -BIN := passt passt.avx2 pasta pasta.avx2 qrap +BIN := passt passt.avx2 pasta pasta.avx2 qrap passt-repair else -BIN := passt pasta qrap +BIN := passt pasta qrap passt-repair endif

all: $(BIN) $(MANPAGES) docs @@ -101,6 +102,9 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% qrap: $(QRAP_SRCS) passt.h $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)

+passt-repair: $(PASST_REPAIR_SRCS) + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS) + valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ rt_sigreturn getpid gettid kill clock_gettime mmap \ mmap2 munmap open unlink gettimeofday futex statx \ diff --git a/passt-repair.c b/passt-repair.c new file mode 100644 index 0000000..e9b9609 --- /dev/null +++ b/passt-repair.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * passt-repair.c - Privileged helper to set/clear TCP_REPAIR on sockets + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + * + * Connect to passt via UNIX domain socket, receive sockets via SCM_RIGHTS along + * with commands mapping to TCP_REPAIR values, and switch repair mode on or + * off. Reply by echoing the command. Exit if the command is INT_MAX. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */ + +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 cmd, fds[SCM_MAX_FD], s, ret, i; + struct cmsghdr *cmsg; + struct msghdr msg; + struct iovec iov; + + iov = (struct iovec){ &cmd, sizeof(cmd) };

I mean, local to local, it's *probably* fine, but still a network protocol not defined in terms of explicit width fields makes me nervous. I'd prefer to see the cmd being a packed structure with fixed width elements.

It actually is, because:

struct { int cmd; };

is a packet structure with fixed width elements. Any architecture we build for (at least the ones I'm aware of) has a 32-bit int. We can make it uint32_t if it makes you feel better.

Sorry, I should have said "*explicitly* fixed width fields". So, yes, uint32_t would make me feel better :)

Changed to int8_t anyway meanwhile. We don't need all those bits.

...

...

I also think we should do some sort of basic magic / version exchange. I don't see any reason we'd need to extend the protocol, but I'd rather have the option if we have to.

passt-repair will be packaged and distributed together with passt, though. Versions must match.

But nothing enforces that.

Distribution packages. If I run claws-mail with the wrong version of, say, libpixman, it won't start. If you don't use them, you're on your own.

AIUI KubeVirt will be running passt-repair in a different context. Which means it may well be deployed by a different path than the passt binary

No, that's not the way it works. It needs to match, in the sense that 1. it's a KubeVirt requirement to have compatible packages between distribution and the "base container image" and 2. this would most likely be sourced from the "base container image" anyway. I maintain the packages for four distributions, plus AppArmor and SELinux policies upstream and downstream, and I take care of updating the package in KubeVirt as well, so I guess I have a vague idea of what's convenient, enforced, burdensome, and so on.

which means however we distribute it's quite plausible that a downstream screwup could mismatch the versions. We should endeavour to have a reasonably graceful failure mode for that.

Regardless of this, I think that *this one* is an interface (I wouldn't even call it a protocol) that needs to be set in stone, except for hypothetical (and highly unlikely) UAPI additions which we'll be anyway able to accommodate for easily. It's a single socket option with three possible values (for 13 years now), of which we plan to use two. If we want this interface to do anything else, it should be another interface. So there's really no problem with this. Besides, the helper runs with CAP_NET_ADMIN (even though CAP_NET_RAW should ideally suffice), so it needs to be extremely simple and auditable.

...

And latency here might matter more than in the rest of the migration process.

...

...

Plus checking a magic number should make things less damaging and more debuggable if you were to point the repair helper at an entirely unrelated unix socket instead of passt's repair socket.

Maybe, yes, even though I don't really see good chances for that mistake to happen. Feel free to post a proposal, of course.

I disagree on the good chances for a mistake thing. In GSS I saw plenty of occasions where things that shouldn't be mismatched were due to some packaging or user screwup. And that's before even considering the way that KubeVirt deploys its various pieces seems to provide a number of opportunities to mess this up.

So, I'll see what I can come up with. I'm fine with requiring matching versions if it's actually checked. Maybe a magic derived from our git hash, or even our build-id.

Both would make things significantly less usable, because they would make different but compatible builds incompatible, and different implementations rather inconvenient. For example, it might be a practical solution to have a Go implementation of this in KubeVirt's virt-handler itself, but if it needs to extract information or strings from the binary, that becomes impractical. -- Stefano

On Thu, 30 Jan 2025 11:53:08 +1100 David Gibson wrote:

On Wed, Jan 29, 2025 at 08:04:28AM +0100, Stefano Brivio wrote:

...

On Wed, 29 Jan 2025 12:29:27 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 07:51:31AM +0100, Stefano Brivio wrote:

...

On Tue, 28 Jan 2025 12:51:59 +1100 David Gibson wrote:

...

On Tue, Jan 28, 2025 at 12:15:32AM +0100, Stefano Brivio wrote:

...

A privileged helper to set/clear TCP_REPAIR on sockets on behalf of passt. Not used yet.

Signed-off-by: Stefano Brivio --- Makefile | 10 +++-- passt-repair.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 passt-repair.c

diff --git a/Makefile b/Makefile index 1383875..1b71cb0 100644 --- a/Makefile +++ b/Makefile @@ -42,7 +42,8 @@ PASST_SRCS = arch.c arp.c checksum.c conf.c dhcp.c dhcpv6.c flow.c fwd.c \ tcp.c tcp_buf.c tcp_splice.c tcp_vu.c udp.c udp_flow.c udp_vu.c util.c \ vhost_user.c virtio.c vu_common.c QRAP_SRCS = qrap.c -SRCS = $(PASST_SRCS) $(QRAP_SRCS) +PASST_REPAIR_SRCS = passt-repair.c +SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS)

MANPAGES = passt.1 pasta.1 qrap.1

@@ -72,9 +73,9 @@ mandir ?= $(datarootdir)/man man1dir ?= $(mandir)/man1

ifeq ($(TARGET_ARCH),x86_64) -BIN := passt passt.avx2 pasta pasta.avx2 qrap +BIN := passt passt.avx2 pasta pasta.avx2 qrap passt-repair else -BIN := passt pasta qrap +BIN := passt pasta qrap passt-repair endif

all: $(BIN) $(MANPAGES) docs @@ -101,6 +102,9 @@ pasta.avx2 pasta.1 pasta: pasta%: passt% qrap: $(QRAP_SRCS) passt.h $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DARCH=\"$(TARGET_ARCH)\" $(QRAP_SRCS) -o qrap $(LDFLAGS)

+passt-repair: $(PASST_REPAIR_SRCS) + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS) + valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ rt_sigreturn getpid gettid kill clock_gettime mmap \ mmap2 munmap open unlink gettimeofday futex statx \ diff --git a/passt-repair.c b/passt-repair.c new file mode 100644 index 0000000..e9b9609 --- /dev/null +++ b/passt-repair.c @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +/* PASST - Plug A Simple Socket Transport + * for qemu/UNIX domain socket mode + * + * passt-repair.c - Privileged helper to set/clear TCP_REPAIR on sockets + * + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + * + * Connect to passt via UNIX domain socket, receive sockets via SCM_RIGHTS along + * with commands mapping to TCP_REPAIR values, and switch repair mode on or + * off. Reply by echoing the command. Exit if the command is INT_MAX. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */ + +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 cmd, fds[SCM_MAX_FD], s, ret, i; + struct cmsghdr *cmsg; + struct msghdr msg; + struct iovec iov; + + iov = (struct iovec){ &cmd, sizeof(cmd) };

I mean, local to local, it's *probably* fine, but still a network protocol not defined in terms of explicit width fields makes me nervous. I'd prefer to see the cmd being a packed structure with fixed width elements.

It actually is, because:

struct { int cmd; };

is a packet structure with fixed width elements. Any architecture we build for (at least the ones I'm aware of) has a 32-bit int. We can make it uint32_t if it makes you feel better.

Sorry, I should have said "*explicitly* fixed width fields". So, yes, uint32_t would make me feel better :)

Changed to int8_t anyway meanwhile. We don't need all those bits.

Works or me.

...

...

...

...

I also think we should do some sort of basic magic / version exchange. I don't see any reason we'd need to extend the protocol, but I'd rather have the option if we have to.

passt-repair will be packaged and distributed together with passt, though. Versions must match.

But nothing enforces that.

Distribution packages. If I run claws-mail with the wrong version of, say, libpixman, it won't start. If you don't use them, you're on your own.

But shared libraries *do* have versioning checks: there are defined compatibility semantics for sonames, and there can be symbol versions as well.

...

...

AIUI KubeVirt will be running passt-repair in a different context. Which means it may well be deployed by a different path than the passt binary

No, that's not the way it works. It needs to match, in the sense that 1. it's a KubeVirt requirement to have compatible packages between distribution and the "base container image" and 2. this would most likely be sourced from the "base container image" anyway.

I maintain the packages for four distributions, plus AppArmor and SELinux policies upstream and downstream, and I take care of updating the package in KubeVirt as well, so I guess I have a vague idea of what's convenient, enforced, burdensome, and so on.

...

which means however we distribute it's quite plausible that a downstream screwup could mismatch the versions. We should endeavour to have a reasonably graceful failure mode for that.

Regardless of this, I think that *this one* is an interface (I wouldn't even call it a protocol) that needs to be set in stone, except for hypothetical (and highly unlikely) UAPI additions which we'll be anyway able to accommodate for easily.

Ok, I can buy that, but it's a contradictory position to "versions must match".

Note: "Regardless of this". It's *another* consideration *on top of that*. 1. Versions (builds) match. 2. And even if they didn't, it wouldn't be a problem, because this interface will not change.

...

It's a single socket option with three possible values (for 13 years now), of which we plan to use two. If we want this interface to do anything else, it should be another interface.

So there's really no problem with this.

Besides, the helper runs with CAP_NET_ADMIN (even though CAP_NET_RAW should ideally suffice), so it needs to be extremely simple and auditable.

Sending and checking a magic number is not a lot of complexity, even in something on this scale.

If you want to have multiple bytes (because I'm forecasting that you won't be happy with 255 values), it's substantial complexity in comparison to the current implementation.

...

...

...

And latency here might matter more than in the rest of the migration process.

...

...

Plus checking a magic number should make things less damaging and more debuggable if you were to point the repair helper at an entirely unrelated unix socket instead of passt's repair socket.

Maybe, yes, even though I don't really see good chances for that mistake to happen. Feel free to post a proposal, of course.

I disagree on the good chances for a mistake thing. In GSS I saw plenty of occasions where things that shouldn't be mismatched were due to some packaging or user screwup. And that's before even considering the way that KubeVirt deploys its various pieces seems to provide a number of opportunities to mess this up.

So, I'll see what I can come up with. I'm fine with requiring matching versions if it's actually checked. Maybe a magic derived from our git hash, or even our build-id.

Both would make things significantly less usable, because they would make different but compatible builds incompatible, and different implementations rather inconvenient.

Ok, so you're definitely now saying versions *don't* need to match.

They don't need to, no. They will match, but they don't need to.

...

For example, it might be a practical solution to have a Go implementation of this in KubeVirt's virt-handler itself, but if it needs to extract information or strings from the binary, that becomes impractical.

Ok... could we at least add just a magic number then. If we do ever need a new protocol we can change it, otherwise the protocol immutable for now.

Adding a non-byte magic number implies handling short reads and short writes, which I think is absolutely unnecessary. Feel free to propose an implementation, as usual. If you are happy with a single byte magic number, then I suppose that, given that we're just using three values, we could encode that information using a combination of the remaining bits, which has the advantage of not needing any specific implementation until it's actually needed (never, I suppose), because passt-repair already terminates on an unknown command value. -- Stefano