On Wed, Jan 29, 2025 at 12:39:33AM +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.

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

Ugly, but necessary for this v1 approach.

--- 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 Wed, Jan 29, 2025 at 12:39:34AM +0100, Stefano Brivio wrote:

...to keep migration sane. Right now, the biggest struct in union flow is struct tcp_splice_conn with 120 bytes on x86_64, which should also have the biggest storage and alignment requirements of any architecture we might run on.

Signed-off-by: Stefano Brivio

Again, ugly, but necessary for the v1 approach.

--- flow.h | 18 ++++++++++++------ flow_table.h | 13 ++++++++++--- 2 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/flow.h b/flow.h index 24ba3ef..8eb5964 100644 --- a/flow.h +++ b/flow.h @@ -202,15 +202,21 @@ struct flow_common {

/** * struct flow_sidx - ID for one side of a specific flow - * @sidei: Index of side referenced (0 or 1) - * @flowi: Index of flow referenced + * @sidei: Index of side referenced (0 or 1) + * @flowi: Index of flow referenced + * @flow_sidx_storage: Pad to 32 bits */ typedef struct flow_sidx { - unsigned sidei :1; - unsigned flowi :FLOW_INDEX_BITS; + union { + struct { + unsigned sidei :1; + unsigned flowi :FLOW_INDEX_BITS; + }; + uint32_t flow_sidx_storage; + }; } flow_sidx_t; -static_assert(sizeof(flow_sidx_t) <= sizeof(uint32_t), - "flow_sidx_t must fit within 32 bits"); +static_assert(sizeof(flow_sidx_t) == sizeof(uint32_t), + "flow_sidx_t must be 32-bit wide");

#define FLOW_SIDX_NONE ((flow_sidx_t){ .flowi = FLOW_MAX })

diff --git a/flow_table.h b/flow_table.h index f15db53..007f4dd 100644 --- a/flow_table.h +++ b/flow_table.h @@ -26,9 +26,13 @@ struct flow_free_cluster {

/** * union flow - Descriptor for a logical packet flow (e.g. connection) - * @f: Fields common between all variants - * @tcp: Fields for non-spliced TCP connections - * @tcp_splice: Fields for spliced TCP connections + * @f: Fields common between all variants + * @free: Entry in a cluster of free entries + * @tcp: Fields for non-spliced TCP connections + * @tcp_splice: Fields for spliced TCP connections + * @ping: Tracking for ping flows + * @udp: Tracking for UDP flows + * @flow_storage: Pad flow entries to 128 bytes to ease state migration */ union flow { struct flow_common f; @@ -37,8 +41,11 @@ union flow { struct tcp_splice_conn tcp_splice; struct icmp_ping_flow ping; struct udp_flow udp; + char flow_storage[128]; };

+static_assert(sizeof(union flow) == 128, "union flow should be 128-byte wide"); + /* Global Flow Table */ extern unsigned flow_first_free; extern union flow flowtab[];

-- 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, 29 Jan 2025 16:41:01 +1100 David Gibson wrote:

On Wed, Jan 29, 2025 at 12:39:37AM +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

Tomorrow, I'm planning to try implementing a bunch of the suggestions I have below.

...

--- Makefile | 12 +-- migrate.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++++++++ migrate.h | 88 ++++++++++++++++++ passt.c | 2 +- vu_common.c | 124 ++++++++++++++++-------- vu_common.h | 2 +- 6 files changed, 443 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..b8b79e0 --- /dev/null +++ b/migrate.c @@ -0,0 +1,264 @@ +// 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 0xB0D1B1B01B1DBBB1 + +/* 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) },

This format assumes that everything we send is in buffers with statically known size and address. I'm pretty sure we'll outgrow that quickly. For one thing, I realised we want to transfer addr*_seen, or we won't know how to direct an incoming connection after migration, if that occurs before the guest sends anything outgoing.

Yes, I'm transferring those too (local changes not completely working yet) as separate sections. The layout is just fine for those. What we don't cover with this layout is arrays, and I guess that should be added, because if we want to transfer a minimal representation of single flows instead of the whole flow table, we need something like that. I think it could be something on top of iovecs, that is, a simple structure with iovec pointers and a count, or a pointer to a count.

...

+}; + +/* Set of data versions */ +static struct migrate_data data_versions[] = { + { + 1, sections_v1, + },

I realise it's a bit less "declarative", but maybe just a "receive" function for each version which open codes sending each section would be more flexible, and not that much more verbose.

It's 18 lines of migrate_source() and 17 lines of migrate_target() right now, so it's not really *that* short, and if it can be declarative (I'm finding this quite convenient as I'm *using* it), I think it should be. What else would you like to add? Besides, it can always be changed later as a use case appears.

...

+ { 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 }, +};

I also wonder if these tables of handlers are overkill, rather than just having a function that calls all the things in the right order.

That risks turning into a long series of: ret = handler(c); if (ret) return ret; ... but anyway, let me finish writing the code using this first, then we'll know, instead of speculating.

...

+ +/* 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 + * @c: Execution context + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_source_pre(struct ctx *c, struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_pre; h->fn; h++) { + int rc; + + if ((rc = h->fn(c, m))) + 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; + int count, rc; + + (void)m; + + for (d = data_versions; d->v != MIGRATE_VERSION; d++); + + 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 + * @c: Execution context + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +void migrate_source_post(struct ctx *c, struct migrate_meta *m) +{ + struct migrate_handler *h; + + for (h = handlers_source_post; h->fn; h++) + h->fn(c, m); +} + +/** + * 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 + */ +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->v; d++); + if (!d->v) + return ENOTSUP; + 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 + * @c: Execution context + * @m: Migration metadata + * + * Return: 0 on success, error code on failure + */ +int migrate_target_pre(struct ctx *c, 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(c, m))) + 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 + * @c: Execution context + * @m: Migration metadata + */ +void migrate_target_post(struct ctx *c, 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(c, m); +} diff --git a/migrate.h b/migrate.h new file mode 100644 index 0000000..f9635ac --- /dev/null +++ b/migrate.h @@ -0,0 +1,88 @@ +/* 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]; +}; + +/** + * 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 context and metadata + */ +struct migrate_handler { + int (*fn)(struct ctx *c, struct migrate_meta *m); +}; + +/** + * 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 ctx *c, struct migrate_meta *m); +int migrate_source(int fd, const struct migrate_meta *m); +void migrate_source_post(struct ctx *c, struct migrate_meta *m); + +int migrate_target_read_header(int fd, struct migrate_meta *m); +int migrate_target_pre(struct ctx *c, struct migrate_meta *m); +int migrate_target(int fd, const struct migrate_meta *m); +void migrate_target_post(struct ctx *c, 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);

Not really a comment on this patch, but do we actually need/want to put the migration fd into the epoll loop? IIUC, everything we're doing on the migration fd is synchronous and blocking, so we could run the whole migration from the VHOST_USER_SET_DEVICE_STATE callback.

The epoll event bits are practical to have here, though.

Or, maybe less confusing, flag the migration to run before we next call epoll_wait(): having the migration run strictly in-between epoll cycles seems like it might make some things easier to analyse.

Well, we don't really *need* to have the descriptor in the epoll list, but it's more consistent. Should we really introduce a different mechanism just because we don't strictly need that? What's the actual problem with the current implementation?

...

break; default: /* Can't happen */ diff --git a/vu_common.c b/vu_common.c index f43d8ac..6c346c8 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,90 @@ 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 + * @c: Execution context + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_source(struct ctx *c, int fd) +{ + struct migrate_meta m; + int rc; + + if ((rc = migrate_source_pre(c, &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(c, &m); + + return rc; +} + +/** + * vu_migrate_target() - Migration as target, receive state from hypervisor + * @c: Execution context + * @fd: File descriptor for state transfer + * + * Return: 0 on success, positive error code on failure + */ +static int vu_migrate_target(struct ctx *c, 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(c, &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(c, &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(c, vdev->device_state_fd); + else if (events & EPOLLIN) + rc = vu_migrate_target(c, vdev->device_state_fd); + + /* EPOLLHUP without EPOLLIN/EPOLLOUT, or EPOLLERR? Migration failed */ + + vdev->device_state_result = rc;

Again, not really a comment on this patch, but I think it would be safer to set device_state_result to something non-zero (probably EINPROGRESS) as soon as we get the VHOST_USER_SET_DEVICE_STATE_FD. That way if somehow VHOST_USER_CHECK_DEVICE_STATE were called before we actually do anything about the migration we'd correctly report that it hasn't happened.

I see your point, but I'm not sure if it's correct to report EINPROGRESS while the migration is not actually in progress. That is, if we get VHOST_USER_SET_DEVICE_STATE_FD, and then the hypervisor decides to just send VHOST_USER_CHECK_DEVICE_STATE right away, the migration isn't in progress. This is especially relevant as the vhost-user documentation says that: "any non-zero value is an error" Further, it also states, about VHOST_USER_CHECK_DEVICE_STATE, that: The back-end responds once it knows whether the transfer and processing was successful or not. so, strictly speaking, we should probably *not* respond to that before we're done here. On the other hand, we risk triggering issues/hangs in the hypervisor just for the sake of being correct, so I would rather report 0 as it would be (not actually happening) done now. -- Stefano

On Wed, Jan 29, 2025 at 12:39:39AM +0100, Stefano Brivio wrote:

In vhost-user mode, by default, create a second UNIX domain socket accepting connections from passt-repair, with the usual listener socket.

When we need to set or clear TCP_REPAIR on sockets, we'll send them via SCM_RIGHTS to passt-repair, who sets the socket option values we ask for.

To that end, introduce batched functions to request TCP_REPAIR settings on sockets, so that we don't have to send a single message for each socket, on migration. When needed, repair_flush() will send the message and check for the reply.

Signed-off-by: Stefano Brivio --- Makefile | 12 ++-- conf.c | 46 ++++++++++-- epoll_type.h | 4 ++ passt.1 | 11 +++ passt.c | 9 +++ passt.h | 7 ++ repair.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++ repair.h | 16 +++++ tap.c | 65 +---------------- util.c | 62 +++++++++++++++++ util.h | 1 + 11 files changed, 353 insertions(+), 72 deletions(-) create mode 100644 repair.c create mode 100644 repair.h

diff --git a/Makefile b/Makefile index 1b71cb0..f67a20b 100644 --- a/Makefile +++ b/Makefile @@ -38,9 +38,9 @@ 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 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 + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c \ + repair.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 PASST_REPAIR_SRCS = passt-repair.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) @@ -50,9 +50,9 @@ 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 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 + pcap.h pif.h repair.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/conf.c b/conf.c index df2b016..85dec44 100644 --- a/conf.c +++ b/conf.c @@ -816,6 +816,9 @@ static void usage(const char *name, FILE *f, int status) " UNIX domain socket is provided by -s option\n" " --print-capabilities print back-end capabilities in JSON format,\n" " only meaningful for vhost-user mode\n"); + FPRINTF(f, + " --repair-path PATH path for passt-repair(1)\n"

Nit: as a privileged helper, should it be passt-repair(8)?

+ " default: append '.repair' to UNIX domain path\n"); }

FPRINTF(f, @@ -1240,8 +1243,30 @@ static void conf_nat(const char *arg, struct in_addr *addr4, */ static void conf_open_files(struct ctx *c) { - if (c->mode != MODE_PASTA && c->fd_tap == -1) - c->fd_tap_listen = tap_sock_unix_open(c->sock_path); + if (c->mode != MODE_PASTA && c->fd_tap == -1) { + c->fd_tap_listen = sock_unix(c->sock_path); + + if (c->mode == MODE_VU && strcmp(c->repair_path, "none")) { + if (!strncmp(c->repair_path, "./", 2)) { + memmove(c->repair_path, c->repair_path + 2, + sizeof(c->repair_path) - 2); + }

Do you need this? Shouldn't "./whatever" be usable as-is?

+ + if (!*c->repair_path && + snprintf_check(c->repair_path, + sizeof(c->repair_path), "%s.repair", + c->sock_path)) { + warn("passt-repair path %s not usable", + c->repair_path);

I'd prefer a die() here - I think omitting a possibly expected feature, with just a warning that could easily be lost in the logs is not a good idea.

+ c->fd_repair_listen = -1; + } else { + c->fd_repair_listen = sock_unix(c->repair_path); + } + } else { + c->fd_repair_listen = -1; + } + c->fd_repair = -1; + }

if (*c->pidfile) { c->pidfile_fd = output_file_open(c->pidfile, O_WRONLY); @@ -1354,9 +1379,12 @@ void conf(struct ctx *c, int argc, char **argv) {"host-lo-to-ns-lo", no_argument, NULL, 23 }, {"dns-host", required_argument, NULL, 24 }, {"vhost-user", no_argument, NULL, 25 }, + /* vhost-user backend program convention */ {"print-capabilities", no_argument, NULL, 26 }, {"socket-path", required_argument, NULL, 's' }, + + {"repair-path", required_argument, NULL, 27 }, { 0 }, }; const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; @@ -1824,8 +1852,8 @@ void conf(struct ctx *c, int argc, char **argv) if (c->ifi4 && IN4_IS_ADDR_UNSPECIFIED(&c->ip4.guest_gw)) c->no_dhcp = 1;

- /* Inbound port options & DNS can be parsed now (after IPv4/IPv6 - * settings) + /* Inbound port options, DNS, and --repair-path can be parsed now, after + * IPv4/IPv6 settings and --vhost-user. */ fwd_probe_ephemeral(); udp_portmap_clear(); @@ -1871,6 +1899,16 @@ void conf(struct ctx *c, int argc, char **argv) }

die("Cannot use DNS address %s", optarg); + } else if (name == 27) { + if (c->mode != MODE_VU && strcmp(optarg, "none")) + die("--repair-path is for vhost-user mode only"); + + if (snprintf_check(c->repair_path, + sizeof(c->repair_path), "%s", + optarg)) + die("Invalid passt-repair path: %s", optarg); + + break; } } while (name != -1);

diff --git a/epoll_type.h b/epoll_type.h index fd9eac3..706238a 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -42,6 +42,10 @@ enum epoll_type { EPOLL_TYPE_VHOST_KICK, /* vhost-user migration socket */ EPOLL_TYPE_VHOST_MIGRATION, + /* TCP_REPAIR helper listening socket */ + EPOLL_TYPE_REPAIR_LISTEN, + /* TCP_REPAIR helper socket */ + EPOLL_TYPE_REPAIR,

EPOLL_NUM_TYPES, }; diff --git a/passt.1 b/passt.1 index d9cd33e..63a3a01 100644 --- a/passt.1 +++ b/passt.1 @@ -418,6 +418,17 @@ Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR. .BR \-\-print-capabilities Print back-end capabilities in JSON format, only meaningful for vhost-user mode.

+.TP +.BR \-\-repair-path " " \fIpath +Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect

passt-repair(8)?

+to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during +migration. \fB--repair-path none\fR disables this interface (if you need to +specify a socket path called "none" you can prefix the path by \fI./\fR). + +Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path +chosen for the hypervisor UNIX domain socket. No socket is created if not in +\-\-vhost-user mode. + .TP .BR \-F ", " \-\-fd " " \fIFD Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened diff --git a/passt.c b/passt.c index 184d4e5..1fa2ddd 100644 --- a/passt.c +++ b/passt.c @@ -51,6 +51,7 @@ #include "tcp_splice.h" #include "ndp.h" #include "vu_common.h" +#include "repair.h"

#define EPOLL_EVENTS 8

@@ -76,6 +77,8 @@ char *epoll_type_str[] = { [EPOLL_TYPE_VHOST_CMD] = "vhost-user command socket", [EPOLL_TYPE_VHOST_KICK] = "vhost-user kick socket", [EPOLL_TYPE_VHOST_MIGRATION] = "vhost-user migration socket", + [EPOLL_TYPE_REPAIR_LISTEN] = "TCP_REPAIR helper listening socket", + [EPOLL_TYPE_REPAIR] = "TCP_REPAIR helper socket", }; static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES, "epoll_type_str[] doesn't match enum epoll_type"); @@ -360,6 +363,12 @@ loop: case EPOLL_TYPE_VHOST_MIGRATION: vu_migrate(&c, eventmask); break; + case EPOLL_TYPE_REPAIR_LISTEN: + repair_listen_handler(&c, eventmask); + break; + case EPOLL_TYPE_REPAIR: + repair_handler(&c, eventmask); + break; default: /* Can't happen */ ASSERT(0); diff --git a/passt.h b/passt.h index 0dd4efa..85b0a10 100644 --- a/passt.h +++ b/passt.h @@ -20,6 +20,7 @@ union epoll_ref; #include "siphash.h" #include "ip.h" #include "inany.h" +#include "migrate.h" #include "flow.h" #include "icmp.h" #include "fwd.h" @@ -193,6 +194,7 @@ struct ip6_ctx { * @foreground: Run in foreground, don't log to stderr by default * @nofile: Maximum number of open files (ulimit -n) * @sock_path: Path for UNIX domain socket + * @repair_path: TCP_REPAIR helper path, can be "none", empty for default * @pcap: Path for packet capture file * @pidfile: Path to PID file, empty string if not configured * @pidfile_fd: File descriptor for PID file, -1 if none @@ -203,6 +205,8 @@ struct ip6_ctx { * @epollfd: File descriptor for epoll instance * @fd_tap_listen: File descriptor for listening AF_UNIX socket, if any * @fd_tap: AF_UNIX socket, tuntap device, or pre-opened socket + * @fd_repair_listen: File descriptor for listening TCP_REPAIR socket, if any + * @fd_repair: Connected AF_UNIX socket for TCP_REPAIR helper * @our_tap_mac: Pasta/passt's MAC on the tap link * @guest_mac: MAC address of guest or namespace, seen or configured * @hash_secret: 128-bit secret for siphash functions @@ -244,6 +248,7 @@ struct ctx { int foreground; int nofile; char sock_path[UNIX_PATH_MAX]; + char repair_path[UNIX_PATH_MAX]; char pcap[PATH_MAX];

char pidfile[PATH_MAX]; @@ -260,6 +265,8 @@ struct ctx { int epollfd; int fd_tap_listen; int fd_tap; + int fd_repair_listen; + int fd_repair; unsigned char our_tap_mac[ETH_ALEN]; unsigned char guest_mac[ETH_ALEN]; uint64_t hash_secret[2]; diff --git a/repair.c b/repair.c new file mode 100644 index 0000000..24966f5 --- /dev/null +++ b/repair.c @@ -0,0 +1,192 @@ +// 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 + * + * repair.c - Interface (server) for passt-repair, set/clear TCP_REPAIR + * + * 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 "repair.h" + +#define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */ + +static int fds[SCM_MAX_FD];

Even 'static', I'd prefer a longer name for a global variable.

+static int current_cmd;

Is there any particular rationale behind these being globals, whereas fd_repair is in struct ctx? AFAICT they're basically equally global in practice.

+static int nfds; + +/** + * repair_sock_init() - Start listening for connections on helper socket + * @c: Execution context + */ +void repair_sock_init(const struct ctx *c) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_REPAIR_LISTEN }; + struct epoll_event ev = { 0 }; + + listen(c->fd_repair_listen, 0); + + ref.fd = c->fd_repair_listen; + ev.events = EPOLLIN | EPOLLHUP | EPOLLET; + ev.data.u64 = ref.u64; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_repair_listen, &ev); +} + +/** + * repair_listen_handler() - Handle events on TCP_REPAIR helper listening socket + * @c: Execution context + * @events: epoll events + */ +void repair_listen_handler(struct ctx *c, uint32_t events) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_REPAIR }; + struct epoll_event ev = { 0 }; + struct ucred ucred; + socklen_t len; + + if (events != EPOLLIN) { + debug("Spurious event 0x%04x on TCP_REPAIR helper socket", + events); + return; + } + + len = sizeof(ucred); + + /* Another client is already connected: accept and close right away. */

