Re: [PATCH v4 12/13] fwd_rule: Allow "all" port specs to be combined with other options

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

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

On Thu, Jul 02, 2026 at 09:14:17AM +0200, Stefano Brivio wrote:
> On Thu,  2 Jul 2026 16:31:42 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we handle -t all and the like as a special case, it can't be
> > combined with other port specifier options.  Remove that restriction,
> > allowing combined options like:
> >      -t all,~9999          # Forward everything non-ephemeral except 9999
> >      -t all,auto           # Equivalent to -t auto
> >      -t all,33000          # Forward non-ephemeral plus port 33,000
> > 
> > This isn't particularly useful immediately, but will become important for
> > destination address specification - it provides a place to attach the
> > target address for "all" or exclude only mappings.  It will also work
> > better with some parsing reworks we want to make.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c     | 11 +++++------
> >  fwd_rule.c | 39 ++++++++++++++++++++-------------------
> >  passt.1    | 33 ++++++++++++++++-----------------
> >  3 files changed, 41 insertions(+), 42 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index c4a36dee..a610c0c6 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -660,11 +660,9 @@ static void usage(const char *name, FILE *f, int status)
> >  		"    SPEC can be:\n"
> >  		"      'none': don't forward any ports\n"
> >  		"      [ADDR[%%IFACE]/]PORTS: forward specific ports\n"
> > -		"        PORTS is either 'all' (forward all unbound, non-ephemeral\n"
> > -		"        ports), or a comma-separated list of ports, optionally\n"
> > -		"        ranged with '-' and optional target ports after ':'.\n"
> > -		"        Ranges can be reduced by excluding ports or ranges\n"
> > -		"        prefixed by '~'.\n"
> > +		"        PORTS is comma-separated list of ports, either\n"
> 
> I didn't really consider this change as worth updating usage and man
> page (the previous version wouldn't be entirely accurate anymore but
> practically speaking rather clear, I thought).

I think we should update the manpage, since it is pretty detailed (and
now wrong).  usage() I'll grant is borderline at best.

> If it is:
> 
> - PORTS is _a_ comma-separated ...

Fixed.

> - I think we should maintain the description for 'all' (forward all
>   unbound, non-ephemeral ports), because otherwise just "Forward all
>   ports" below becomes particularly misleading

Good point.  I've reworded again, I think it's better.

> 
> > +		"        'all', a port number or range. Ranges can be reduced\n"
> > +		"        by excluding ports or ranges prefixed by '~'.\n"
> >  		"%s"
> >  		"        Examples:\n"
> >  		"        -t all		Forward all ports\n"
> > @@ -677,7 +675,8 @@ static void usage(const char *name, FILE *f, int status)
> >  		"			corresponding port numbers plus 10\n"
> >  		"        -t 192.0.2.1/5	Bind port 5 of 192.0.2.1 to %s\n"
> >  		"        -t 5-25,~10-20	Forward ports 5 to 9, and 21 to 25\n"
> > -		"        -t ~25		Forward all ports except for 25\n"
> > +		"        -t ~25,all\n"
> > +		"        -t 25		Forward all ports except for 25\n"
> 
> I think the previous version makes more sense. This isn't an exhaustive
> description, it just shows how to quickly do things. This is missing a
> ~ by the way.

Oops, yes.  I dropped these lines.

