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

[Laurent Vivier <lvivier@redhat.com>]

On 5/6/26 15:23, 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 <sbrivio@redhat.com>

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

> ---
>   conf.c     |  26 +++++-------
>   fwd_rule.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++------
>   fwd_rule.h |   4 +-
>   pesto.1    |  85 +++++++++++++++++++++++++++++++++++++++
>   pesto.c    |  55 +++++++++++++++++++++++--
>   5 files changed, 253 insertions(+), 33 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index f4ab72a..b5576f5 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 200f4b5..5fc04d7 100644
> --- a/fwd_rule.c
> +++ b/fwd_rule.c
> @@ -180,6 +180,89 @@ static bool fwd_rule_conflicts(const struct fwd_rule *a, const struct fwd_rule *
>   	return true;
>   }
>   
> +/**
> + * 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)
> +{
> +	return !memcmp(a, b, sizeof(*a));
> +}
> +
> +/**
> + * 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;
> +		}
> +	}
> +
> +	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 <sbrivio@redhat.com>,
> 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;