For the repair socket, last-connection-wins would make more sense to me than first-connection-wins. While hacking/debugging seems it might be useful to fix something in the passt-repair, re-run it and have it displace the stale version for existing passt instances.

+ if (c->fd_repair != -1) { + int discard = accept4(c->fd_repair_listen, NULL, NULL, + SOCK_NONBLOCK); + + if (discard == -1) + return; + + if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("Discarding TCP_REPAIR helper, PID %i", ucred.pid); + + close(discard); + return; + } + + c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, 0); + + if (!getsockopt(c->fd_repair, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("Accepted TCP_REPAIR helper, PID %i", ucred.pid); + + ref.fd = c->fd_repair; + ev.events = EPOLLHUP | EPOLLET; + ev.data.u64 = ref.u64; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_repair, &ev); +} + +/** + * repair_close() - Close connection to TCP_REPAIR helper + * @c: Execution context + */ +void repair_close(struct ctx *c) +{ + debug("Closing TCP_REPAIR helper socket"); + + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_repair, NULL); + close(c->fd_repair); + c->fd_repair = -1; +} + +/** + * repair_handler() - Handle EPOLLHUP and EPOLLERR on TCP_REPAIR helper socket + * @c: Execution context + * @events: epoll events + */ +void repair_handler(struct ctx *c, uint32_t events) +{ + (void)events; + + repair_close(c); +} + +/** + * repair_flush() - Flush current set of sockets to helper, with current command + * @c: Execution context + * + * Return: 0 on success, negative error code on failure + */ +int repair_flush(struct ctx *c) +{ + struct iovec iov = { &((int8_t){ current_cmd }), sizeof(int8_t) }; + char buf[CMSG_SPACE(sizeof(int) * SCM_MAX_FD)] + __attribute__ ((aligned(__alignof__(struct cmsghdr)))); + struct cmsghdr *cmsg; + struct msghdr msg; + int ret = 0; + + if (!nfds) + return 0; + + msg = (struct msghdr){ NULL, 0, &iov, 1, + buf, CMSG_SPACE(sizeof(int) * nfds), 0 }; + cmsg = CMSG_FIRSTHDR(&msg); + + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(int) * nfds); + memcpy(CMSG_DATA(cmsg), fds, sizeof(int) * nfds); + + nfds = 0; + + if (sendmsg(c->fd_repair, &msg, 0) < 0) { + ret = -errno;

This error code won't be reported to the caller: you'll continue on to the recv() below, which will return EBADF, clobbering ret.

+ err_perror("Failed to send sockets to TCP_REPAIR helper"); + repair_close(c); + } + + if (recv(c->fd_repair, &((int8_t){ 0 }), 1, 0) < 0) { + ret = -errno; + err_perror("Failed to receive reply from TCP_REPAIR helper"); + repair_close(c); + } + + return ret; +} + +/** + * repair_flush() - Add socket to TCP_REPAIR set with given command + * @c: Execution context + * @s: Socket to add + * @cmd: TCP_REPAIR_ON, TCP_REPAIR_OFF, or TCP_REPAIR_OFF_NO_WP + * + * Return: 0 on success, negative error code on failure + */ +/* cppcheck-suppress unusedFunction */ +int repair_set(struct ctx *c, int s, int cmd) +{ + int rc; + + if (nfds && current_cmd != cmd) { + if ((rc = repair_flush(c))) + return rc; + } + + current_cmd = cmd; + fds[nfds++] = s; + + if (nfds >= SCM_MAX_FD) { + if ((rc = repair_flush(c))) + return rc; + } + + return 0; +} diff --git a/repair.h b/repair.h new file mode 100644 index 0000000..693c515 --- /dev/null +++ b/repair.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef REPAIR_H +#define REPAIR_H + +void repair_sock_init(const struct ctx *c); +void repair_listen_handler(struct ctx *c, uint32_t events); +void repair_handler(struct ctx *c, uint32_t events); +void repair_close(struct ctx *c); +int repair_flush(struct ctx *c); +int repair_set(struct ctx *c, int s, int cmd); + +#endif /* REPAIR_H */ diff --git a/tap.c b/tap.c index cd32a90..0e60eb4 100644 --- a/tap.c +++ b/tap.c @@ -56,6 +56,7 @@ #include "netlink.h" #include "pasta.h" #include "packet.h" +#include "repair.h" #include "tap.h" #include "log.h" #include "vhost_user.h" @@ -1151,68 +1152,6 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, tap_pasta_input(c, now); }

