From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=gL3Mo3zU; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id AE7AC5A0265 for ; Thu, 07 May 2026 01:55:00 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1778111697; bh=dW2/WH2tDPGHQEKyClyfEVex23gZyYcyrutnEwf2KGI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gL3Mo3zUMtHKtPdGrBLt0z0fywxrO2pYQrp4p3xQKeH3wkn0vOO7ETwvan9YpO0di g6MPoPoxvYRr1h3WoKkPAeeophl/7Od6dp2xeQ8n5QW0QkKFQ8KqAj8KdStplZlAdb aqoiS3DdErbwLxoa6xw4lcm7KJdmd3OW5uKUpdBwAc1+X/20ZkD88ZEQ/yp2oTGbYm H2IIscvZVJIU79oROxIIVLdc0BcH+2RkQm4UoraDi5/AxSCXESpJabweBtpHUj1hXm YDdoSB2LGTvcaU7JL6114up/NjYm1/1019YdkrBsCRPLNukzOdsOiKaCCcDtRCuGrD b/Vb87t17GDfQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4g9sgj1DTfz4wJs; Thu, 07 May 2026 09:54:57 +1000 (AEST) Date: Thu, 7 May 2026 09:51:10 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v11 19/23] pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules Message-ID: References: <20260506213155.1886983-1-sbrivio@redhat.com> <20260506213155.1886983-20-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="tVfuUdM2ubgrq4xV" Content-Disposition: inline In-Reply-To: <20260506213155.1886983-20-sbrivio@redhat.com> Message-ID-Hash: UPVMOLZOWBVXI7KXHFNLDGHTJHCALG7I X-Message-ID-Hash: UPVMOLZOWBVXI7KXHFNLDGHTJHCALG7I X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Jon Maloy , Laurent Vivier , Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --tVfuUdM2ubgrq4xV Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > The option --clear PIF forces the clearing of a table, instead. >=20 > These options can be combined arbitrarily and are handled as > sequential commands, as now described in pesto(1). >=20 > If no option is given before forwarding specifiers for a matching > table, the command line is interpreted as a replacement of the > existing rules. >=20 > To this end: >=20 > - there's no protocol change, as pesto is anyway sending updated > copies of the table >=20 > - the forwarding table functions now include a new fwd_rule_del(), > which deletes existing rule only if a matching one is found >=20 > - a trivial fwd_rule_clear() is factored out from the existing > conf_handler() implementation, so that it can be directly used > in pesto >=20 > 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. >=20 > 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. [snip] > +/** > + * 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_ru= le *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 =3D 0; > + fwd->sock_count =3D 0; > +} > + > +/** > + * fwd_rule_del() - Partially validate and delete a rule from a forwardi= ng 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 ope= n socket > + * stored in fwd->rulesocks. > + */ > +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *ru= le) > +{ > + char rulestr[FWD_RULE_STRLEN], oldstr[FWD_RULE_STRLEN]; > + unsigned num, i; > + > + for (i =3D 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 =3D=3D 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 =3D &fwd->rules[i]; > + num =3D (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 -=3D 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 minu= s 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 =3D i - 1; > rule.to =3D base + delta; > =20 > - 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; > + } > =20 > base =3D i - 1; > } > return; > =20 > fail: > - die("Unable to add rule %s", > + die("Unable to %s rule %s", del ? "delete" : "add", > fwd_rule_fmt(&rule, rulestr, sizeof(rulestr))); > } > =20 > @@ -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 *fw= d, uint8_t proto, > /* Exclude ephemeral ports */ > fwd_port_map_ephemeral(exclude); > =20 > - 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 *fw= d, uint8_t proto, > if (p !=3D ep) /* Garbage after the ranges */ > goto bad; > =20 > - 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 =3D inany_any6, *addr =3D &addr_buf; > char buf[BUFSIZ], *spec, *ifname =3D NULL; > @@ -634,12 +726,12 @@ void fwd_rule_parse(char optname, const char *optar= g, struct fwd_table *fwd) > optname, optarg); > =20 > 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); > } > =20 > - fwd_rule_parse_ports(fwd, proto, addr, ifname, spec); > + fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec); > } > =20 > /** > 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); > =20 > 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); > =20 > /** > 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. > =20 > +.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 del= etions > +(see \fB--delete\fR below), and implies that all the specifiers followin= g it, > +before a further \fB--delete\fR option occurs, will be handled as additi= ons. > + > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\f= R for > +more details. > + > +.TP > +.BR \-D ", " \-\-delete > +Delete the port forwarding specifiers following this option from the cur= rent > +forwarding table, rather than adding them to it. > + > +This option can be given multiple times, as it might follow previous add= itions > +(see \fB--add\fR above), and implies that all the specifiers following i= t, > +before a further \fB--add\fR option occurs, will be handled as deletions. > + > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\f= R 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) represe= nting a > +specific data path and direction. > + > +The available \fIpif\fR names can be obtained by querying the current fo= rwarding > +configuration, which can be done by calling \fBpesto\fR(1) without optio= ns. > + > +See the section \fBAdding, deleting, clearing rules\fR in the \fBNOTES\f= R for > +more details. > + > .TP > .BR \-t ", " \-\-tcp-ports " " \fIspec > Configure TCP port forwarding to guest or namespace. \fIspec\fR can be o= ne of: > @@ -166,6 +202,55 @@ Configure UDP port forwarding from target namespace = to init namespace. > .BR \-\-version > Show version and exit. > =20 > +.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 replac= ement 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 specif= ic > +rules or delete existing ones\fR, instead of replacing tables. For examp= le: > + > +.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 rul= es. > + > +All these options are interpreted as sequential commands and can be arbi= trarily > +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 > =20 > 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 =3D MODE_CLEAR; > + bool inbound_cleared =3D false, outbound_cleared =3D false; > struct pif_configuration *inbound, *outbound; > + const char *optstring =3D "dhADC:t:u:T:U:s"; > struct sockaddr_un a =3D { AF_UNIX, "" }; > - const char *optstring =3D "dht:u:T:U:s"; > struct configuration conf =3D { 0 }; > bool update =3D false, show =3D 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 =3D true; > break; > case 's': > @@ -436,16 +449,43 @@ int main(int argc, char **argv) > =20 > optind =3D 0; > do { > + struct pif_configuration *pif_to_clear; > + > optname =3D getopt_long(argc, argv, optstring, options, NULL); > =20 > switch (optname) { > + case 'A': > + mode =3D MODE_ADD; > + break; > + case 'D': > + mode =3D MODE_DEL; > + break; > + case 'C': > + if (!(pif_to_clear =3D 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 =3D true; > + else if (!strcmp(optarg, "SPLICE")) > + outbound_cleared =3D 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 =3D=3D MODE_CLEAR && !inbound_cleared) { > + fwd_rule_clear(&inbound->fwd); > + inbound_cleared =3D true; > + } > + > + fwd_rule_parse(optname, mode =3D=3D 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 =3D=3D MODE_CLEAR && !outbound_cleared) { > + fwd_rule_clear(&outbound->fwd); > + outbound_cleared =3D true; > + } > + > + fwd_rule_parse(optname, mode =3D=3D MODE_DEL, optarg, > + &outbound->fwd); > break; > default: > continue; > --=20 > 2.43.0 >=20 --=20 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 --tVfuUdM2ubgrq4xV Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmn70+QACgkQzQJF27ox 2GfJiQ/9EAjVxCDykaLZ2vFt5svNjIW7KecwN1yodH5Il/dDjXTMyt/kghtE/Bxo CPUykjc7+JuC1It/31Pc2vz1p9SKTylyvDYxV3shDJcXQxpZqkBsLUeZVNtJP4kk DfZV3mCWFQcAKPpeVNFCOBqVcGTDwvXqxNGmheLezbopEx4yszvgq3e/Zp7ij0EK sUf1H9G603M6qLi0Y1mWXB1UHHEqbsn/tLp/GMIIbGrNldC6T3lCU7crFGjtdgU9 bYn7n2YJjuCig5y5oCThsyNogFjUYklrOo9W2PHCkgHmAH8YzxCa+G9ksLxuIyjp ob3MZ/wN/BJkx5oszjE+RjNmkwS2PY2CLds33Oc06FZ0ayHT2TspkWFtcwt/nkJa rV6GAtyFFwzenBHdHS5lOgKSuez1/5EQLw4JIO5abzK1TBtKypvfA+Wer30a52MY +oE+DyH9/3iKsVCo5amoIgnwau8ebxlyA5EcSUm/iV3EsS9xcapNlY21vWBNoWo+ ilURUM97+qIDST08g4SS27naYrH9NaJFUazfxYb8g5CbyjvGmuZFxeOoPiigc+ug m457cJQsrKFfDCxLGvRcFaPYuz9iab0ewCNU4IK/E6Xb0GonrHk9n7T9eDvoDp9+ f/WSE7d0z5w/lDS85lPHxWy370JQ26m7UCnsGgdv/eZUMVdc10E= =Sl1F -----END PGP SIGNATURE----- --tVfuUdM2ubgrq4xV--