Re: [PATCH v3 02/14] conf, fwd: Keep a table of our port forwarding configuration

[Stefano Brivio <sbrivio@redhat.com>]

On Thu,  8 Jan 2026 13:29:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> At present, we set up forwarding as we parse the -t, and -u options, not
> keeping a persistent data structure with all the details.  We do have
> some information in struct fwd_ports, but it doesn't capture all the nuance
> that the options do.
> 
> As a first step to generalising our forwarding model, add a table of all
> the forwarding rules to struct fwd_ports.  For now it covers only explicit
> forwards, not automatic, and we don't do anything with it other than print
> some additional debug information.  We'll do more with it in future
> patches.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c | 80 ++++++++++++++++++++++++++++++-------------------
>  fwd.c  | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  fwd.h  | 35 +++++++++++++++++++++-
>  3 files changed, 179 insertions(+), 31 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index c936664b..127e69f5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -137,7 +137,7 @@ static int parse_port_range(const char *s, char **endptr,
>   * @last:	Last port to forward
>   * @exclude:	Bitmap of ports to exclude
>   * @to:		Port to translate @first to when forwarding
> - * @weak:	Ignore errors, as long as at least one port is mapped
> + * @flags:	Flags for forwarding entries
>   */
>  static void conf_ports_range_except(const struct ctx *c, char optname,
>  				    const char *optarg, struct fwd_ports *fwd,
> @@ -145,10 +145,11 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
>  				    const char *ifname,
>  				    uint16_t first, uint16_t last,
>  				    const uint8_t *exclude, uint16_t to,
> -				    bool weak)
> +				    uint8_t flags)
>  {
> +	unsigned delta = to - first;
>  	bool bound_one = false;
> -	unsigned i;
> +	unsigned base, i;
>  	int fd;
>  
>  	if (first == 0) {
> @@ -172,37 +173,45 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
>  		}
>  	}
>  
> -	for (i = first; i <= last; i++) {
> -		if (bitmap_isset(exclude, i))
> +	for (base = first; base <= last; base++) {
> +		if (bitmap_isset(exclude, base))
>  			continue;
>  
> -		if (bitmap_isset(fwd->map, i)) {
> -			warn(
> -"Altering mapping of already mapped port number: %s", optarg);
> -		}
> +		for (i = base; i <= last; i++) {

This nested loop is a bit convoluted, but I don't have better ideas and
I suppose it goes away later (I just reached 10/14 so far).

> +			if (bitmap_isset(exclude, i))
> +				break;
>  
> -		bitmap_set(fwd->map, i);
> -		fwd->delta[i] = to - first;
> +			if (bitmap_isset(fwd->map, i)) {
> +				warn(
> +"Altering mapping of already mapped port number: %s", optarg);
> +			}
>  
> -		if (optname == 't')
> -			fd = tcp_listen(c, PIF_HOST, addr, ifname, i);
> -		else if (optname == 'u')
> -			fd = udp_listen(c, PIF_HOST, addr, ifname, i);
> -		else
> -			/* No way to check in advance for -T and -U */
> -			fd = 0;
> +			if (optname == 't')
> +				fd = tcp_listen(c, PIF_HOST, addr, ifname, i);
> +			else if (optname == 'u')
> +				fd = udp_listen(c, PIF_HOST, addr, ifname, i);
> +			else
> +				/* No way to check in advance for -T and -U */
> +				fd = 0;
> +
> +			if (fd == -ENFILE || fd == -EMFILE) {
> +				die(
> +"Can't open enough sockets for port specifier: %s",
> +				    optarg);
> +			}
>  
> -		if (fd == -ENFILE || fd == -EMFILE) {
> -			die("Can't open enough sockets for port specifier: %s",
> -			    optarg);
> +			if (fd >= 0) {
> +				bound_one = true;
> +			} else if (!(flags & FWD_WEAK)) {
> +				die(
> +"Failed to bind port %u (%s) for option '-%c %s'",
> +				    i, strerror_(-fd), optname, optarg);
> +			}
>  		}
>  
> -		if (fd >= 0) {
> -			bound_one = true;
> -		} else if (!weak) {
> -			die("Failed to bind port %u (%s) for option '-%c %s'",
> -			    i, strerror_(-fd), optname, optarg);
> -		}
> +		fwd_rule_add(fwd, flags, addr, ifname, base, i - 1,
> +			     base + delta);
> +		base = i - 1;
>  	}
>  
>  	if (!bound_one)
> @@ -272,7 +281,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		conf_ports_range_except(c, optname, optarg, fwd,
>  					NULL, NULL,
>  					1, NUM_PORTS - 1, exclude,
> -					1, true);
> +					1, FWD_WEAK);
>  		return;
>  	}
>  
> @@ -357,7 +366,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		conf_ports_range_except(c, optname, optarg, fwd,
>  					addr, ifname,
>  					1, NUM_PORTS - 1, exclude,
> -					1, true);
> +					1, FWD_WEAK);
>  		return;
>  	}
>  
> @@ -390,7 +399,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  					addr, ifname,
>  					orig_range.first, orig_range.last,
>  					exclude,
> -					mapped_range.first, false);
> +					mapped_range.first, 0);
>  	} while ((p = next_chunk(p, ',')));
>  
>  	return;
> @@ -1225,6 +1234,17 @@ dns6:
>  			info("    %s", c->dns_search[i].n);
>  		}
>  	}
> +
> +	info("Inbound TCP forwarding:");
> +	fwd_rules_print(&c->tcp.fwd_in);
> +	info("Inbound UDP forwarding:");
> +	fwd_rules_print(&c->udp.fwd_in);
> +	if (c->mode == MODE_PASTA) {
> +		info("Outbound TCP forwarding:");
> +		fwd_rules_print(&c->tcp.fwd_out);
> +		info("Outbound UDP forwarding:");
> +		fwd_rules_print(&c->udp.fwd_out);
> +	}
>  }
>  
>  /**
> diff --git a/fwd.c b/fwd.c
> index 961c533f..ad398e54 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -13,6 +13,7 @@
>   * Author: David Gibson <david@gibson.dropbear.id.au>
>   */
>  
> +#include <assert.h>
>  #include <stdint.h>
>  #include <errno.h>
>  #include <fcntl.h>
> @@ -22,6 +23,8 @@
>  
>  #include "util.h"
>  #include "ip.h"
> +#include "siphash.h"
> +#include "inany.h"
>  #include "fwd.h"
>  #include "passt.h"
>  #include "lineread.h"
> @@ -300,6 +303,19 @@ parse_err:
>  	warn("Unable to parse %s", PORT_RANGE_SYSCTL);
>  }
>  
> +/**
> + * fwd_rule_addr() - Return match address for a rule
> + * @rule:	Forwarding rule
> + *
> + * Return: Rule's match address, or NULL, if it matches any address

What about:

  matching address for rule, NULL if it matches all addresses

> + */
> +static const union inany_addr *fwd_rule_addr(const struct fwd_rule *rule)
> +{
> +	if (rule->flags & FWD_DUAL_STACK_ANY)
> +		return NULL;

Nit: usual newline here.

> +	return &rule->addr;
> +}
> +
>  /**
>   * fwd_port_is_ephemeral() - Is port number ephemeral?
>   * @port:	Port number
> @@ -313,6 +329,85 @@ bool fwd_port_is_ephemeral(in_port_t port)
>  	return (port >= fwd_ephemeral_min) && (port <= fwd_ephemeral_max);
>  }
>  
> +/**
> + * fwd_rule_add() - Add a rule to a forwarding table
> + * @fwd:	Table to add to
> + * @flags:	Flags for this entry
> + * @addr:	Our address to forward (NULL for both 0.0.0.0 and ::)
> + * @ifname:	Only forward from this interface name, if non-empty
> + * @first:	First port number to forward
> + * @last:	Last port number to forward
> + * @to:		First port of target port range to map to
> + */
> +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> +		  const union inany_addr *addr, const char *ifname,
> +		  in_port_t first, in_port_t last, in_port_t to)
> +{
> +	/* Flags which can be set from the caller */
> +	const uint8_t allowed_flags = FWD_WEAK;
> +	struct fwd_rule *new;
> +	unsigned port;
> +
> +	ASSERT(!(flags & ~allowed_flags));
> +
> +	if (fwd->count >= ARRAY_SIZE(fwd->rules))
> +		die("Too many port forwarding ranges");
> +
> +	new = &fwd->rules[fwd->count++];
> +	new->flags = flags;
> +
> +	if (addr) {
> +		new->addr = *addr;
> +	} else {
> +		new->addr = inany_any6;
> +		new->flags |= FWD_DUAL_STACK_ANY;
> +	}
> +
> +	memset(new->ifname, 0, sizeof(new->ifname));
> +	if (ifname)
> +		strncpy(new->ifname, ifname, sizeof(new->ifname));

Coverity Scan complains because we copy exactly up to
sizeof(new->ifname) bytes, and the terminator might be missing:

/home/sbrivio/passt/fwd.c:368:3:
  Type: Buffer not null terminated (BUFFER_SIZE)

/home/sbrivio/passt/fwd.c:351:2:
  1. path: Condition "!(flags & -3 /* ~allowed_flags */)", taking true branch.
/home/sbrivio/passt/fwd.c:353:2:
  2. path: Condition "fwd->count >= 128U /* (int)(sizeof (fwd->rules) / sizeof (fwd->rules[0])) */", taking false branch.
/home/sbrivio/passt/fwd.c:359:2:
  3. path: Condition "addr", taking true branch.
/home/sbrivio/passt/fwd.c:361:2:
  4. path: Falling through to end of if statement.
/home/sbrivio/passt/fwd.c:367:2:
  5. path: Condition "ifname", taking true branch.
/home/sbrivio/passt/fwd.c:368:3:
  6. buffer_size_warning: Calling "strncpy" with a maximum size argument of 16 bytes on destination array "new->ifname" of size 16 bytes might leave the destination string unterminated.
/home/sbrivio/passt/fwd.c:370:2:


> +
> +	ASSERT(first <= last);
> +	new->first = first;
> +	new->last = last;
> +
> +	new->to = to;
> +
> +	for (port = new->first; port <= new->last; port++) {
> +		/* Fill in the legacy data structures to match the table */
> +		bitmap_set(fwd->map, port);
> +		fwd->delta[port] = new->to - new->first;
> +	}
> +}
> +
> +/**
> + * fwd_rules_print() - Print forwarding rules for debugging
> + * @fwd:	Table to print
> + */
> +void fwd_rules_print(const struct fwd_ports *fwd)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < fwd->count; i++) {
> +		const struct fwd_rule *rule = &fwd->rules[i];
> +		const char *weak = rule->flags & FWD_WEAK ? " WEAK" : "";

Should we print " might fail" or " can fail" instead of " WEAK"? This
is for users.

> +		const char *percent = *rule->ifname ? "%" : "";
> +		char addr[INANY_ADDRSTRLEN];
> +
> +		inany_ntop(fwd_rule_addr(rule), addr, sizeof(addr));
> +
> +		if (rule->first == rule->last) {
> +			info("    [%s]%s%s:%hu  =>  %hu %s",
> +			     addr, percent, rule->ifname,
> +			     rule->first, rule->to, weak);
> +		} else {
> +			info("    [%s]%s%s:%hu-%hu  =>  %hu-%hu %s",
> +			     addr, percent, rule->ifname, rule->first, rule->last,
> +			     rule->to, rule->last - rule->first + rule->to, weak);
> +		}
> +	}
> +}
> +
>  /* See enum in kernel's include/net/tcp_states.h */
>  #define UDP_LISTEN	0x07
>  #define TCP_LISTEN	0x0a
> diff --git a/fwd.h b/fwd.h
> index 934bab33..3dfc7959 100644
> --- a/fwd.h
> +++ b/fwd.h
> @@ -16,6 +16,30 @@ struct flowside;
>  void fwd_probe_ephemeral(void);
>  bool fwd_port_is_ephemeral(in_port_t port);
>  
> +/**
> + * struct fwd_rule - Forwarding rule governing a range of ports
> + * @addr:	Address to forward from
> + * @ifname:	Interface to forward from
> + * @first:	First port number to forward
> + * @last:	Last port number to forward
> + * @to:		Port number to forward port @first to.
> + * @flags:	Flag mask
> + * 	FWD_DUAL_STACK_ANY - match any IPv4 or IPv6 address (@addr should be ::)
> + *	FWD_WEAK - Don't give an error if binds fail for some forwards

Usually we document bit flags inline as we define them (see struct
tcp_tap_conn), I guess it's a bit easier to follow, but this has the
advantage of looking less cramped (unless we add comment lines before
the #defines, which would also be reasonable I think).

Not a strong preference either way.

> + *
> + * FIXME: @addr and @ifname currently ignored for outbound tables
> + */
> +struct fwd_rule {
> +	union inany_addr addr;
> +	char ifname[IFNAMSIZ];
> +	in_port_t first, last, to;

Having one line for each helps (me at least) visualising the size of the
struct. Not a strong preference.

> +#define FWD_DUAL_STACK_ANY	BIT(0)
> +#define FWD_WEAK		BIT(1)
> +	uint8_t flags;
> +};
> +
> +#define MAX_FWD_RULES	128
> +
>  /**
>   * union fwd_listen_ref - information about a single listening socket
>   * @port:	Bound port number of the socket
> @@ -44,6 +68,8 @@ enum fwd_ports_mode {
>   * @mode:	Overall forwarding mode (all, none, auto, specific ports)
>   * @scan4:	/proc/net fd to scan for IPv4 ports when in AUTO mode
>   * @scan6:	/proc/net fd to scan for IPv6 ports when in AUTO mode
> + * @count:	Number of forwarding rules
> + * @rules:	Array of forwarding rules
>   * @map:	Bitmap describing which ports are forwarded
>   * @delta:	Offset between the original destination and mapped port number
>   */
> @@ -51,14 +77,21 @@ struct fwd_ports {
>  	enum fwd_ports_mode mode;
>  	int scan4;
>  	int scan6;
> +	unsigned count;
> +	struct fwd_rule rules[MAX_FWD_RULES];
>  	uint8_t map[PORT_BITMAP_SIZE];
>  	in_port_t delta[NUM_PORTS];
>  };
>  
>  #define FWD_PORT_SCAN_INTERVAL		1000	/* ms */
>  
> +void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> +		  const union inany_addr *addr, const char *ifname,
> +		  in_port_t first, in_port_t last, in_port_t to);
> +void fwd_rules_print(const struct fwd_ports *fwd);
> +
>  void fwd_scan_ports_init(struct ctx *c);
> -void fwd_scan_ports_timer(struct ctx *c, const struct timespec *now);
> +void fwd_scan_ports_timer(struct ctx * c, const struct timespec *now);
>  
>  bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
>  		 union inany_addr *translated);

-- 
Stefano