-/** - * tap_sock_unix_open() - Create and bind AF_UNIX socket - * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) - * - * Return: socket descriptor on success, won't return on failure - */ -int tap_sock_unix_open(char *sock_path) -{ - int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); - struct sockaddr_un addr = { - .sun_family = AF_UNIX, - }; - int i; - - if (fd < 0) - die_perror("Failed to open UNIX domain socket"); - - for (i = 1; i < UNIX_SOCK_MAX; i++) { - char *path = addr.sun_path; - int ex, ret; - - if (*sock_path) - memcpy(path, sock_path, UNIX_PATH_MAX); - else if (snprintf_check(path, UNIX_PATH_MAX - 1, - UNIX_SOCK_PATH, i)) - die_perror("Can't build UNIX domain socket path"); - - ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, - 0); - if (ex < 0) - die_perror("Failed to check for UNIX domain conflicts"); - - ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); - if (!ret || (errno != ENOENT && errno != ECONNREFUSED && - errno != EACCES)) { - if (*sock_path) - die("Socket path %s already in use", path); - - close(ex); - continue; - } - close(ex); - - unlink(path); - ret = bind(fd, (const struct sockaddr *)&addr, sizeof(addr)); - if (*sock_path && ret) - die_perror("Failed to bind UNIX domain socket"); - - if (!ret) - break; - } - - if (i == UNIX_SOCK_MAX) - die_perror("Failed to bind UNIX domain socket"); - - info("UNIX domain socket bound at %s", addr.sun_path); - if (!*sock_path) - memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); - - return fd; -} - /** * tap_backend_show_hints() - Give help information to start QEMU * @c: Execution context @@ -1423,6 +1362,8 @@ void tap_backend_init(struct ctx *c) tap_sock_tun_init(c); break; case MODE_VU: + repair_sock_init(c); + /* fall through */ case MODE_PASST: tap_sock_unix_init(c);

