Re: [PATCH v9 10/23] pesto, conf: Have pesto connect to passt and check versions

[Laurent Vivier <lvivier@redhat.com>]

On 5/6/26 11:22, Stefano Brivio wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> Start implementing pesto in earnest.  Create a control/configuration
> socket in passt.  Have pesto connect to it and retrieve a server greeting
> Perform some basic version checking.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [sbrivio: Avoid potential recursive calling between conf_accept() and
>   conf_close(), reported by clang-tidy]
> [sbrivio: In conf(), check we're not exceeding sizeof(c->control_path)
>   instead of sizeof(c->socket_path), and, in pesto's main(), print
>   argv[optind] instead of argv[1] to indicate an invalid socket path,
>   both reported by Jon Maloy]
> [sbrivio: In pesto's main(), drop unnecessary newline from error
>   message, reported by Laurent]
> [sbrivio: Don't use SOCK_NONBLOCK on accept4(), as that only applies
>   to the *new* file descriptor, which we don't want -- set O_NONBLOCK
>   on the listening file descriptor using fcntl()]
> [sbrivio: Switch to protocol version 1, and reflect the true magic
>   behind pesto, i.e. basil, into the magic string]

But the real value is in pine nuts (110 €/kg here).
(I make mine with bear's garlic and almonds)

> [sbrivio: Fix conflicts in the Makefile caused by the fact that I'm
>   not merging a previous series reworking it]
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>   Makefile     |   2 +-
>   conf.c       | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>   conf.h       |   2 +
>   epoll_type.h |   4 ++
>   passt.1      |   5 ++
>   passt.c      |   8 +++
>   passt.h      |   6 ++
>   pesto.c      |  47 ++++++++++++-
>   pesto.h      |  22 ++++++
>   serialise.c  |   3 +
>   10 files changed, 279 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2639472..b1003d8 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -45,7 +45,7 @@ PASST_SRCS = arch.c arp.c bitmap.c checksum.c conf.c dhcp.c dhcpv6.c \
>   	vhost_user.c virtio.c vu_common.c
>   QRAP_SRCS = qrap.c
>   PASST_REPAIR_SRCS = passt-repair.c
> -PESTO_SRCS = pesto.c
> +PESTO_SRCS = pesto.c serialise.c
>   SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
>   
>   MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
> diff --git a/conf.c b/conf.c
> index 0586107..5ec0072 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -48,6 +48,10 @@
>   #include "isolation.h"
>   #include "log.h"
>   #include "vhost_user.h"
> +#include "epoll_ctl.h"
> +#include "conf.h"
> +#include "pesto.h"
> +#include "serialise.h"
>   
>   #define NETNS_RUN_DIR	"/run/netns"
>   
> @@ -541,6 +545,7 @@ static void usage(const char *name, FILE *f, int status)
>   		"  --runas UID|UID:GID 	Run as given UID, GID, which can be\n"
>   		"    numeric, or login and group names\n"
>   		"    default: drop to user \"nobody\"\n"
> +		"  -c, --conf-path PATH	Configuration socket path\n"
>   		"  -h, --help		Display this help message and exit\n"
>   		"  --version		Show version and exit\n");
>   
> @@ -779,6 +784,9 @@ static void conf_print(const struct ctx *c)
>   	char buf[INANY_ADDRSTRLEN];
>   	int i;
>   
> +	if (c->fd_control_listen >= 0)
> +		info("Configuration socket: %s", c->control_path);
> +
>   	if (c->ifi4 > 0 || c->ifi6 > 0) {
>   		char ifn[IFNAMSIZ];
>   
> @@ -1072,6 +1080,19 @@ static void conf_open_files(struct ctx *c)
>   		if (c->pidfile_fd < 0)
>   			die_perror("Couldn't open PID file %s", c->pidfile);
>   	}
> +
> +	c->fd_control = -1;
> +	if (*c->control_path) {
> +		c->fd_control_listen = sock_unix(c->control_path);
> +		if (c->fd_control_listen < 0) {
> +			die_perror("Couldn't open control socket %s",
> +				   c->control_path);
> +		}
> +		if (fcntl(c->fd_control_listen, F_SETFL, O_NONBLOCK))
> +			die_perror("Couldn't set O_NONBLOCK on control socket");
> +	} else {
> +		c->fd_control_listen = -1;
> +	}
>   }
>   
>   /**
> @@ -1107,6 +1128,25 @@ fail:
>   	die("Invalid MAC address: %s", str);
>   }
>   
> +/**
> + * conf_sock_listen() - Start listening for connections on configuration socket
> + * @c:		Execution context
> + */
> +static void conf_sock_listen(const struct ctx *c)
> +{
> +	union epoll_ref ref = { .type = EPOLL_TYPE_CONF_LISTEN };
> +
> +	if (c->fd_control_listen < 0)
> +		return;
> +
> +	if (listen(c->fd_control_listen, 0))
> +		die_perror("Couldn't listen on configuration socket");
> +
> +	ref.fd = c->fd_control_listen;
> +	if (epoll_add(c->epollfd, EPOLLIN | EPOLLET, ref))
> +		die_perror("Couldn't add configuration socket to epoll");
> +}
> +
>   /**
>    * conf() - Process command-line arguments and set configuration
>    * @c:		Execution context
> @@ -1189,9 +1229,10 @@ void conf(struct ctx *c, int argc, char **argv)
>   		{"migrate-exit", no_argument,		NULL,		29 },
>   		{"migrate-no-linger", no_argument,	NULL,		30 },
>   		{"stats", required_argument,		NULL,		31 },
> +		{"conf-path",	required_argument,	NULL,		'c' },
>   		{ 0 },
>   	};
> -	const char *optstring = "+dqfel:hs:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> +	const char *optstring = "+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
>   	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
>   	bool opt_t = false, opt_T = false, opt_u = false, opt_U = false;
>   	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX] = { 0 };
> @@ -1449,6 +1490,13 @@ void conf(struct ctx *c, int argc, char **argv)
>   
>   			c->fd_tap = -1;
>   			break;
> +		case 'c':
> +			ret = snprintf(c->control_path, sizeof(c->control_path),
> +				       "%s", optarg);
> +			if (ret <= 0 || ret >= (int)sizeof(c->control_path))
> +				die("Invalid configuration path: %s", optarg);
> +			c->fd_control_listen = c->fd_control = -1;
> +			break;
>   		case 'F':
>   			errno = 0;
>   			fd_tap_opt = strtol(optarg, NULL, 0);
> @@ -1871,6 +1919,140 @@ void conf(struct ctx *c, int argc, char **argv)
>   			fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]);
>   	}
>   
> +	conf_sock_listen(c);
> +
>   	if (!c->quiet)
>   		conf_print(c);
>   }
> +
> +static void conf_accept(struct ctx *c);
> +
> +/**
> + * conf_close() - Close configuration / control socket and clean up
> + * @c:		Execution context
> + */
> +static void conf_close(struct ctx *c)
> +{
> +	debug("Closing configuration socket");
> +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_control, NULL);
> +	close(c->fd_control);
> +	c->fd_control = -1;
> +}
> +
> +/**
> + * conf_listen_handler() - Handle events on configuration listening socket
> + * @c:		Execution context
> + * @events:	epoll events
> + */
> +void conf_listen_handler(struct ctx *c, uint32_t events)
> +{
> +	if (events != EPOLLIN) {
> +		err("Unexpected event 0x%04x on configuration socket", events);
> +		return;
> +	}
> +
> +	if (c->fd_control >= 0) {
> +		/* Ignore the new connection for now, blocking it until the
> +		 * current one finishes.
> +		 */
> +		return;
> +	}
> +
> +	conf_accept(c);
> +}
> +
> +/**
> + * conf_accept() - Accept a new control connection
> + * @c:		Execution context
> + */
> +static void conf_accept(struct ctx *c)
> +{
> +	struct pesto_hello hello = {
> +		.magic = PESTO_SERVER_MAGIC,
> +		.version = htonl(PESTO_PROTOCOL_VERSION),
> +	};
> +	union epoll_ref ref = { .type = EPOLL_TYPE_CONF };
> +	struct ucred uc = { 0 };
> +	socklen_t len = sizeof(uc);
> +	int fd, rc;
> +
> +retry:
> +	err("%s: %i", __func__, __LINE__);
> +	fd = accept4(c->fd_control_listen, NULL, NULL, SOCK_CLOEXEC);
> +	if (fd < 0) {
> +		err("%s: %i", __func__, __LINE__);
> +		if (errno != EAGAIN)
> +			warn_perror("accept4() on configuration listening socket");
> +		return;
> +	}
> +
> +	err("%s: %i", __func__, __LINE__);
> +
> +	if (getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &uc, &len) < 0)
> +		warn_perror("Can't get configuration client credentials");
> +
> +	c->fd_control = ref.fd = fd;
> +	rc = epoll_add(c->epollfd, EPOLLIN | EPOLLET, ref);
> +	if (rc < 0) {
> +		warn_perror("epoll_ctl() on configuration socket");
> +		goto fail;
> +	}
> +
> +	rc = write_all_buf(fd, &hello, sizeof(hello));
> +	if (rc < 0) {
> +		warn_perror("Error writing configuration protocol hello");
> +		goto fail;
> +	}
> +
> +	info("Accepted configuration client, PID %i", uc.pid);
> +	if (!PESTO_PROTOCOL_VERSION) {
> +		warn(
> +"Warning: Using experimental unsupported configuration protocol");
> +	}
> +
> +	return;
> +
> +fail:
> +	conf_close(c);
> +	goto retry;
> +}
> +
> +/**
> + * conf_handler() - Handle events on configuration socket
> + * @c:		Execution context
> + * @events:	epoll events
> + */
> +void conf_handler(struct ctx *c, uint32_t events)
> +{
> +	if (events & EPOLLIN) {
> +		char discard[BUFSIZ];
> +		ssize_t n;
> +
> +		do {
> +			n = read(c->fd_control, discard, sizeof(discard));
> +			if (n > 0)
> +				debug("Discarded %zd bytes of config data", n);
> +		} while (n > 0);
> +		if (n == 0) {
> +			debug("Configuration client EOF");
> +			goto close;
> +		}
> +		if (errno != EAGAIN && errno != EWOULDBLOCK) {
> +			err_perror("Error reading config data");
> +			goto close;
> +		}
> +	}
> +
> +	if (events & EPOLLHUP) {
> +		debug("Configuration client hangup");
> +		goto close;
> +	}
> +
> +	return;
> +
> +close:
> +	conf_close(c);
> +
> +	/* Check if any other clients are waiting to connect */
> +	conf_accept(c);
> +}
> diff --git a/conf.h b/conf.h
> index b45ad74..16f9718 100644
> --- a/conf.h
> +++ b/conf.h
> @@ -8,5 +8,7 @@
>   
>   enum passt_modes conf_mode(int argc, char *argv[]);
>   void conf(struct ctx *c, int argc, char **argv);
> +void conf_listen_handler(struct ctx *c, uint32_t events);
> +void conf_handler(struct ctx *c, uint32_t events);
>   
>   #endif /* CONF_H */
> diff --git a/epoll_type.h b/epoll_type.h
> index a90ffb6..061325a 100644
> --- a/epoll_type.h
> +++ b/epoll_type.h
> @@ -46,6 +46,10 @@ enum epoll_type {
>   	EPOLL_TYPE_REPAIR,
>   	/* Netlink neighbour subscription socket */
>   	EPOLL_TYPE_NL_NEIGH,
> +	/* Configuration listening socket */
> +	EPOLL_TYPE_CONF_LISTEN,
> +	/* Configuration socket */
> +	EPOLL_TYPE_CONF,
>   
>   	EPOLL_NUM_TYPES,
>   };
> diff --git a/passt.1 b/passt.1
> index 6303aeb..908fd4a 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -127,6 +127,11 @@ login name and group name can be passed. This requires privileges (either
>   initial effective UID 0 or CAP_SETUID capability) to work.
>   Default is to change to user \fInobody\fR if started as root.
>   
> +.TP
> +.BR \-c ", " \-\-conf-path " " \fIpath " " (EXPERIMENTAL)
> +Path for configuration and control socket used by \fBpesto\fR(1) to
> +dynamically update passt or pasta's configuration.
> +
>   .TP
>   .BR \-h ", " \-\-help
>   Display a help message and exit.
> diff --git a/passt.c b/passt.c
> index f84419c..bc42ea3 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -80,6 +80,8 @@ char *epoll_type_str[] = {
>   	[EPOLL_TYPE_REPAIR_LISTEN]	= "TCP_REPAIR helper listening socket",
>   	[EPOLL_TYPE_REPAIR]		= "TCP_REPAIR helper socket",
>   	[EPOLL_TYPE_NL_NEIGH]		= "netlink neighbour notifier socket",
> +	[EPOLL_TYPE_CONF_LISTEN]	= "configuration listening socket",
> +	[EPOLL_TYPE_CONF]		= "configuration socket",
>   };
>   static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
>   	      "epoll_type_str[] doesn't match enum epoll_type");
> @@ -303,6 +305,12 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
>   		case EPOLL_TYPE_NL_NEIGH:
>   			nl_neigh_notify_handler(c);
>   			break;
> +		case EPOLL_TYPE_CONF_LISTEN:
> +			conf_listen_handler(c, eventmask);
> +			break;
> +		case EPOLL_TYPE_CONF:
> +			conf_handler(c, eventmask);
> +			break;
>   		default:
>   			/* Can't happen */
>   			assert(0);
> diff --git a/passt.h b/passt.h
> index 62b8dcd..b3f049d 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -158,6 +158,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
> + * @control_path:	Path for control/configuration 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
> @@ -169,6 +170,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_control_listen:	Listening control/configuration socket, if any
> + * @fd_control:		Control/configuration socket, if any
>    * @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
> @@ -223,6 +226,7 @@ struct ctx {
>   	int foreground;
>   	int nofile;
>   	char sock_path[UNIX_PATH_MAX];
> +	char control_path[UNIX_PATH_MAX];
>   	char repair_path[UNIX_PATH_MAX];
>   	char pcap[PATH_MAX];
>   
> @@ -240,6 +244,8 @@ struct ctx {
>   	int epollfd;
>   	int fd_tap_listen;
>   	int fd_tap;
> +	int fd_control_listen;
> +	int fd_control;
>   	int fd_repair_listen;
>   	int fd_repair;
>   	unsigned char our_tap_mac[ETH_ALEN];
> diff --git a/pesto.c b/pesto.c
> index f0916e8..ab476c5 100644
> --- a/pesto.c
> +++ b/pesto.c
> @@ -33,6 +33,7 @@
>   
>   #include "common.h"
>   #include "seccomp_pesto.h"
> +#include "serialise.h"
>   #include "pesto.h"
>   #include "log.h"
>   
> @@ -66,6 +67,8 @@ static void usage(const char *name, FILE *f, int status)
>    *
>    * Return: 0 on success, won't return on failure
>    *
> + * #syscalls:pesto socket s390x:socketcall i686:socketcall
> + * #syscalls:pesto connect shutdown close
>    * #syscalls:pesto exit_group fstat read write
>    */
>   int main(int argc, char **argv)
> @@ -76,9 +79,12 @@ int main(int argc, char **argv)
>   		{"version",	no_argument,		NULL,		1 },
>   		{ 0 },
>   	};
> +	struct sockaddr_un a = { AF_UNIX, "" };
>   	const char *optstring = "dh";
> +	struct pesto_hello hello;
>   	struct sock_fprog prog;
> -	int optname;
> +	int optname, ret, s;
> +	uint32_t s_version;
>   
>   	prctl(PR_SET_DUMPABLE, 0);
>   
> @@ -122,5 +128,42 @@ int main(int argc, char **argv)
>   
>   	debug("debug_flag=%d, path=\"%s\"", debug_flag, argv[optind]);
>   
> -	die("pesto is not implemented yet");
> +	if ((s = socket(AF_UNIX, SOCK_STREAM, 0)) < 0)
> +		die_perror("Failed to create AF_UNIX socket");
> +
> +	ret = snprintf(a.sun_path, sizeof(a.sun_path), "%s", argv[optind]);
> +	if (ret <= 0 || ret >= (int)sizeof(a.sun_path))
> +		die("Invalid socket path \"%s\"", argv[optind]);
> +
> +	ret = connect(s, (struct sockaddr *)&a, sizeof(a));
> +	if (ret < 0) {
> +		die_perror("Failed to connect to %s", a.sun_path);
> +	}
> +
> +	ret = read_all_buf(s, &hello, sizeof(hello));
> +	if (ret < 0)
> +		die_perror("Couldn't read server greeting");
> +
> +	if (memcmp(hello.magic, PESTO_SERVER_MAGIC, sizeof(hello.magic)))
> +		die("Bad magic number from server");
> +
> +	s_version = ntohl(hello.version);
> +
> +	if (s_version > PESTO_PROTOCOL_VERSION) {
> +		die("Unknown server protocol version %"PRIu32" > %"PRIu32,
> +		    s_version, PESTO_PROTOCOL_VERSION);
> +	}
> +
> +	/* cppcheck-suppress knownConditionTrueFalse */
> +	if (!s_version) {
> +		if (PESTO_PROTOCOL_VERSION)
> +			die("Unsupported experimental server protocol");
> +		FPRINTF(stderr,
> +"Warning: Using experimental protocol version, client and server must match\n");
> +	}
> +
> +	if (shutdown(s, SHUT_RDWR) < 0 || close(s) < 0)
> +		die_perror("Error shutting down control socket");
> +
> +	exit(0);
>   }
> diff --git a/pesto.h b/pesto.h
> index e9b329f..3c93d3e 100644
> --- a/pesto.h
> +++ b/pesto.h
> @@ -9,4 +9,26 @@
>   #ifndef PESTO_H
>   #define PESTO_H
>   
> +#include <assert.h>
> +#include <stdint.h>
> +
> +#define PESTO_SERVER_MAGIC	"basil:s"
> +
> +/* Version 0 is reserved for unreleased / unsupported experimental versions */
> +#define PESTO_PROTOCOL_VERSION	1
> +
> +/**
> + * struct pesto_hello - Server introduction message
> + * @magic:	PESTO_SERVER_MAGIC
> + * @version:	Version number
> + */
> +struct pesto_hello {
> +	char magic[8];
> +	uint32_t version;
> +} __attribute__ ((__packed__));
> +
> +static_assert(sizeof(PESTO_SERVER_MAGIC)
> +	      == sizeof(((struct pesto_hello *)0)->magic),
> +	      "PESTO_SERVER_MAGIC has wrong size");
> +
>   #endif /* PESTO_H */
> diff --git a/serialise.c b/serialise.c
> index 944e741..346df99 100644
> --- a/serialise.c
> +++ b/serialise.c
> @@ -6,6 +6,9 @@
>    * PASTA - Pack A Subtle Tap Abstraction
>    *  for network namespace/tap device mode
>    *
> + * PESTO - Programmable Extensible Socket Translation Orchestrator
> + *  front-end for passt(1) and pasta(1) forwarding configuration
> + *
>    * serialise.c - Serialisation of data structures over bytestreams
>    *
>    * Copyright Red Hat