Re: [PATCH v8 19/19] pesto, conf, fwd_rule: Add options and modes to add, delete, clear rules

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 19969 bytes --]

On Wed, May 06, 2026 at 01:47:19AM +0200, Stefano Brivio wrote:
> Instead of just being able to replace the existing forwarding table,

As of my last version, we already added, rather than replacing.

> implement --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 <sbrivio@redhat.com>
> ---
>  conf.c     | 26 ++++++----------
>  fwd_rule.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  fwd_rule.h |  4 ++-
>  pesto.1    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  pesto.c    | 53 ++++++++++++++++++++++++++++---
>  5 files changed, 227 insertions(+), 32 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 3f48793..909c34c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1849,16 +1849,16 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  		if (name == 't') {
>  			opt_t = true;
> -			fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]);
> +			fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]);
>  		} else if (name == 'u') {
>  			opt_u = true;
> -			fwd_rule_parse(name, optarg, c->fwd[PIF_HOST]);
> +			fwd_rule_parse(name, false, optarg, c->fwd[PIF_HOST]);
>  		} else if (name == 'T') {
>  			opt_T = true;
> -			fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]);
> +			fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]);
>  		} else if (name == 'U') {
>  			opt_U = true;
> -			fwd_rule_parse(name, optarg, c->fwd[PIF_SPLICE]);
> +			fwd_rule_parse(name, false, optarg, c->fwd[PIF_SPLICE]);
>  		}
>  	} while (name != -1);
>  
> @@ -1910,13 +1910,13 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  	if (c->mode == MODE_PASTA) {
>  		if (!opt_t)
> -			fwd_rule_parse('t', "auto", c->fwd[PIF_HOST]);
> +			fwd_rule_parse('t', false, "auto", c->fwd[PIF_HOST]);
>  		if (!opt_T)
> -			fwd_rule_parse('T', "auto", c->fwd[PIF_SPLICE]);
> +			fwd_rule_parse('T', false, "auto", c->fwd[PIF_SPLICE]);
>  		if (!opt_u)
> -			fwd_rule_parse('u', "auto", c->fwd[PIF_HOST]);
> +			fwd_rule_parse('u', false, "auto", c->fwd[PIF_HOST]);
>  		if (!opt_U)
> -			fwd_rule_parse('U', "auto", c->fwd[PIF_SPLICE]);
> +			fwd_rule_parse('U', false, "auto", c->fwd[PIF_SPLICE]);
>  	}
>  
>  	conf_sock_listen(c);
> @@ -2135,14 +2135,8 @@ void conf_handler(struct ctx *c, uint32_t events)
>  		unsigned pif;
>  
>  		/* Clear pending tables */
> -		for (pif = 0; pif < PIF_NUM_TYPES; pif++) {
> -			struct fwd_table *fwd = c->fwd_pending[pif];
> -
> -			if (!fwd)
> -				continue;
> -			fwd->count = 0;
> -			fwd->sock_count = 0;
> -		}
> +		for (pif = 0; pif < PIF_NUM_TYPES; pif++)
> +			fwd_rule_clear(c->fwd_pending[pif]);
>  
>  		/* FIXME: this could block indefinitely if the client doesn't
>  		 * write as much as it should
> diff --git a/fwd_rule.c b/fwd_rule.c
> index 03e8e80..eb9a601 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -180,6 +180,66 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
>  	return true;
>  }
>  
> +/**
> + * 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;
> +

Not essential, but I wonder if it would be wise to verify that there
are no currently open sockets associated with any of the rules.

> +	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 match 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 valid sockets
> + * stored in fwd->rulesocks.

The exact meaning of this isn't very clear to me.  Does "valid" mean
"open" or something else?

I think what you're getting at is that every entry in fwd->socks[]
must be -1.  Or at least every entry with index in [0,sock_count)

> + */
> +static int fwd_rule_del(struct fwd_table *fwd, const struct fwd_rule *rule)
> +{
> +	unsigned num, i;
> +
> +	for (i = 0; i < fwd->count; i++) {
> +		if (fwd_rule_conflicts(rule, &fwd->rules[i]))
> +			break;
> +	}

So, this deletes any conflicting rule, not only exact matches.  That's
not very clear from the description of @rule.

> +
> +	if (i == fwd->count) {
> +		char newstr[FWD_RULE_STRLEN];
> +
> +		warn("Couldn't find forwarding rule to delete: %s",
> +		     fwd_rule_fmt(rule, newstr, sizeof(newstr)));
> +		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),

I don't think the explicit (void *) casts are necessary - they should
be implicit from memmove()s signature.

> +		(fwd->count - i) * sizeof(*fwd->rulesocks));

Is memmove() guaranteed to be safe if given a zero length?  That will
occur if deleting the last entry.

> +	/* TODO: move sockets stored starting from fwd->rulesocks[i + i], 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)

Again, is this safe if i == fwd->count?

> +
> +	return 0;
> +}
> +
>  /**
>   * fwd_rule_add() - Validate and add a rule to a forwarding table
>   * @fwd:	Table to add to
> @@ -368,6 +428,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

Clunky, but it gets the job done.

>   * @proto:	Protocol to forward
>   * @addr:	Listening address
>   * @ifname:	Listening interface
> @@ -377,8 +438,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,
> @@ -418,8 +479,13 @@ 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;
>  	}
> @@ -445,12 +511,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)
> @@ -507,7 +574,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;
> @@ -537,7 +604,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);
> @@ -551,10 +618,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;
> @@ -632,12 +701,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);
>  			}
> @@ -653,7 +722,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 1ea1d12..cd0f3dc 100644
> --- a/pesto.1
> +++ b/pesto.1
> @@ -31,6 +31,42 @@ Be verbose.
>  .BR \-h ", " \-\-help
>  Display a help message and exit.
>  
> +.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 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:
> @@ -162,6 +198,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:

I thought we wanted to default to add, rather than replace.  That
seems both a little simpler to implement and agruably more likely to
be what peopke want.

> +.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 <sbrivio@redhat.com>,
> diff --git a/pesto.c b/pesto.c
> index 73fdc39..143d4c6 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;

MODE_CLEAR doesn't make sense to me.  Unlike add or del, clear is a
once-off operation, it's not clear to me how it would affect the
interpretation of -[tTuU].


> +	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':
> @@ -439,13 +452,38 @@ int main(int argc, char **argv)
>  		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 (!strcmp(optarg, "HOST")) {
> +				fwd_rule_clear(&inbound->fwd);
> +				inbound_cleared = true;
> +			} else if (!strcmp(optarg, "SPLICE")) {
> +				fwd_rule_clear(&outbound->fwd);

outbound will be NULL if talking to passt, so this could SEGV.

> +				outbound_cleared = true;
> +			} else {
> +				die("Unsupported pif name %s", optarg);
> +			}

For the time being pesto is limited to a single "inbound" and single
"outbound" table, simply because we haven't devised syntax for
anything else.  However, we don't actually need that for --clear.
Since it already takes a pif name we can use pif_conf_by_name() to
clear an arbitrary named pif's rules.

> +
> +			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 +491,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,
> +				       &inbound->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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]