[PATCH v11 00/23] Dynamic configuration update implementation
Changes in v11: * Drop debugging left-overs in 10/23, reported by Paul * In 9/23, don't declare argv as const argument for conf_pasta_ns(), because some versions of gcc (perhaps depending on the glibc version?), at least gcc 16.0.1 from Fedora Rawhide, are not happy with that. Suppress the cppcheck warning instead Changes in v10: * For some reason, changes in 9/23 now trigger seemingly unrelated, but valid, cppcheck warnings: fix them directly there * In 19/23, only consider exact matches for rules we're deleting, report an error if there are conflicts that are not exact matches. Further, address (other) comments by Laurent: a typo in the man page, a typo in a comment in fwd_rule_del(), and a serious issue in pesto's main where we would use the "inbound" table for -T / -U Changes in v9: * Rework Makefile changes and solve conflicts so that we can drop the dependency on "Improvements to static checker invocation" * In 8/23, drop the "experimental" note from the man page * In 10/23, switch to protocol version 1, add basil to the magic sauce * In 11/23, initialise struct pesto_pif_info sent by the server (details in commit message) * In 15/23, add description for -s / --show to pesto.1 as well * In 18/23, make comments about redundant checks more verbose * In 19/23, make it clear that tables handled by fwd_rule_del() can't refer to any open socket, add a TODO to fwd_rule_clear() in that sense as well, and use pif_conf_by_name() in pesto to find the table we need to clear * Add 19/23 to 23/23 (LSM policies, packaging stuff) to make pesto ready for shipping Changes in v8: * Implement --add, --delete, and --clear in 19/19, to add forwarding rules instead of replacing tables, delete existing rules, and explicitly clear tables * Address Laurent's comments for 15/19 and 17/19 * In 10/19, instead of passing SOCK_NONBLOCK to accept4(), explicitly set O_NONBLOCK on the listening socket. Using SOCK_NONBLOCK doesn't do what we want, as it results in setting O_NONBLOCK on the new socket rather than on the listening one * Note: 18/19 is left as it is, I didn't address pending comments yet * Note: this doesn't include yet changes for AppArmor and SELinux policies, as well as changes for the template Fedora spec file. I'm still working on them Changes in v7: * Addressed comments from Laurent in 6/18, 8/18, 9/18, 10/18, 11/18, 12/18, 14/18, 15/18 (details in commit messages of single patches, before my Signed-off-by) * Note: this doesn't include yet --add and --delete, I'm still working on that Changes in v6: * Addressed comments from Jon in 10/18, 11/18, 14/18, and 16/18 * Dodged all warnings from static checkers (Coverity Scan and clang-tidy) with changes in 10/18, 11/18, 16/18, and with a new patch, 18/18 * This does *not* include yet the implementation of --add and --delete switches for pesto as I originally intended, I'm rather far from being done with those. At the moment I just have a "mode selection" implementation for command line parsing but merging rules to / removing rules from / clearing the current table is something I barely started (and what I have at the moment isn't really valuable anyway) David wrote: --- Here's the next draft of dynamic configuration updates. This now can successfully update rules, though I've not tested it very extensively. Patches 1..8/18 are preliminary reworks that make sense even without pesto - feel free to apply if you're happy with them. I don't think the rest should be applied yet; we need to at least harden it so passt can't be blocked indefinitely by a client which sends a partial update then waits. Based on my earlier series reworking static checking invocation. TODO: - Don't allow a client which sends a partial configuration then blocks also block passt - Allow pesto to clear existing configuration, not just add - Allow pesto selectively delete existing rules, not just add Changes in v5: * If multiple clients connect at once, they're now blocked until the first one finishes, instead of later ones being discarded Changes in v4: * Merged with remainder of forward rule parsing rework series * Fix some bugs in rule checking pointed out by Laurent * Significantly cleaned up option parsing code * Changed from replacing all existing rules to adding new rules (clear and remove still TBD) * Somewhat simplified protocol (pif names and rules sent in a single pass) * pesto is now allocation free * Fixed commit message and style nits pointed out by Stefano Changes in v3: * Removed already applied ASSERT() rename * Renamed serialisation functions * Incorporated Stefano's extensions, reworked and fixed * Several additional cleanups / preliminary reworks Changes in v2: * Removed already applied cleanups * Reworked assert() patch to handle -DNDEBUG properly * Numerous extra patches: * Factored out serialisation helpers and use them for migration as well * Reworked to allow ip.[ch] and inany.[ch] to be shared with pesto * Reworks to share some forwarding rule datatypes with pesto * Implemented sending pif names and current ruleset to pesto --- David Gibson (17): conf, fwd: Stricter rule checking in fwd_rule_add() fwd_rule: Move ephemeral port probing to fwd_rule.c fwd, conf: Move rule parsing code to fwd_rule.[ch] fwd_rule: Move conflict checking back within fwd_rule_add() fwd: Generalise fwd_rules_info() pif: Limit pif names to 128 bytes fwd_rule: Fix some format specifiers pesto: Introduce stub configuration tool pesto, log: Share log.h (but not log.c) with pesto tool pesto, conf: Have pesto connect to passt and check versions pesto: Expose list of pifs to pesto and display them ip: Prepare ip.[ch] for sharing with pesto tool inany: Prepare inany.[ch] for sharing with pesto tool pesto: Read current ruleset from passt/pasta and optionally display it pesto: Parse and add new rules from command line pesto, conf: Send updated rules from pesto back to passt/pasta conf, fwd: Allow switching to new rules received from pesto Stefano Brivio (6): fwd_rule: Fix static checkers warnings in fwd_rule_add() pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules apparmor: Add policy file for pesto selinux: Add file context and type enforcement for pesto fedora: Install pesto, its SELinux policy, and the man page from the spec file hooks: Copy static build of pesto and related man page to server .gitignore | 2 + Makefile | 35 +- common.h | 116 ++++++ conf.c | 696 ++++++++++++++------------------ conf.h | 2 + contrib/apparmor/usr.bin.pesto | 23 ++ contrib/fedora/passt.spec | 14 +- contrib/selinux/pesto.fc | 11 + contrib/selinux/pesto.te | 95 +++++ epoll_type.h | 4 + flow.c | 4 +- fwd.c | 169 ++------ fwd.h | 41 +- fwd_rule.c | 705 +++++++++++++++++++++++++++++++-- fwd_rule.h | 68 +++- hooks/pre-push | 1 + inany.c | 19 +- inany.h | 17 +- ip.c | 56 +-- ip.h | 4 +- lineread.c | 2 +- log.h | 53 ++- passt.1 | 5 + passt.c | 8 + passt.h | 8 + pasta.c | 4 +- pesto.1 | 275 +++++++++++++ pesto.c | 522 ++++++++++++++++++++++++ pesto.h | 54 +++ pif.c | 2 +- pif.h | 7 +- serialise.c | 7 + serialise.h | 1 + siphash.h | 13 + tap.c | 64 ++- util.h | 110 +---- 36 files changed, 2419 insertions(+), 798 deletions(-) create mode 100644 common.h create mode 100644 contrib/apparmor/usr.bin.pesto create mode 100644 contrib/selinux/pesto.fc create mode 100644 contrib/selinux/pesto.te create mode 100644 pesto.1 create mode 100644 pesto.c create mode 100644 pesto.h -- 2.43.0
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
From: David Gibson
The new checks are actually sufficient but not enough for Coverity
Scan. Now that fwd->sock_count and new->last are affected or supplied
by clients, we need explicit (albeit redundant) checks on them.
Signed-off-by: Stefano Brivio
Instead of just being able to add to the existing tables, implement
an explicit --clear option to replace them, which now becomes the
default behaviour, and implement explicit --add and --delete options
to maintain the table and add or delete specific ports.
The option --clear PIF forces the clearing of a table, instead.
These options can be combined arbitrarily and are handled as
sequential commands, as now described in pesto(1).
If no option is given before forwarding specifiers for a matching
table, the command line is interpreted as a replacement of the
existing rules.
To this end:
- there's no protocol change, as pesto is anyway sending updated
copies of the table
- the forwarding table functions now include a new fwd_rule_del(),
which deletes existing rule only if a matching one is found
- a trivial fwd_rule_clear() is factored out from the existing
conf_handler() implementation, so that it can be directly used
in pesto
The entry points for parsing of port specifiers now take an additional
'del' parameter which is passed down all the way before reaching the
fwd_rule_add() implementation. If a rule should be deleted, at that
point, fwd_rule_del() is called instead.
Signed-off-by: Stefano Brivio
It needs to connect to passt and pasta, whether they're started as
root or not, and the control socket can be anywhere.
Signed-off-by: Stefano Brivio
Loosely inspired by passt-repair's policy: pesto needs to be able to
run, check networking entries under /proc (for ip_local_port_range),
talk to passt and pasta, wherever the control socket is.
Signed-off-by: Stefano Brivio
It's time to ship it in packages.
Signed-off-by: Stefano Brivio
Signed-off-by: Stefano Brivio
On Wed, May 06, 2026 at 11:31:51PM +0200, Stefano Brivio wrote:
Instead of just being able to add to the existing tables, implement an explicit --clear option to replace them, which now becomes the default behaviour, and implement explicit --add and --delete options to maintain the table and add or delete specific ports.
The option --clear PIF forces the clearing of a table, instead.
These options can be combined arbitrarily and are handled as sequential commands, as now described in pesto(1).
If no option is given before forwarding specifiers for a matching table, the command line is interpreted as a replacement of the existing rules.
To this end:
- there's no protocol change, as pesto is anyway sending updated copies of the table
- the forwarding table functions now include a new fwd_rule_del(), which deletes existing rule only if a matching one is found
- a trivial fwd_rule_clear() is factored out from the existing conf_handler() implementation, so that it can be directly used in pesto
The entry points for parsing of port specifiers now take an additional 'del' parameter which is passed down all the way before reaching the fwd_rule_add() implementation. If a rule should be deleted, at that point, fwd_rule_del() is called instead.
Signed-off-by: Stefano Brivio
Reviewed-by: Laurent Vivier
Reviewed-by: David Gibson
+/** + * fwd_rule_match() - Test if two rules exactly match each other + * @a: Rule to check against @b + * @b: Rule to check against @a + * + * Return: true if rules match exactly, false otherwise + */ +static bool fwd_rule_match(const struct fwd_rule *a, const struct fwd_rule *b)
This needs a different name to avoid confusion with the fwd_rule_match() that's static in fwd.c (which matches a *packet* against a rule, rather than a rule against another rule). I'd suggest fwd_rule_equal().
+{ + return !memcmp(a, b, sizeof(*a));
I think we should explicitly test for equality of each field, so that this is robust if the structure ends up with any padding. We probably also want to explicitly ignore FWD_WEAK. Won't hurt us for now, since passt and pesto should always set the same value for WEAK, but I think it makes sense.
+} + +/** + * fwd_rule_clear() - Clear a forwarding table + * @fwd: Table to clear (might be NULL) + */ +void fwd_rule_clear(struct fwd_table *fwd) +{ + if (!fwd) + return; + + /* TODO: check that there are no open sockets in the table before + * going on. See also a related item in fwd_rule_del(). + */ + + fwd->count = 0; + fwd->sock_count = 0; +} + +/** + * fwd_rule_del() - Partially validate and delete a rule from a forwarding table + * @fwd: Table to delete from + * @rule: Rule to delete (must conflict with an existing rule) + * + * Return: 0 on success, negative error code on failure (-ENOENT if not found) + * + * NOTE: This function can't be used for a forwarding table with any open socket + * stored in fwd->rulesocks. + */ +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *rule) +{ + char rulestr[FWD_RULE_STRLEN], oldstr[FWD_RULE_STRLEN]; + unsigned num, i; + + for (i = 0; i < fwd->count; i++) { + if (fwd_rule_match(rule, &fwd->rules[i])) + break; + + if (fwd_rule_conflicts(rule, &fwd->rules[i])) { + warn( +"Specifier %s conflicts with rule %s, but doesn't match it, can't delete", + fwd_rule_fmt(rule, rulestr, sizeof(rulestr)), + fwd_rule_fmt(&fwd->rules[i], oldstr, sizeof(oldstr))); + return -EINVAL;
"%s overlaps with %s but isn't equal, can't delete" might be a slightly clearer wording?
+ } + } + + if (i == fwd->count) { + warn("Couldn't find forwarding rule to delete: %s", + fwd_rule_fmt(rule, rulestr, sizeof(rulestr))); + return -ENOENT; + } + + /* Don't use anything else from 'rule' as passed, it's not validated */ + rule = &fwd->rules[i]; + num = (unsigned)rule->last - rule->first + 1; + + fwd->count--; + + memmove((void *)(fwd->rulesocks + i), (void *)(fwd->rulesocks + i + 1), + (fwd->count - i) * sizeof(*fwd->rulesocks)); + + /* TODO: move sockets stored starting from fwd->rulesocks[i + 1], should + * we ever need to delete rules from a table with open sockets. + */ + fwd->sock_count -= num; + + memmove(fwd->rules + i, fwd->rules + i + 1, + (fwd->count - i) * sizeof(*fwd->rules)); + + return 0; +} + /** * fwd_rule_add() - Validate and add a rule to a forwarding table * @fwd: Table to add to @@ -370,6 +453,7 @@ static int parse_keyword(const char *s, const char **endptr, const char *kw) * fwd_rule_range_except() - Set up forwarding for a range of ports minus a * bitmap of exclusions * @fwd: Forwarding table to be updated + * @del: Delete resulting rules from forwarding table, instead of adding * @proto: Protocol to forward * @addr: Listening address * @ifname: Listening interface @@ -379,8 +463,8 @@ static int parse_keyword(const char *s, const char **endptr, const char *kw) * @to: Port to translate @first to when forwarding * @flags: Flags for forwarding entries */ -static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto, - const union inany_addr *addr, +static void fwd_rule_range_except(struct fwd_table *fwd, bool del, + uint8_t proto, const union inany_addr *addr, const char *ifname, uint16_t first, uint16_t last, const uint8_t *exclude, uint16_t to, @@ -420,15 +504,20 @@ static void fwd_rule_range_except(struct fwd_table *fwd, uint8_t proto, rule.last = i - 1; rule.to = base + delta;
- if (fwd_rule_add(fwd, &rule) < 0) - goto fail; + if (del) { + if (fwd_rule_del(fwd, &rule) < 0) + goto fail; + } else { + if (fwd_rule_add(fwd, &rule) < 0) + goto fail; + }
base = i - 1; } return;
fail: - die("Unable to add rule %s", + die("Unable to %s rule %s", del ? "delete" : "add", fwd_rule_fmt(&rule, rulestr, sizeof(rulestr))); }
@@ -447,12 +536,13 @@ fail: /** * fwd_rule_parse_ports() - Parse port range(s) specifier * @fwd: Forwarding table to be updated + * @del: Delete resulting rules from forwarding table, instead of adding * @proto: Protocol to forward * @addr: Listening address for forwarding * @ifname: Interface name for listening * @spec: Port range(s) specifier */ -static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto, +static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, const union inany_addr *addr, const char *ifname, const char *spec) @@ -509,7 +599,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto, /* Exclude ephemeral ports */ fwd_port_map_ephemeral(exclude);
- fwd_rule_range_except(fwd, proto, addr, ifname, + fwd_rule_range_except(fwd, del, proto, addr, ifname, 1, NUM_PORTS - 1, exclude, 1, flags | FWD_WEAK); return; @@ -539,7 +629,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, uint8_t proto, if (p != ep) /* Garbage after the ranges */ goto bad;
- fwd_rule_range_except(fwd, proto, addr, ifname, + fwd_rule_range_except(fwd, del, proto, addr, ifname, orig_range.first, orig_range.last, exclude, mapped_range.first, flags); @@ -553,10 +643,12 @@ bad: /** * fwd_rule_parse() - Parse port configuration option * @optname: Short option name, t, T, u, or U + * @del: Delete resulting rules from forwarding table, instead of adding * @optarg: Option argument (port specification) * @fwd: Forwarding table to be updated */ -void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd) +void fwd_rule_parse(char optname, bool del, const char *optarg, + struct fwd_table *fwd) { union inany_addr addr_buf = inany_any6, *addr = &addr_buf; char buf[BUFSIZ], *spec, *ifname = NULL; @@ -634,12 +726,12 @@ void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd) optname, optarg);
if (fwd->caps & FWD_CAP_IPV4) { - fwd_rule_parse_ports(fwd, proto, + fwd_rule_parse_ports(fwd, del, proto, &inany_loopback4, NULL, spec); } if (fwd->caps & FWD_CAP_IPV6) { - fwd_rule_parse_ports(fwd, proto, + fwd_rule_parse_ports(fwd, del, proto, &inany_loopback6, NULL, spec); } @@ -655,7 +747,7 @@ void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd) optname, optarg); }
- fwd_rule_parse_ports(fwd, proto, addr, ifname, spec); + fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec); }
/** diff --git a/fwd_rule.h b/fwd_rule.h index f43b37d..ae9a3cb 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -100,9 +100,11 @@ void fwd_probe_ephemeral(void);
const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule); const char *fwd_rule_fmt(const struct fwd_rule *rule, char *dst, size_t size); -void fwd_rule_parse(char optname, const char *optarg, struct fwd_table *fwd); +void fwd_rule_parse(char optname, bool del, const char *optarg, + struct fwd_table *fwd); int fwd_rule_read(int fd, struct fwd_rule *rule); int fwd_rule_write(int fd, const struct fwd_rule *rule); +void fwd_rule_clear(struct fwd_table *fwd); int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new);
/** diff --git a/pesto.1 b/pesto.1 index 1e1c0f3..c13a18e 100644 --- a/pesto.1 +++ b/pesto.1 @@ -35,6 +35,42 @@ Display a help message and exit. .BR \-s ", " \-\-show Show the forwarding configuration before and after changes are applied.
+.TP +.BR \-A ", " \-\-add +Add the port forwarding specifiers following this option to the current +forwarding table, rather than replacing it. + +This option can be given multiple times, as it might follow previous deletions +(see \fB--delete\fR below), and implies that all the specifiers following it, +before a further \fB--delete\fR option occurs, will be handled as additions. + +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\fR for +more details. + +.TP +.BR \-D ", " \-\-delete +Delete the port forwarding specifiers following this option from the current +forwarding table, rather than adding them to it. + +This option can be given multiple times, as it might follow previous additions +(see \fB--add\fR above), and implies that all the specifiers following it, +before a further \fB--add\fR option occurs, will be handled as deletions. + +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\fR for +more details. + +.TP +.BR \-C ", " \-\-clear " " \fIpif +Clear the forwarding table associated to a given \fIpif\fR, that is, a +conceptual type of interface in \fBpasst\fR(1) or \fBpasta\fR(1) representing a +specific data path and direction. + +The available \fIpif\fR names can be obtained by querying the current forwarding +configuration, which can be done by calling \fBpesto\fR(1) without options. + +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\fR for +more details. + .TP .BR \-t ", " \-\-tcp-ports " " \fIspec Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of: @@ -166,6 +202,55 @@ Configure UDP port forwarding from target namespace to init namespace. .BR \-\-version Show version and exit.
+.SH NOTES + +.SS Adding, deleting, clearing rules + +The options \fB--add\fR, \fB--delete\fR, and \fB--clear\fR are handled as +sequential commands to manipulate the current forwarding tables. If none of them +is given, forwarding specifiers for a given table are intended as replacement of +the corresponding table. That is: + +.nf + pesto -t 1024 -U 1025 +.fi + +will \fBreplace\fR the current TCP inbound port forwarding table with a single +rule, forwarding port 1024, and will similarly replace the UDP outbound +forwarding table with a single forwarding rule for port 1025. This usage is a +short-hand form for: + +.nf + pesto -C HOST -t 1024 -C SPLICE -U 1025 +.fi + +The options \fB--add\fR and \fB--delete\fR are used to \fBadd new specific +rules or delete existing ones\fR, instead of replacing tables. For example: + +.nf + pesto -A -t 2000 -D -t 3000 -U 5000 +.fi + +will add a forwarding rule for inbound TCP port 2000, and delete inbound TCP +port 3000 as well as outbound UDP port 5000 from the existing set of rules. + +All these options are interpreted as sequential commands and can be arbitrarily +combined. For example: + +.nf + pesto -A -t 2000 -C HOST -A -T 3000 -t 2001 -D -u 5000 +.fi + +will, in order: + +.RS +- add inbound TCP port 2000 +- clear inbound ports, reverting the addition above +- add outbound TCP port 3000 +- add inbound TCP port 2001 +- delete inbound UDP port 5000 +.RE + .SH AUTHORS
Stefano Brivio
, diff --git a/pesto.c b/pesto.c index 73fdc39..f4d752b 100644 --- a/pesto.c +++ b/pesto.c @@ -55,6 +55,9 @@ static void usage(const char *name, FILE *f, int status) FPRINTF(f, "Usage: %s [OPTION]... PATH\n", name); FPRINTF(f, "\n" + " -A, --add Add following specifiers to forwards\n" + " -D, --delete Delete following specifiers instead\n" + " -C, --clear PIF Clear forwarding table for given PIF\n" " -t, --tcp-ports SPEC TCP inbound port forwarding\n" " can be specified multiple times\n" " SPEC can be:\n" @@ -298,6 +301,9 @@ int main(int argc, char **argv) {"debug", no_argument, NULL, 'd' }, {"help", no_argument, NULL, 'h' }, {"version", no_argument, NULL, 1 }, + {"add", no_argument, NULL, 'A' }, + {"delete", no_argument, NULL, 'D' }, + {"clear", required_argument, NULL, 'C' }, {"tcp-ports", required_argument, NULL, 't' }, {"udp-ports", required_argument, NULL, 'u' }, {"tcp-ns", required_argument, NULL, 'T' }, @@ -305,9 +311,11 @@ int main(int argc, char **argv) {"show", no_argument, NULL, 's' }, { 0 }, }; + enum { MODE_CLEAR, MODE_ADD, MODE_DEL } mode = MODE_CLEAR; + bool inbound_cleared = false, outbound_cleared = false; struct pif_configuration *inbound, *outbound; + const char *optstring = "dhADC:t:u:T:U:s"; struct sockaddr_un a = { AF_UNIX, "" }; - const char *optstring = "dht:u:T:U:s"; struct configuration conf = { 0 }; bool update = false, show = false; struct pesto_hello hello; @@ -339,11 +347,16 @@ int main(int argc, char **argv) case -1: case 0: break; + case 'C': case 't': case 'u': case 'T': case 'U': - /* Parse these options after we've read state from passt/pasta */ + case 'A': + case 'D': + /* Parse these options after we've read state from + * passt/pasta + */ update = true; break; case 's': @@ -436,16 +449,43 @@ int main(int argc, char **argv) optind = 0; do { + struct pif_configuration *pif_to_clear; + optname = getopt_long(argc, argv, optstring, options, NULL);
switch (optname) { + case 'A': + mode = MODE_ADD; + break; + case 'D': + mode = MODE_DEL; + break; + case 'C': + if (!(pif_to_clear = pif_conf_by_name(&conf, optarg))) + die("Unsupported pif name %s", optarg); + + fwd_rule_clear(&pif_to_clear->fwd); + + if (!strcmp(optarg, "HOST")) + inbound_cleared = true; + else if (!strcmp(optarg, "SPLICE")) + outbound_cleared = true; + + break; case 't': case 'u': if (!inbound) { die("Can't use -%c, no inbound interface", optname); } - fwd_rule_parse(optname, optarg, &inbound->fwd); + + if (mode == MODE_CLEAR && !inbound_cleared) { + fwd_rule_clear(&inbound->fwd); + inbound_cleared = true; + } + + fwd_rule_parse(optname, mode == MODE_DEL, optarg, + &inbound->fwd); break; case 'T': case 'U': @@ -453,7 +493,14 @@ int main(int argc, char **argv) die("Can't use -%c, no outbound interface", optname); } - fwd_rule_parse(optname, optarg, &outbound->fwd); + + if (mode == MODE_CLEAR && !outbound_cleared) { + fwd_rule_clear(&outbound->fwd); + outbound_cleared = true; + } + + fwd_rule_parse(optname, mode == MODE_DEL, optarg, + &outbound->fwd); break; default: continue; -- 2.43.0
-- 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, May 06, 2026 at 11:31:41PM +0200, Stefano Brivio wrote:
From: David Gibson
In pesto we're going to want several levels of error/warning messages, much like passt itself. Particularly as we start to share mode code between passt and pesto, we want to use a similar interface to emit those. However we don't want to use the same implementation - logging to a file or syslog doesn't make sense for the command line tool.
To accomplish this loosely share log.h, but not log.c between pesto and passt. In fact, an #ifdef means even most of log.h isn't actually shared, but we do provide similar warn(), die() etc. macros.
This includes the *_perror() variants, which need strerror(). However, we want to avoid allocations for pesto as we do for passt, and strerror() allocates in some libc versions. Therefore, also move our workaround for this to be shared with pesto.
Reviewed-by: Laurent Vivier
[dwg: Based on changes part of a larger patch by Stefano] Signed-off-by: David Gibson [sbrivio: Dropped debug_perror_() as it's not used anyway, Laurent was asking about its name] [sbrivio: Fix conflicts in the Makefile caused by the fact that I'm not merging a previous series reworking it] [sbrivio: For some reason, this triggers some unrelated, but valid, cppcheck warnings in tap.c and conf.c: fix / suppress them]
Yeah, I've hit some of the same warnings appearing and disappearing
for no clear reason. The scope reduction warnings around debug() are
particularly nasty, because they could need to be suppressed in a
bunch of places. Oh well, this will do for now.
Reviewed-by: David Gibson
Signed-off-by: Stefano Brivio
--- Makefile | 10 +++++----- common.h | 32 ++++++++++++++++++++++++++++++++ conf.c | 4 +++- log.h | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++- pasta.c | 4 ++-- pesto.c | 14 ++++---------- tap.c | 12 +++++++++++- util.h | 32 -------------------------------- 8 files changed, 109 insertions(+), 52 deletions(-) diff --git a/Makefile b/Makefile index 76b9b5c..2639472 100644 --- a/Makefile +++ b/Makefile @@ -50,10 +50,10 @@ SRCS = $(PASST_SRCS) $(QRAP_SRCS) $(PASST_REPAIR_SRCS) $(PESTO_SRCS)
MANPAGES = passt.1 pasta.1 pesto.1 qrap.1 passt-repair.1
-PASST_HEADERS = arch.h arp.h bitmap.h checksum.h conf.h dhcp.h dhcpv6.h \ - epoll_ctl.h flow.h fwd.h fwd_rule.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 pesto.h pcap.h pif.h repair.h \ +PASST_HEADERS = arch.h arp.h bitmap.h checksum.h common.h conf.h dhcp.h \ + dhcpv6.h epoll_ctl.h flow.h fwd.h fwd_rule.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 pesto.h pif.h repair.h \ serialise.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 @@ -116,7 +116,7 @@ passt-repair: $(PASST_REPAIR_SRCS) seccomp_repair.h $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PASST_REPAIR_SRCS) -o passt-repair $(LDFLAGS)
pesto: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h - $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) $(PESTO_SRCS) -o pesto $(LDFLAGS) + $(CC) $(FLAGS) $(CFLAGS) $(CPPFLAGS) -DPESTO $(PESTO_SRCS) -o pesto $(LDFLAGS)
valgrind: EXTRA_SYSCALLS += rt_sigprocmask rt_sigtimedwait rt_sigaction \ rt_sigreturn getpid gettid kill clock_gettime \ diff --git a/common.h b/common.h index f3506b4..4251781 100644 --- a/common.h +++ b/common.h @@ -21,4 +21,36 @@ /* FPRINTF() intentionally silences cert-err33-c clang-tidy warnings */ #define FPRINTF(f, ...) (void)fprintf(f, __VA_ARGS__)
+/* + * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror, + * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs + * getrandom(2) and brk(2) as it allocates memory for the locale-translated + * error description, but our seccomp profiles forbid both. + * + * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get + * a static untranslated string. It's a GNU implementation, but also defined by + * bionic. + * + * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries + * not defining strerrordesc_np() are expected to provide strerror() + * implementations that are simple enough for us to call. + */ +__attribute__ ((weak)) const char *strerrordesc_np(int errnum); + +/** + * strerror_() - strerror() wrapper calling strerrordesc_np() if available + * @errnum: Error code + * + * Return: error description string + */ +static inline const char *strerror_(int errnum) +{ + if (strerrordesc_np) + return strerrordesc_np(errnum); + + return strerror(errnum); +} + +#define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" + #endif /* COMMON_H */ diff --git a/conf.c b/conf.c index 0586107..05e93db 100644 --- a/conf.c +++ b/conf.c @@ -308,7 +308,9 @@ static void conf_netns_opt(char *netns, const char *arg) * @argv: Command line arguments */ static void conf_pasta_ns(int *netns_only, char *userns, char *netns, - int optind, int argc, char *argv[]) + int optind, int argc, +/* cppcheck-suppress [constParameter, unmatchedSuppression] */ + char *argv[]) { if (*netns && optind != argc) die("Both --netns and PID or command given"); diff --git a/log.h b/log.h index dbab006..c6befe3 100644 --- a/log.h +++ b/log.h @@ -6,8 +6,57 @@ #ifndef LOG_H #define LOG_H
-#include
#include +#include +#include + +#ifdef PESTO + +#include + +#include "common.h" + +extern bool debug_flag; + +#define msg(...) \ + do { \ + FPRINTF(stderr, __VA_ARGS__); \ + FPRINTF(stderr, "\n"); \ + } while (0) + +#define msg_perror(...) \ + do { \ + int errno_ = errno; \ + FPRINTF(stderr, __VA_ARGS__); \ + FPRINTF(stderr, ": %s\n", strerror_(errno_)); \ + } while (0) + +#define die(...) \ + do { \ + msg(__VA_ARGS__); \ + exit(EXIT_FAILURE); \ + } while (0) + +#define die_perror(...) \ + do { \ + msg_perror(__VA_ARGS__); \ + exit(EXIT_FAILURE); \ + } while (0) + +#define warn(...) msg(__VA_ARGS__) +#define warn_perror(...) msg_perror(__VA_ARGS__) +#define info(...) msg(__VA_ARGS__) +#define info_perror(...) msg_perror(__VA_ARGS__) + +#define debug(...) \ + do { \ + if (debug_flag) \ + msg(__VA_ARGS__); \ + } while (0) + +#else /* !PESTO */ + +#include #include #include @@ -109,4 +158,6 @@ void __openlog(const char *ident, int option, int facility); void logfile_init(const char *name, const char *path, size_t size); void __setlogmask(int mask);
+#endif /* !PESTO */ + #endif /* LOG_H */ diff --git a/pasta.c b/pasta.c index bab945f..4e7ee54 100644 --- a/pasta.c +++ b/pasta.c @@ -521,7 +521,7 @@ void pasta_netns_quit_init(const struct ctx *c) * @c: Execution context * @inotify_fd: inotify file descriptor with watch on namespace directory */ -void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd) +void pasta_netns_quit_inotify_handler(const struct ctx *c, int inotify_fd) { char buf[sizeof(struct inotify_event) + NAME_MAX + 1] __attribute__ ((aligned(__alignof__(struct inotify_event)))); @@ -547,7 +547,7 @@ void pasta_netns_quit_inotify_handler(struct ctx *c, int inotify_fd) * @c: Execution context * @ref: epoll reference for timer descriptor */ -void pasta_netns_quit_timer_handler(struct ctx *c, union epoll_ref ref) +void pasta_netns_quit_timer_handler(const struct ctx *c, union epoll_ref ref) { uint64_t expirations; ssize_t n; diff --git a/pesto.c b/pesto.c index 9f2fa5d..f0916e8 100644 --- a/pesto.c +++ b/pesto.c @@ -34,18 +34,12 @@ #include "common.h" #include "seccomp_pesto.h" #include "pesto.h" +#include "log.h"
-static bool debug_flag = false; +bool debug_flag = false;
static char stdout_buf[BUFSIZ];
-#define die(...) \ - do { \ - FPRINTF(stderr, __VA_ARGS__); \ - FPRINTF(stderr, "\n"); \ - exit(EXIT_FAILURE); \ - } while (0) - /** * usage() - Print usage, exit with given status code * @name: Executable name @@ -99,7 +93,7 @@ int main(int argc, char **argv) * breaking our seccomp profile. */ if (setvbuf(stdout, stdout_buf, _IOFBF, sizeof(stdout_buf))) - die("Failed to set stdout buffer"); + die_perror("Failed to set stdout buffer");
do { optname = getopt_long(argc, argv, optstring, options, NULL); @@ -126,7 +120,7 @@ int main(int argc, char **argv) if (argc - optind != 1) usage(argv[0], stderr, EXIT_FAILURE);
- printf("debug_flag=%d, path=\"%s\"\n", debug_flag, argv[optind]); + debug("debug_flag=%d, path=\"%s\"", debug_flag, argv[optind]);
die("pesto is not implemented yet"); } diff --git a/tap.c b/tap.c index 7d06189..412f437 100644 --- a/tap.c +++ b/tap.c @@ -756,6 +756,11 @@ resume:
if (IN4_IS_ADDR_LOOPBACK(&iph->saddr) || IN4_IS_ADDR_LOOPBACK(&iph->daddr)) { + /* The scope of sstr and dstr could be in theory reduced + * into the conditional block debug() expands to, but + * it's awkward and unreadable, so ignore this warning. + */ + /* cppcheck-suppress [variableScope,unmatchedSuppression] */ char sstr[INET_ADDRSTRLEN], dstr[INET_ADDRSTRLEN];
debug("Loopback address on tap interface: %s -> %s", @@ -929,6 +934,11 @@ resume: continue;
if (IN6_IS_ADDR_LOOPBACK(saddr) || IN6_IS_ADDR_LOOPBACK(daddr)) { + /* The scope of sstr and dstr could be in theory reduced + * into the conditional block debug() expands to, but + * it's awkward and unreadable, so ignore this warning. + */ + /* cppcheck-suppress [variableScope,unmatchedSuppression] */ char sstr[INET6_ADDRSTRLEN], dstr[INET6_ADDRSTRLEN];
debug("Loopback address on tap interface: %s -> %s", @@ -1304,7 +1314,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events, * tap_backend_show_hints() - Give help information to start QEMU * @c: Execution context */ -static void tap_backend_show_hints(struct ctx *c) +static void tap_backend_show_hints(const struct ctx *c) { switch (c->mode) { case MODE_PASTA: diff --git a/util.h b/util.h index 770ff93..e90be47 100644 --- a/util.h +++ b/util.h @@ -302,38 +302,6 @@ static inline bool mod_between(unsigned x, unsigned i, unsigned j, unsigned m)
void raw_random(void *buf, size_t buflen);
-/* - * Starting from glibc 2.40.9000 and commit 25a5eb4010df ("string: strerror, - * strsignal cannot use buffer after dlmopen (bug 32026)"), strerror() needs - * getrandom(2) and brk(2) as it allocates memory for the locale-translated - * error description, but our seccomp profiles forbid both. - * - * Use the strerror_() wrapper instead, calling into strerrordesc_np() to get - * a static untranslated string. It's a GNU implementation, but also defined by - * bionic. - * - * If strerrordesc_np() is not defined (e.g. musl), call strerror(). C libraries - * not defining strerrordesc_np() are expected to provide strerror() - * implementations that are simple enough for us to call. - */ -__attribute__ ((weak)) const char *strerrordesc_np(int errnum); - -/** - * strerror_() - strerror() wrapper calling strerrordesc_np() if available - * @errnum: Error code - * - * Return: error description string - */ -static inline const char *strerror_(int errnum) -{ - if (strerrordesc_np) - return strerrordesc_np(errnum); - - return strerror(errnum); -} - -#define strerror(x) @ "Don't call strerror() directly, use strerror_() instead" - /* * Workarounds for https://github.com/llvm/llvm-project/issues/58992 * -- 2.43.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, 7 May 2026 09:51:10 +1000
David Gibson
On Wed, May 06, 2026 at 11:31:51PM +0200, Stefano Brivio wrote:
Instead of just being able to add to the existing tables, implement an explicit --clear option to replace them, which now becomes the default behaviour, and implement explicit --add and --delete options to maintain the table and add or delete specific ports.
The option --clear PIF forces the clearing of a table, instead.
These options can be combined arbitrarily and are handled as sequential commands, as now described in pesto(1).
If no option is given before forwarding specifiers for a matching table, the command line is interpreted as a replacement of the existing rules.
To this end:
- there's no protocol change, as pesto is anyway sending updated copies of the table
- the forwarding table functions now include a new fwd_rule_del(), which deletes existing rule only if a matching one is found
- a trivial fwd_rule_clear() is factored out from the existing conf_handler() implementation, so that it can be directly used in pesto
The entry points for parsing of port specifiers now take an additional 'del' parameter which is passed down all the way before reaching the fwd_rule_add() implementation. If a rule should be deleted, at that point, fwd_rule_del() is called instead.
Signed-off-by: Stefano Brivio
Reviewed-by: Laurent Vivier Reviewed-by: David Gibson
Several concerns below, but they can all be addressed as follow ups.
Just to set expectations: I won't take care of those, mostly because there are actual blocking issues (not with this series, they would also be follow up) that I'm trying to take care of instead, see e.g.: https://github.com/containers/container-libs/pull/755#issuecomment-439242731... so you'll need to follow up with patches, in case (and expect delays in reviews). -- Stefano
On Thu, May 07, 2026 at 04:10:33AM +0200, Stefano Brivio wrote:
On Thu, 7 May 2026 09:51:10 +1000 David Gibson
wrote: On Wed, May 06, 2026 at 11:31:51PM +0200, Stefano Brivio wrote:
Instead of just being able to add to the existing tables, implement an explicit --clear option to replace them, which now becomes the default behaviour, and implement explicit --add and --delete options to maintain the table and add or delete specific ports.
The option --clear PIF forces the clearing of a table, instead.
These options can be combined arbitrarily and are handled as sequential commands, as now described in pesto(1).
If no option is given before forwarding specifiers for a matching table, the command line is interpreted as a replacement of the existing rules.
To this end:
- there's no protocol change, as pesto is anyway sending updated copies of the table
- the forwarding table functions now include a new fwd_rule_del(), which deletes existing rule only if a matching one is found
- a trivial fwd_rule_clear() is factored out from the existing conf_handler() implementation, so that it can be directly used in pesto
The entry points for parsing of port specifiers now take an additional 'del' parameter which is passed down all the way before reaching the fwd_rule_add() implementation. If a rule should be deleted, at that point, fwd_rule_del() is called instead.
Signed-off-by: Stefano Brivio
Reviewed-by: Laurent Vivier Reviewed-by: David Gibson
Several concerns below, but they can all be addressed as follow ups.
Just to set expectations: I won't take care of those, mostly because there are actual blocking issues (not with this series, they would also be follow up) that I'm trying to take care of instead, see e.g.:
Makes sense.
https://github.com/containers/container-libs/pull/755#issuecomment-439242731...
so you'll need to follow up with patches, in case (and expect delays in reviews).
Yes, that's what I expected. -- 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
participants (2)
-
David Gibson
-
Stefano Brivio