diff --git a/util.c b/util.c index 36857d4..e98da74 100644 --- a/util.c +++ b/util.c @@ -178,6 +178,68 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type, return fd; }

+/** + * sock_unix() - Create and bind AF_UNIX socket + * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) + * + * Return: socket descriptor on success, won't return on failure + */

I like making tap_sock_unix_open() more general. Could be split into a separate patch.

+int sock_unix(char *sock_path) +{ + int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); + struct sockaddr_un addr = { + .sun_family = AF_UNIX, + }; + int i; + + if (fd < 0) + die_perror("Failed to open UNIX domain socket"); + + for (i = 1; i < UNIX_SOCK_MAX; i++) { + char *path = addr.sun_path; + int ex, ret; + + if (*sock_path) + memcpy(path, sock_path, UNIX_PATH_MAX); + else if (snprintf_check(path, UNIX_PATH_MAX - 1, + UNIX_SOCK_PATH, i)) + die_perror("Can't build UNIX domain socket path"); + + ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, + 0); + if (ex < 0) + die_perror("Failed to check for UNIX domain conflicts"); + + ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); + if (!ret || (errno != ENOENT && errno != ECONNREFUSED && + errno != EACCES)) { + if (*sock_path) + die("Socket path %s already in use", path); + + close(ex); + continue; + } + close(ex); + + unlink(path); + ret = bind(fd, (const struct sockaddr *)&addr, sizeof(addr)); + if (*sock_path && ret) + die_perror("Failed to bind UNIX domain socket"); + + if (!ret) + break; + } + + if (i == UNIX_SOCK_MAX) + die_perror("Failed to bind UNIX domain socket"); + + info("UNIX domain socket bound at %s", addr.sun_path); + if (!*sock_path) + memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); + + return fd; +} + /** * sock_probe_mem() - Check if setting high SO_SNDBUF and SO_RCVBUF is allowed * @c: Execution context diff --git a/util.h b/util.h index 73a7a33..d2fd2fe 100644 --- a/util.h +++ b/util.h @@ -185,6 +185,7 @@ struct ctx; int sock_l4_sa(const struct ctx *c, enum epoll_type type, const void *sa, socklen_t sl, const char *ifname, bool v6only, uint32_t data); +int sock_unix(char *sock_path); void sock_probe_mem(struct ctx *c); long timespec_diff_ms(const struct timespec *a, const struct timespec *b); int64_t timespec_diff_us(const struct timespec *a, const struct timespec *b);

-- 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 09:46:10AM +0100, Stefano Brivio wrote:

On Wed, 29 Jan 2025 17:09:07 +1100 David Gibson wrote:

...

On Wed, Jan 29, 2025 at 12:39:39AM +0100, Stefano Brivio wrote:

...

In vhost-user mode, by default, create a second UNIX domain socket accepting connections from passt-repair, with the usual listener socket.

When we need to set or clear TCP_REPAIR on sockets, we'll send them via SCM_RIGHTS to passt-repair, who sets the socket option values we ask for.

To that end, introduce batched functions to request TCP_REPAIR settings on sockets, so that we don't have to send a single message for each socket, on migration. When needed, repair_flush() will send the message and check for the reply.

Signed-off-by: Stefano Brivio --- Makefile | 12 ++-- conf.c | 46 ++++++++++-- epoll_type.h | 4 ++ passt.1 | 11 +++ passt.c | 9 +++ passt.h | 7 ++ repair.c | 192 +++++++++++++++++++++++++++++++++++++++++++++++++++ repair.h | 16 +++++ tap.c | 65 +---------------- util.c | 62 +++++++++++++++++ util.h | 1 + 11 files changed, 353 insertions(+), 72 deletions(-) create mode 100644 repair.c create mode 100644 repair.h

diff --git a/Makefile b/Makefile index 1b71cb0..f67a20b 100644 --- a/Makefile +++ b/Makefile @@ -38,9 +38,9 @@ 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 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 + ndp.c netlink.c migrate.c packet.c passt.c pasta.c pcap.c pif.c \ + repair.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 PASST_REPAIR_SRCS = passt-repair.c SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) @@ -50,9 +50,9 @@ 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 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 + pcap.h pif.h repair.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/conf.c b/conf.c index df2b016..85dec44 100644 --- a/conf.c +++ b/conf.c @@ -816,6 +816,9 @@ static void usage(const char *name, FILE *f, int status) " UNIX domain socket is provided by -s option\n" " --print-capabilities print back-end capabilities in JSON format,\n" " only meaningful for vhost-user mode\n"); + FPRINTF(f, + " --repair-path PATH path for passt-repair(1)\n"

Nit: as a privileged helper, should it be passt-repair(8)?

So, I spent a couple of minutes on this as I wrote this, and I spent a bit longer now: the most authoritative definition I can find of section 8 is from man-pages(7):

8 System management commands Commands like mount(8), many of which only root can execute.

It's not really a system management command, and the idea is to run it with CAP_NET_ADMIN, but not necessarily as root. So I would rather keep it in section 1, unless there's some other conflicting definition I'm not aware of.

Yeah oh, on that basis (1) makes more sense than (8).

There's also the topic of where it should be installed (/sbin, /usr/sbin/, /bin, /usr/bin). I'd pick /usr/bin, because /sbin doesn't really mean much nowadays, and it's anyway fitting with the FHS 3.0:

Utilities used for system administration (and other root-only commands) are stored in /sbin, /usr/ sbin, and /usr/local/sbin.

/sbin contains binaries essential for booting, restoring, recovering, and/or repairing the system in addition to the binaries in /bin. 18 Programs executed after /usr is known to be mounted (when there are no problems) are generally placed into /usr/sbin.

...and I don't think this helper would qualify for /sbin or /usr/sbin.

...

...

+ " default: append '.repair' to UNIX domain path\n"); }

FPRINTF(f, @@ -1240,8 +1243,30 @@ static void conf_nat(const char *arg, struct in_addr *addr4, */ static void conf_open_files(struct ctx *c) { - if (c->mode != MODE_PASTA && c->fd_tap == -1) - c->fd_tap_listen = tap_sock_unix_open(c->sock_path); + if (c->mode != MODE_PASTA && c->fd_tap == -1) { + c->fd_tap_listen = sock_unix(c->sock_path); + + if (c->mode == MODE_VU && strcmp(c->repair_path, "none")) { + if (!strncmp(c->repair_path, "./", 2)) { + memmove(c->repair_path, c->repair_path + 2, + sizeof(c->repair_path) - 2); + }

Do you need this? Shouldn't "./whatever" be usable as-is?

Ah, yes, I didn't know. I explicitly added this for the '--repair-path ./none' case, but it's not actually needed. I'll drop this.

...

...

+ + if (!*c->repair_path && + snprintf_check(c->repair_path, + sizeof(c->repair_path), "%s.repair", + c->sock_path)) { + warn("passt-repair path %s not usable", + c->repair_path);

I'd prefer a die() here - I think omitting a possibly expected feature, with just a warning that could easily be lost in the logs is not a good idea.

I was thinking about that, but should we really risk *not* starting because with ".repair" the path is now too long, for a feature that, realistically, most users won't actually use?

If we're started by any kind of framework (which is where we run the risk of the warning being ignored), then there should be explicit checks about the path and the usability of passt-repair (or equivalent).

Hm, yeah, I guess. Honestly I'm not sure which option will cause us less trouble.

...

...

+ c->fd_repair_listen = -1; + } else { + c->fd_repair_listen = sock_unix(c->repair_path); + } + } else { + c->fd_repair_listen = -1; + } + c->fd_repair = -1; + }

if (*c->pidfile) { c->pidfile_fd = output_file_open(c->pidfile, O_WRONLY); @@ -1354,9 +1379,12 @@ void conf(struct ctx *c, int argc, char **argv) {"host-lo-to-ns-lo", no_argument, NULL, 23 }, {"dns-host", required_argument, NULL, 24 }, {"vhost-user", no_argument, NULL, 25 }, + /* vhost-user backend program convention */ {"print-capabilities", no_argument, NULL, 26 }, {"socket-path", required_argument, NULL, 's' }, + + {"repair-path", required_argument, NULL, 27 }, { 0 }, }; const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt"; @@ -1824,8 +1852,8 @@ void conf(struct ctx *c, int argc, char **argv) if (c->ifi4 && IN4_IS_ADDR_UNSPECIFIED(&c->ip4.guest_gw)) c->no_dhcp = 1;

- /* Inbound port options & DNS can be parsed now (after IPv4/IPv6 - * settings) + /* Inbound port options, DNS, and --repair-path can be parsed now, after + * IPv4/IPv6 settings and --vhost-user. */ fwd_probe_ephemeral(); udp_portmap_clear(); @@ -1871,6 +1899,16 @@ void conf(struct ctx *c, int argc, char **argv) }

