Re: [PATCH v3 15/25] pesto: Expose list of pifs to pesto

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 23 Mar 2026 18:37:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Extend the dynamic update protocol to expose the pif indices and names
> from a running passt/pasta to the pesto tool.  pesto records that data
> and (for now) prints it out.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c      | 38 +++++++++++++++++++++
>  pesto.c     | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  serialise.c | 21 ++++++++++++
>  serialise.h |  3 ++
>  4 files changed, 159 insertions(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index 7b960fe9..b878db94 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -2303,6 +2303,41 @@ void conf(struct ctx *c, int argc, char **argv)
>  		conf_print(c);
>  }
>  
> +/**
> + * conf_send_pifs() - Send list of pifs to dynamic update client (pesto)
> + * @c:		Execution context
> + * @fd:		Socket to the client
> + *
> + * Return: 0 on success, -1 on failure
> + */
> +static int conf_send_pifs(const struct ctx *c, int fd)
> +{
> +	uint32_t num = 0;
> +	unsigned pif;
> +
> +	/* First count the number of pifs with tables */
> +	for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> +		if (c->fwd[pif])
> +			num++;
> +	}
> +
> +	if (write_u32(fd, num))
> +		return -1;
> +
> +	for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> +		if (!c->fwd[pif])
> +			continue;
> +
> +		if (write_u8(fd, pif))
> +			return -1;
> +
> +		if (write_str(fd, pif_name(pif)) < 0)
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * conf_listen_handler() - Handle events on configuration listening socket
>   * @c:		Execution context
> @@ -2359,6 +2394,9 @@ void conf_listen_handler(struct ctx *c, uint32_t events)
>  "Warning: Using experimental unsupported configuration protocol");
>  	}
>  
> +	if (conf_send_pifs(c, fd) < 0)
> +		goto fail;
> +
>  	return;
>  
>  fail:
> diff --git a/pesto.c b/pesto.c
> index f8c9e01d..ed4f2aab 100644
> --- a/pesto.c
> +++ b/pesto.c
> @@ -36,6 +36,8 @@
>  #include "serialise.h"
>  #include "pesto.h"
>  
> +static int verbosity = 1;
> +
>  #define die(...)							\
>  	do {								\
>  		FPRINTF(stderr, __VA_ARGS__);				\
> @@ -51,6 +53,19 @@
>  		}							\
>  	} while (0)
>  
> +/**
> + * xmalloc() - Allocate memory, with fatal error on failure
> + * @size:	Number of bytes to allocate
> + */
> +static void *xmalloc(size_t size)
> +{
> +	void *p = malloc(size);

I would really really prefer to skip this unless it's absolutely
necessary, for a number of reasons:

1. this thing is talking to passt which talks to untrusted workloads,
   and there's a risk of arbitrary code execution, which implies the
   possibility of specially crafted messages that might make us
   allocate:

   a. in specific regions and build a heap spraying attack based on that

   b. a lot, and cause a denial of service

   ...and, while this process should be short lived, we can't assume
   that, if somebody achieves arbitrary code execution in passt and
   malicious messages, because we risk looping on rules, or having
   passt send us information very slowly (minutes / hours), etc.

2. besides, we might want to add a "daemon" mode one day, and then it's
   not short lived anymore, which opens the door to leaks and
   double-frees (adding to a. and b. above)

3. we might need to reuse functions from passt and not notice right
   away that they call this xmalloc(), or have to eventually adapt them
   anyway

4. consistency, in messaging ("passt doesn't allocate dynamic memory",
   without caveats) and for auditing reasons

Strings can be 32 bytes, or 1024 if we want to splurge. Unless we need
to pile a lot of them on the stack (but it's not the case in pesto) a
malloc() doesn't really bring any advantage compared to static buffers
here and there. It's not megabytes.

> +
> +	if (!p)
> +		die("Memory allocation failure");
> +	return p;
> +}
> +
>  /**
>   * usage() - Print usage, exit with given status code
>   * @name:	Executable name
> @@ -69,6 +84,83 @@ static void usage(const char *name, FILE *f, int status)
>  	exit(status);
>  }
>  
> +/**
> + * pesto_recv_str() - Receive a string from passt/pasta
> + * @fd:		Control socket
> + *
> + * Return: pointer to malloc()ed string
> + */
> +static const char *pesto_recv_str(int fd)
> +{
> +	uint32_t len;
> +	char *buf;
> +
> +	if (read_u32(fd, &len) < 0)
> +		die("Error reading from control socket");
> +
> +	buf = xmalloc(len);
> +	if (read_all_buf(fd, buf, len) < 0)
> +		die("Error reading from control socket");
> +
> +	return buf;
> +}
> +
> +struct pif_state {
> +	uint8_t pif;
> +	const char *name;
> +};
> +
> +struct conf_state {

More as a to-do list item rather than as a review comment: we need
documentation for those structs.

> +	uint32_t npifs;
> +	struct pif_state pif[];
> +};
> +
> +/**
> + * pesto_read_pifs() - Read pif names and IDs from passt/pasta
> + * @fd:		Control socket
> + */
> +static const struct conf_state *pesto_read_pifs(int fd)
> +{
> +	uint32_t num;
> +	struct conf_state *state;
> +	unsigned i;
> +
> +	if (read_u32(fd, &num) < 0)
> +		die("Error reading from control socket");
> +
> +	debug("Receiving %"PRIu32" interface names", num);
> +
> +	state = xmalloc(sizeof(*state) + num * sizeof(struct pif_state));
> +	state->npifs = num;
> +
> +	for (i = 0; i < num; i++) {
> +		struct pif_state *ps = &state->pif[i];
> +
> +		if (read_u8(fd, &ps->pif) < 0)
> +			die("Error reading from control socket");
> +		ps->name = pesto_recv_str(fd);
> +
> +		debug("%u: %s", ps->pif, ps->name);
> +	}
> +
> +	return state;
> +}
> +
> +/**
> + * show_state() - Show current rule state obtained from passt/pasta
> + * @pifs:	PIF name information
> + */
> +static void show_state(const struct conf_state *state)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < state->npifs; i++) {
> +		const struct pif_state *ps = &state->pif[i];
> +		printf("Forwarding rules for %s interface\n", ps->name);
> +		printf("\tTBD\n");
> +	}
> +}
> +
>  /**
>   * main() - Entry point and whole program with loop
>   * @argc:	Argument count
> @@ -91,12 +183,12 @@ int main(int argc, char **argv)
>  		{ 0 },
>  	};
>  	struct sockaddr_un a = { AF_UNIX, "" };
> +	const struct conf_state *state;
>  	const char *optstring = "vh";
>  	struct pesto_hello hello;
>  	struct sock_fprog prog;
>  	int optname, ret, s;
>  	uint32_t s_version;
> -	int verbosity = 1;
>  
>  	prog.len = (unsigned short)sizeof(filter_pesto) /
>  				   sizeof(filter_pesto[0]);
> @@ -172,5 +264,9 @@ int main(int argc, char **argv)
>  "Warning: Using experimental protocol version, client and server must match\n");
>  	}
>  
> +	state = pesto_read_pifs(s);
> +
> +	show_state(state);
> +
>  	exit(0);
>  }
> diff --git a/serialise.c b/serialise.c
> index e3ea86e3..94c34d3c 100644
> --- a/serialise.c
> +++ b/serialise.c
> @@ -19,6 +19,7 @@
>  #include <endian.h>
>  #include <errno.h>
>  #include <stdint.h>
> +#include <string.h>
>  #include <unistd.h>
>  
>  #include "serialise.h"
> @@ -121,6 +122,26 @@ int write_all_buf(int fd, const void *buf, size_t len)
>  		return write_all_buf(fd, &beval, sizeof(beval));	\
>  	}
>  
> +#define be8toh(x)	(x)
> +#define htobe8(x)	(x)
> +
> +SERIALISE_UINT(8)
>  SERIALISE_UINT(32)
>  
>  #undef SERIALISE_UNIT
> +
> +/**
> + * write_str() - Write a string to an fd in length/value format
> + * @fd:		Socket to the client
> + * @s:		String to send
> + *
> + * Return: 0 on success, -1 on error
> + */
> +int write_str(int fd, const char *s)
> +{
> +	uint32_t len = strlen(s) + 1; /* Include \0 */
> +
> +	if (write_u32(fd, len) < 0)
> +		return -1;

Even if we don't need to allocate here, I would feel more comfortable
if we passed fixed-size strings only to passt to have a slightly lower
risk of buffer overflows, in general.

> +	return write_all_buf(fd, s, len);
> +}
> diff --git a/serialise.h b/serialise.h
> index 0a4ed086..7ef35786 100644
> --- a/serialise.h
> +++ b/serialise.h
> @@ -16,6 +16,9 @@ int write_all_buf(int fd, const void *buf, size_t len);
>  	int read_u##bits(int fd, uint##bits##_t *val);			\
>  	int write_u##bits(int fd, uint##bits##_t val);
>  
> +SERIALISE_UINT_DECL(8)
>  SERIALISE_UINT_DECL(32)
>  
> +int write_str(int fd, const char *s);
> +
>  #endif /* _SERIALISE_H */

-- 
Stefano