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

On Wed, Mar 25, 2026 at 01:56:22AM +0100, Stefano Brivio wrote:

On Mon, 23 Mar 2026 18:37:22 +1100 David Gibson 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 --- 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

Eh, I suppose.

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

...ah, those are good points.

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)

I really don't see any reason we'd want that, but I guess that's irrelevant given that previous paragraph.

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

I mean.. when we test the path in question, we should hit the seccomp filter, which should make it pretty obvious.

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

Yeah, fair.

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.

So, you've convinced me in principle, but I'm not putting this at the top of my priorities. Using malloc() makes things a bit easier while we're playing around with the protocol a bunch. Once we look reasonably close to a v1 protocol, I'll do the malloc() removal.

...

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

Ah, yes.

...

+ 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 #include #include +#include #include

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

That's fair. I'd basically already decided to move to fixed length pif names, just haven't implemented it yet.

...

+ 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

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