die("Cannot use DNS address %s", optarg); + } else if (name == 27) { + if (c->mode != MODE_VU && strcmp(optarg, "none")) + die("--repair-path is for vhost-user mode only"); + + if (snprintf_check(c->repair_path, + sizeof(c->repair_path), "%s", + optarg)) + die("Invalid passt-repair path: %s", optarg); + + break; } } while (name != -1);

diff --git a/epoll_type.h b/epoll_type.h index fd9eac3..706238a 100644 --- a/epoll_type.h +++ b/epoll_type.h @@ -42,6 +42,10 @@ enum epoll_type { EPOLL_TYPE_VHOST_KICK, /* vhost-user migration socket */ EPOLL_TYPE_VHOST_MIGRATION, + /* TCP_REPAIR helper listening socket */ + EPOLL_TYPE_REPAIR_LISTEN, + /* TCP_REPAIR helper socket */ + EPOLL_TYPE_REPAIR,

EPOLL_NUM_TYPES, }; diff --git a/passt.1 b/passt.1 index d9cd33e..63a3a01 100644 --- a/passt.1 +++ b/passt.1 @@ -418,6 +418,17 @@ Enable vhost-user. The vhost-user command socket is provided by \fB--socket\fR. .BR \-\-print-capabilities Print back-end capabilities in JSON format, only meaningful for vhost-user mode.