> 
> >  		"%s"
> >  		"    default: %s\n"
> >  		"  -u, --udp-ports SPEC	UDP port forwarding to %s\n"
> > diff --git a/fwd_rule.c b/fwd_rule.c
> > index 6d7ec2c5..b14df340 100644
> > --- a/fwd_rule.c
> > +++ b/fwd_rule.c
> > @@ -471,20 +471,13 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  	uint8_t flags = 0;
> >  	unsigned i;
> >  
> > -	if (!strcmp(spec, "all")) {
> > -		/* Treat "all" as equivalent to "": all non-ephemeral ports */
> > -		spec = "";
> > -	}
> > -
> >  	/* Parse excluded ranges and "auto" in the first pass */
> >  	for_each_chunk(p, ep, spec, ",") {
> >  		struct port_range xrange;
> >  
> > -		if (isdigit(*p)) {
> > -			/* Include range, parse later */
> > -			exclude_only = false;
> > +		/* Include range, parse later */
> > +		if (parse_literal(&p, "all") || isdigit(*p))
> >  			continue;
> > -		}
> >  
> >  		if (parse_literal(&p, "auto")) {
> >  			if (p != ep) /* Garbage after the keyword */
> > @@ -512,20 +505,18 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  			bitmap_set(exclude, i);
> >  	}
> >  
> > -	if (exclude_only) {
> > -		/* Exclude ephemeral ports */
> > -		fwd_port_map_ephemeral(exclude);
> > -
> > -		fwd_rule_range_except(fwd, del, proto, addr, ifname,
> > -				      1, NUM_PORTS - 1, exclude,
> > -				      1, flags | FWD_WEAK);
> > -		return;
> > -	}
> > -
> >  	/* Now process base ranges, skipping exclusions */
> >  	for_each_chunk(p, ep, spec, ",") {
> >  		struct port_range orig_range, mapped_range;
> >  
> > +		/* Handle "all" like exclude only */
> > +		if (parse_literal(&p, "all")) {
> > +			if (p != ep) /* Garbage after the keyword */
> > +				goto bad;
> > +
> > +			continue;
> > +		}
> > +
> >  		if (!isdigit(*p))
> >  			/* Already parsed */
> >  			continue;
> > @@ -533,6 +524,8 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  		if (!parse_port_range(&p, &orig_range))
> >  			goto bad;
> >  
> > +		exclude_only = false;
> > +
> >  		if (parse_literal(&p, ":")) {
> >  			/* There's a range to map to as well */
> >  			if (!parse_port_range(&p, &mapped_range))
> > @@ -553,6 +546,14 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
> >  				      mapped_range.first, flags);
> >  	}
> >  
> > +	/* Finally handle "all" and exclude only specs */
> > +	if (exclude_only) {
> > +		fwd_port_map_ephemeral(exclude);
> > +
> > +		fwd_rule_range_except(fwd, del, proto, addr, ifname,
> > +				      1, NUM_PORTS - 1, exclude,
> > +				      1, flags | FWD_WEAK);
> > +	}
> >  	return;
> >  bad:
> >  	die("Invalid port specifier '%s'", spec);
> > diff --git a/passt.1 b/passt.1
> > index 908fd4a4..c3722ef9 100644
> > --- a/passt.1
> > +++ b/passt.1
> > @@ -432,29 +432,22 @@ Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39.
> >  
> >  .TP
> >  .BR \-t ", " \-\-tcp-ports " " \fIspec
> > -Configure TCP port forwarding to guest or namespace. \fIspec\fR can be one of:
> > +Configure TCP port forwarding to guest or namespace. \fIspec\fR can be either:
> >  .RS
> >  
> >  .TP
> >  .BR none
> >  Don't forward any ports
> >  
> > +or
> >  .TP
> >  [\fIaddress\fR[\fB%\fR\fIinterface\fR]\fB/\fR]\fIports\fR ...
> > -Specific ports to forward.  Optionally, a specific listening address
> > -and interface name (since Linux 5.7) can be specified.  \fIports\fR
> > -may be either:
> > -.RS
> > -.TP
> > -\fBall\fR
> > -Forward all unbound, non-ephemeral ports, as permitted by current
> > -capabilities.  For low (< 1024) ports, see \fBNOTES\fR. No failures
> > -are reported for unavailable ports, unless no ports could be forwarded
> > -at all.
> > +
> > +Ports to forward.  Optionally, a specific listening address and
> > +interface name (since Linux 5.7) can be specified.
> >  .RE
> >  
> > -.RS
> > -or a comma-separated list of entries which may be any of:
> > +\fIports\fR is a comma-separated list of entries which may be any of:
> >  .TP
> >  \fIfirst\fR[\fB-\fR\fIlast\fR][\fB:\fR\fItofirst\fR[\fB-\fR\fItolast\fR]]
> >  Include range. Forward port numbers between \fIfirst\fR and \fIlast\fR
> > @@ -468,6 +461,13 @@ as \fIfirst\fR.
> >  Exclude range.  Don't forward port numbers between \fIfirst\fR and
> >  \fIlast\fR.  This takes precedences over include ranges.
> >  
> > +.TP
> > +.BR all
> > +Forward all unbound, non-ephemeral ports, not covered by exclude
> > +ranges above, as permitted by current capabilities.  For low (< 1024)
> > +ports, see \fBNOTES\fR. No failures are reported for unavailable
> > +ports, unless no ports could be forwarded at all.
> > +
> >  .TP
> >  .BR auto
> >  \fBpasta\fR only.  Only forward ports in the specified set if the
> > @@ -477,10 +477,9 @@ periodically derived (every second) from listening sockets reported by
> >  .RE
> >  
> >  Specifying excluded ranges only implies that all other non-ephemeral
> > -ports are forwarded. Specifying no ranges at all implies forwarding
> > -all non-ephemeral ports permitted by current capabilities.  In this
> > -case, no failures are reported for unavailable ports, unless no ports
> > -could be forwarded at all.
> > +ports are forwarded. Specifying no ranges is equivalent
> > +to '\fBall\fR'.  In this case, no failures are reported for
> > +unavailable ports, unless no ports could be forwarded at all.
> 
> Nit: this could use a few more columns (I think it's slightly more
> readable as source), say:

Huh, odd.  For some reason emacs M-q was making it narrower.  Fixed
manually.

> ports are forwarded.  Specifying no ranges is equivalent to '\fBall\fR'.  In
> this case, no failures are reported for unavailable ports, unless no ports could
> be forwarded at all.
> 
> -- 
> 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 --]