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

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

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

On Tue, Jan 13, 2026 at 12:26:10AM +0100, Stefano Brivio wrote:
> 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).

The nested loop remains, so far.  A lot of the rest of the loop body
does go away, so the overall complexity does reduce.

> > +			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.

Fixed.

> > +	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:

Ah, good point.  I added a check.

> > +
> > +	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.

Good point.  THough I'm not sure "might fail" or "can fail" is
terribly clear in context either.  I've gone with " (best effort)" for
now.

> 
> > +		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.

Keeping it as-is, for now, since some of the descriptions are quite
long so they'd wrap if put on the same line as the #define.

> 
> > + *
> > + * 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.

Fair point, altered.

> > +#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
> 

-- 
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 --]