+.TP +.BR \-\-repair-path " " \fIpath +Path for UNIX domain socket used by the \fBpasst-repair\fR(1) helper to connect

passt-repair(8)?

See above.

...

...

+to \fBpasst\fR in order to set or clear the TCP_REPAIR option on sockets, during +migration. \fB--repair-path none\fR disables this interface (if you need to +specify a socket path called "none" you can prefix the path by \fI./\fR). + +Default, for \-\-vhost-user mode only, is to append \fI.repair\fR to the path +chosen for the hypervisor UNIX domain socket. No socket is created if not in +\-\-vhost-user mode. + .TP .BR \-F ", " \-\-fd " " \fIFD Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened diff --git a/passt.c b/passt.c index 184d4e5..1fa2ddd 100644 --- a/passt.c +++ b/passt.c @@ -51,6 +51,7 @@ #include "tcp_splice.h" #include "ndp.h" #include "vu_common.h" +#include "repair.h"

#define EPOLL_EVENTS 8

@@ -76,6 +77,8 @@ char *epoll_type_str[] = { [EPOLL_TYPE_VHOST_CMD] = "vhost-user command socket", [EPOLL_TYPE_VHOST_KICK] = "vhost-user kick socket", [EPOLL_TYPE_VHOST_MIGRATION] = "vhost-user migration socket", + [EPOLL_TYPE_REPAIR_LISTEN] = "TCP_REPAIR helper listening socket", + [EPOLL_TYPE_REPAIR] = "TCP_REPAIR helper socket", }; static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES, "epoll_type_str[] doesn't match enum epoll_type"); @@ -360,6 +363,12 @@ loop: case EPOLL_TYPE_VHOST_MIGRATION: vu_migrate(&c, eventmask); break; + case EPOLL_TYPE_REPAIR_LISTEN: + repair_listen_handler(&c, eventmask); + break; + case EPOLL_TYPE_REPAIR: + repair_handler(&c, eventmask); + break; default: /* Can't happen */ ASSERT(0); diff --git a/passt.h b/passt.h index 0dd4efa..85b0a10 100644 --- a/passt.h +++ b/passt.h @@ -20,6 +20,7 @@ union epoll_ref; #include "siphash.h" #include "ip.h" #include "inany.h" +#include "migrate.h" #include "flow.h" #include "icmp.h" #include "fwd.h" @@ -193,6 +194,7 @@ struct ip6_ctx { * @foreground: Run in foreground, don't log to stderr by default * @nofile: Maximum number of open files (ulimit -n) * @sock_path: Path for UNIX domain socket + * @repair_path: TCP_REPAIR helper path, can be "none", empty for default * @pcap: Path for packet capture file * @pidfile: Path to PID file, empty string if not configured * @pidfile_fd: File descriptor for PID file, -1 if none @@ -203,6 +205,8 @@ struct ip6_ctx { * @epollfd: File descriptor for epoll instance * @fd_tap_listen: File descriptor for listening AF_UNIX socket, if any * @fd_tap: AF_UNIX socket, tuntap device, or pre-opened socket + * @fd_repair_listen: File descriptor for listening TCP_REPAIR socket, if any + * @fd_repair: Connected AF_UNIX socket for TCP_REPAIR helper * @our_tap_mac: Pasta/passt's MAC on the tap link * @guest_mac: MAC address of guest or namespace, seen or configured * @hash_secret: 128-bit secret for siphash functions @@ -244,6 +248,7 @@ struct ctx { int foreground; int nofile; char sock_path[UNIX_PATH_MAX]; + char repair_path[UNIX_PATH_MAX]; char pcap[PATH_MAX];

char pidfile[PATH_MAX]; @@ -260,6 +265,8 @@ struct ctx { int epollfd; int fd_tap_listen; int fd_tap; + int fd_repair_listen; + int fd_repair; unsigned char our_tap_mac[ETH_ALEN]; unsigned char guest_mac[ETH_ALEN]; uint64_t hash_secret[2]; diff --git a/repair.c b/repair.c new file mode 100644 index 0000000..24966f5 --- /dev/null +++ b/repair.c @@ -0,0 +1,192 @@ +// 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 + * + * repair.c - Interface (server) for passt-repair, set/clear TCP_REPAIR + * + * 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 "repair.h" + +#define SCM_MAX_FD 253 /* From Linux kernel (include/net/scm.h), not in UAPI */ + +static int fds[SCM_MAX_FD];

Even 'static', I'd prefer a longer name for a global variable.

A longer name for this also means a longer name for nfds below, which is not necessarily practical, but I'll check and try to change these to repair_fds[] / repair_nfds if doable.

...

...

+static int current_cmd;

Is there any particular rationale behind these being globals, whereas fd_repair is in struct ctx? AFAICT they're basically equally global in practice.

Yes: c->fd_repair_listen needs to be in struct ctx, and I want to keep this consistent with c->fd_tap_listen / c->fd_tap.

Besides, the variables declared here are really hacks representing state for this compilation unit only.

Oh, right. Essentially these (static) globals are less global than ctx, which is truly world global. Ok, that makes sense.

...

...

+static int nfds; + +/** + * repair_sock_init() - Start listening for connections on helper socket + * @c: Execution context + */ +void repair_sock_init(const struct ctx *c) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_REPAIR_LISTEN }; + struct epoll_event ev = { 0 }; + + listen(c->fd_repair_listen, 0); + + ref.fd = c->fd_repair_listen; + ev.events = EPOLLIN | EPOLLHUP | EPOLLET; + ev.data.u64 = ref.u64; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_repair_listen, &ev); +} + +/** + * repair_listen_handler() - Handle events on TCP_REPAIR helper listening socket + * @c: Execution context + * @events: epoll events + */ +void repair_listen_handler(struct ctx *c, uint32_t events) +{ + union epoll_ref ref = { .type = EPOLL_TYPE_REPAIR }; + struct epoll_event ev = { 0 }; + struct ucred ucred; + socklen_t len; + + if (events != EPOLLIN) { + debug("Spurious event 0x%04x on TCP_REPAIR helper socket", + events); + return; + } + + len = sizeof(ucred); + + /* Another client is already connected: accept and close right away. */

For the repair socket, last-connection-wins would make more sense to me than first-connection-wins. While hacking/debugging seems it might be useful to fix something in the passt-repair, re-run it and have it displace the stale version for existing passt instances.

In the whole debugging I've done so far I'm actually using passt-repair (otherwise I don't even start it), so it already terminates once it's done.

I think it's more secure to keep it like this, because it's more robust against races.

Let's say we have an issue in KubeVirt's virt-handler so that the migration starts a bit before the helper is started, and somebody manages to use this time window to connect another helper, this attempt will actually go unnoticed.

If passt-repair fails to connect, it will be very obvious.

Ok, you convinced me.

...

...

+ if (c->fd_repair != -1) { + int discard = accept4(c->fd_repair_listen, NULL, NULL, + SOCK_NONBLOCK); + + if (discard == -1) + return; + + if (!getsockopt(discard, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("Discarding TCP_REPAIR helper, PID %i", ucred.pid); + + close(discard); + return; + } + + c->fd_repair = accept4(c->fd_repair_listen, NULL, NULL, 0); + + if (!getsockopt(c->fd_repair, SOL_SOCKET, SO_PEERCRED, &ucred, &len)) + info("Accepted TCP_REPAIR helper, PID %i", ucred.pid); + + ref.fd = c->fd_repair; + ev.events = EPOLLHUP | EPOLLET; + ev.data.u64 = ref.u64; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_repair, &ev); +} + +/** + * repair_close() - Close connection to TCP_REPAIR helper + * @c: Execution context + */ +void repair_close(struct ctx *c) +{ + debug("Closing TCP_REPAIR helper socket"); + + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_repair, NULL); + close(c->fd_repair); + c->fd_repair = -1; +} + +/** + * repair_handler() - Handle EPOLLHUP and EPOLLERR on TCP_REPAIR helper socket + * @c: Execution context + * @events: epoll events + */ +void repair_handler(struct ctx *c, uint32_t events) +{ + (void)events; + + repair_close(c); +} + +/** + * repair_flush() - Flush current set of sockets to helper, with current command + * @c: Execution context + * + * Return: 0 on success, negative error code on failure + */ +int repair_flush(struct ctx *c) +{ + struct iovec iov = { &((int8_t){ current_cmd }), sizeof(int8_t) }; + char buf[CMSG_SPACE(sizeof(int) * SCM_MAX_FD)] + __attribute__ ((aligned(__alignof__(struct cmsghdr)))); + struct cmsghdr *cmsg; + struct msghdr msg; + int ret = 0; + + if (!nfds) + return 0; + + msg = (struct msghdr){ NULL, 0, &iov, 1, + buf, CMSG_SPACE(sizeof(int) * nfds), 0 }; + cmsg = CMSG_FIRSTHDR(&msg); + + cmsg->cmsg_level = SOL_SOCKET; + cmsg->cmsg_type = SCM_RIGHTS; + cmsg->cmsg_len = CMSG_LEN(sizeof(int) * nfds); + memcpy(CMSG_DATA(cmsg), fds, sizeof(int) * nfds); + + nfds = 0; + + if (sendmsg(c->fd_repair, &msg, 0) < 0) { + ret = -errno;

This error code won't be reported to the caller: you'll continue on to the recv() below, which will return EBADF, clobbering ret.

Oops, right, I'll return early in this case (or equivalent).

...

...

+ err_perror("Failed to send sockets to TCP_REPAIR helper"); + repair_close(c); + } + + if (recv(c->fd_repair, &((int8_t){ 0 }), 1, 0) < 0) { + ret = -errno; + err_perror("Failed to receive reply from TCP_REPAIR helper"); + repair_close(c); + } + + return ret; +} + +/** + * repair_flush() - Add socket to TCP_REPAIR set with given command + * @c: Execution context + * @s: Socket to add + * @cmd: TCP_REPAIR_ON, TCP_REPAIR_OFF, or TCP_REPAIR_OFF_NO_WP + * + * Return: 0 on success, negative error code on failure + */ +/* cppcheck-suppress unusedFunction */ +int repair_set(struct ctx *c, int s, int cmd) +{ + int rc; + + if (nfds && current_cmd != cmd) { + if ((rc = repair_flush(c))) + return rc; + } + + current_cmd = cmd; + fds[nfds++] = s; + + if (nfds >= SCM_MAX_FD) { + if ((rc = repair_flush(c))) + return rc; + } + + return 0; +} diff --git a/repair.h b/repair.h new file mode 100644 index 0000000..693c515 --- /dev/null +++ b/repair.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later + * Copyright (c) 2025 Red Hat GmbH + * Author: Stefano Brivio + */ + +#ifndef REPAIR_H +#define REPAIR_H + +void repair_sock_init(const struct ctx *c); +void repair_listen_handler(struct ctx *c, uint32_t events); +void repair_handler(struct ctx *c, uint32_t events); +void repair_close(struct ctx *c); +int repair_flush(struct ctx *c); +int repair_set(struct ctx *c, int s, int cmd); + +#endif /* REPAIR_H */ diff --git a/tap.c b/tap.c index cd32a90..0e60eb4 100644 --- a/tap.c +++ b/tap.c @@ -56,6 +56,7 @@ #include "netlink.h" #include "pasta.h" #include "packet.h" +#include "repair.h" #include "tap.h" #include "log.h" #include "vhost_user.h" @@ -1151,68 +1152,6 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, tap_pasta_input(c, now); }

-/** - * tap_sock_unix_open() - Create and bind AF_UNIX socket - * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) - * - * Return: socket descriptor on success, won't return on failure - */ -int tap_sock_unix_open(char *sock_path) -{ - int fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); - struct sockaddr_un addr = { - .sun_family = AF_UNIX, - }; - int i; - - if (fd < 0) - die_perror("Failed to open UNIX domain socket"); - - for (i = 1; i < UNIX_SOCK_MAX; i++) { - char *path = addr.sun_path; - int ex, ret; - - if (*sock_path) - memcpy(path, sock_path, UNIX_PATH_MAX); - else if (snprintf_check(path, UNIX_PATH_MAX - 1, - UNIX_SOCK_PATH, i)) - die_perror("Can't build UNIX domain socket path"); - - ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK | SOCK_CLOEXEC, - 0); - if (ex < 0) - die_perror("Failed to check for UNIX domain conflicts"); - - ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr)); - if (!ret || (errno != ENOENT && errno != ECONNREFUSED && - errno != EACCES)) { - if (*sock_path) - die("Socket path %s already in use", path); - - close(ex); - continue; - } - close(ex); - - unlink(path); - ret = bind(fd, (const struct sockaddr *)&addr, sizeof(addr)); - if (*sock_path && ret) - die_perror("Failed to bind UNIX domain socket"); - - if (!ret) - break; - } - - if (i == UNIX_SOCK_MAX) - die_perror("Failed to bind UNIX domain socket"); - - info("UNIX domain socket bound at %s", addr.sun_path); - if (!*sock_path) - memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX); - - return fd; -} - /** * tap_backend_show_hints() - Give help information to start QEMU * @c: Execution context @@ -1423,6 +1362,8 @@ void tap_backend_init(struct ctx *c) tap_sock_tun_init(c); break; case MODE_VU: + repair_sock_init(c); + /* fall through */ case MODE_PASST: tap_sock_unix_init(c);

diff --git a/util.c b/util.c index 36857d4..e98da74 100644 --- a/util.c +++ b/util.c @@ -178,6 +178,68 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type, return fd; }

+/** + * sock_unix() - Create and bind AF_UNIX socket + * @sock_path: Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix) + * + * Return: socket descriptor on success, won't return on failure + */

I like making tap_sock_unix_open() more general. Could be split into a separate patch.

I need it here though, and keeping it here saves lines in the commit message...

Oh very well. -- 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 12:39:40AM +0100, Stefano Brivio wrote:

Very much draft quality, but it works. Ask passt-repair to switch TCP sockets to repair mode and dump their current sequence numbers to the flow table, which will be transferred and used by the target in the next step.

Signed-off-by: Stefano Brivio --- flow.c | 43 +++++++++++++++++++++++++++++++++++++++++ flow.h | 1 + migrate.c | 1 + tcp.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ tcp_conn.h | 5 +++++ 5 files changed, 106 insertions(+)

diff --git a/flow.c b/flow.c index ee1221b..e7148b2 100644 --- a/flow.c +++ b/flow.c @@ -19,6 +19,7 @@ #include "inany.h" #include "flow.h" #include "flow_table.h" +#include "repair.h"

const char *flow_state_str[] = { [FLOW_STATE_FREE] = "FREE", @@ -874,6 +875,48 @@ void flow_defer_handler(const struct ctx *c, const struct timespec *now) *last_next = FLOW_MAX; }

+/** + * flow_migrate_source_pre() - Prepare all source flows for migration + * @c: Execution context + * @m: Migration metadata + * + * Return: 0 on success + */ +int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m) +{ + unsigned i; + int rc; + + (void)m; + + for (i = 0; i < FLOW_MAX; i++) { /* TODO: iterator with skip */ + union flow *flow = &flowtab[i]; + + if (flow->f.state == FLOW_STATE_FREE) + i += flow->free.n - 1; + else if (flow->f.state == FLOW_STATE_ACTIVE &&

We should probably just abort any flows that are in pre-ACTIVE state at migration time. Wait... IIRC flows have to be in ACTIVE state (or already cancelled) once we get to the next epoll cycle. So we can possibly just assert that state is either ACTIVE or FREE.

+ flow->f.type == FLOW_TCP) + rc = tcp_flow_repair_on(c, &flow->tcp); + + if (rc) + return rc; /* TODO: rollback */ + } + + repair_flush(c); /* TODO: move to TCP logic */ + + for (i = 0; i < FLOW_MAX; i++) { /* TODO: iterator with skip */ + union flow *flow = &flowtab[i]; + + if (flow->f.state == FLOW_STATE_FREE) + i += flow->free.n - 1; + else if (flow->f.state == FLOW_STATE_ACTIVE && + flow->f.type == FLOW_TCP) + tcp_flow_dump_seq(c, &flow->tcp); + } + + return 0; +} + /** * flow_init() - Initialise flow related data structures */ diff --git a/flow.h b/flow.h index 8eb5964..ff390a6 100644 --- a/flow.h +++ b/flow.h @@ -255,6 +255,7 @@ union flow;

void flow_init(void); void flow_defer_handler(const struct ctx *c, const struct timespec *now); +int flow_migrate_source_pre(struct ctx *c, struct migrate_meta *m);

void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...) __attribute__((format(printf, 3, 4))); diff --git a/migrate.c b/migrate.c index b8b79e0..6707c02 100644 --- a/migrate.c +++ b/migrate.c @@ -56,6 +56,7 @@ static struct migrate_data data_versions[] = {

/* Handlers to call in source before sending data */ struct migrate_handler handlers_source_pre[] = { + { flow_migrate_source_pre }, { 0 }, };

diff --git a/tcp.c b/tcp.c index c89f323..3a3038b 100644 --- a/tcp.c +++ b/tcp.c @@ -299,6 +299,7 @@ #include "log.h" #include "inany.h" #include "flow.h" +#include "repair.h" #include "linux_dep.h"

#include "flow_table.h" @@ -868,6 +869,61 @@ void tcp_defer_handler(struct ctx *c) tcp_payload_flush(c); }

+/** + * tcp_flow_repair_on() - Enable repair mode for a single TCP flow + * @c: Execution context + * @conn: Pointer to the TCP connection structure + * + * Return: 0 on success, negative error code on failure + */ +int tcp_flow_repair_on(struct ctx *c, const struct tcp_tap_conn *conn) +{ + int rc = 0; + + if ((rc = repair_set(c, conn->sock, TCP_REPAIR_ON))) + err("Failed to set TCP_REPAIR for socket %i", conn->sock);

Well.. except that the error could just as easily have been on a previous socket that wasn't flushed yet.

+ + return rc; +} + +/** + * tcp_flow_dump_seq() - Dump sequences for send and receive queues + * @c: Execution context + * @conn: Pointer to the TCP connection structure + * + * Return: 0 on success, negative error code on failure + */ +int tcp_flow_dump_seq(struct ctx *c, struct tcp_tap_conn *conn) +{ + int v, s = conn->sock; + socklen_t vlen; + + (void)c; + + vlen = sizeof(v); + + v = TCP_SEND_QUEUE; + /* TODO: proper error management and prints */ + if (setsockopt(s, SOL_TCP, TCP_REPAIR_QUEUE, &v, vlen)) + return -errno; + + if (getsockopt(s, SOL_TCP, TCP_QUEUE_SEQ, &conn->sock_seq_snd, &vlen)) + return -errno; + + debug("Send queue sequence %u for socket %i", conn->sock_seq_snd, s); + + v = TCP_RECV_QUEUE; + if (setsockopt(s, SOL_TCP, TCP_REPAIR_QUEUE, &v, vlen)) + return -errno; + + if (getsockopt(s, SOL_TCP, TCP_QUEUE_SEQ, &conn->sock_seq_rcv, &vlen)) + return -errno; + + debug("Receive queue sequence %u for socket %i", conn->sock_seq_rcv, s); + + return 0; +} + /** * tcp_fill_header() - Fill the TCP header fields for a given TCP segment. * diff --git a/tcp_conn.h b/tcp_conn.h index d342680..0c3e197 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -94,6 +94,9 @@ struct tcp_tap_conn { uint32_t seq_from_tap; uint32_t seq_ack_to_tap; uint32_t seq_init_from_tap; + + uint32_t sock_seq_snd; + uint32_t sock_seq_rcv; };

/** @@ -140,6 +143,8 @@ extern int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; extern int init_sock_pool6 [TCP_SOCK_POOL_SIZE];

bool tcp_flow_defer(const struct tcp_tap_conn *conn); +int tcp_flow_repair_on(struct ctx *c, const struct tcp_tap_conn *conn); +int tcp_flow_dump_seq(struct ctx *c, struct tcp_tap_conn *conn); bool tcp_splice_flow_defer(struct tcp_splice_conn *conn); void tcp_splice_timer(const struct ctx *c, struct tcp_splice_conn *conn); int tcp_conn_pool_sock(int pool[]);

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