Re: [PATCH v3 12/14] fwd: Remap ports based directly on forwarding rule

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

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

On Tue, Jan 13, 2026 at 11:12:26PM +0100, Stefano Brivio wrote:
> On Thu,  8 Jan 2026 13:29:46 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Currently we remap port numbers based on the legacy delta[] array, which
> > is indexed only by original port number, not the listening address.  Now
> > that we look up a forwarding rule entry in flow_target(), we can use this
> > entry to directly determine the correct remapped port.  Implement this,
> > and remove the old delta[] array.
> > 
> > Link: https://bugs.passt.top/show_bug.cgi?id=187
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c |  7 ++++---
> >  fwd.c  | 21 +++++++--------------
> >  fwd.h  |  7 +++----
> >  3 files changed, 14 insertions(+), 21 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index 045e9712..38f88732 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -493,6 +493,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  	struct flow_common *f = &flow->f;
> >  	const struct flowside *ini = &f->side[INISIDE];
> >  	struct flowside *tgt = &f->side[TGTSIDE];
> > +	const struct fwd_rule *rule = NULL;
> 
> Coverity Scan complains about two cases where this NULL rule would be
> passed to NAT functions dereferencing it. They should be both false
> positives because the rule is always set on pif_is_socket(...), but I
> wonder how fragile it is.

Ah, yeah.  I'm not terribly surprised.  I'll see what I can do.

> Regardless, it would be nice to avoid adding further warnings, if it's
> cheap (ASSERT() or check on rule in fwd_nat_from*() functions?). Anyway,
> here you go:
> 
> /home/sbrivio/passt/flow.c:535:3:
>   Type: Explicit null dereferenced (FORWARD_NULL)
> 
> /home/sbrivio/passt/flow.c:496:2:
>   1. assign_zero: Assigning: "rule" = "NULL".
> /home/sbrivio/passt/flow.c:499:2:
>   2. path: Condition "flow_new_entry == flow", taking true branch.
> /home/sbrivio/passt/flow.c:499:2:
>   3. path: Condition "f->state == FLOW_STATE_INI", taking true branch.
> /home/sbrivio/passt/flow.c:500:2:
>   4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:501:2:
>   5. path: Condition "f->pif[0] != PIF_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:501:2:
>   6. path: Condition "f->pif[1] == PIF_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:502:2:
>   7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch.
> /home/sbrivio/passt/flow.c:504:2:
>   8. path: Condition "pif_is_socket(f->pif[0])", taking false branch.
> /home/sbrivio/passt/flow.c:528:2:
>   9. path: Switch case value "PIF_SPLICE".
> /home/sbrivio/passt/flow.c:535:3:
>   10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_splice", which dereferences it.
> /home/sbrivio/passt/fwd.c:991:2:
>   10.1. path: Condition "!inany_is_loopback(&ini->eaddr)", taking false branch.
> /home/sbrivio/passt/fwd.c:991:2:
>   10.2. path: Condition "!inany_is_loopback(&ini->oaddr)", taking false branch.
> /home/sbrivio/passt/fwd.c:1008:2:
>   10.3. path: Condition "proto == IPPROTO_UDP", taking true branch.
> /home/sbrivio/passt/fwd.c:1012:2:
>   10.4. dereference: Dereferencing pointer "rule".
> 
> /home/sbrivio/passt/flow.c:539:3:
>   Type: Explicit null dereferenced (FORWARD_NULL)
> 
> /home/sbrivio/passt/flow.c:496:2:
>   1. assign_zero: Assigning: "rule" = "NULL".
> /home/sbrivio/passt/flow.c:499:2:
>   2. path: Condition "flow_new_entry == flow", taking true branch.
> /home/sbrivio/passt/flow.c:499:2:
>   3. path: Condition "f->state == FLOW_STATE_INI", taking true branch.
> /home/sbrivio/passt/flow.c:500:2:
>   4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:501:2:
>   5. path: Condition "f->pif[0] != PIF_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:501:2:
>   6. path: Condition "f->pif[1] == PIF_NONE", taking true branch.
> /home/sbrivio/passt/flow.c:502:2:
>   7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch.
> /home/sbrivio/passt/flow.c:504:2:
>   8. path: Condition "pif_is_socket(f->pif[0])", taking false branch.
> /home/sbrivio/passt/flow.c:528:2:
>   9. path: Switch case value "PIF_HOST".
> /home/sbrivio/passt/flow.c:539:3:
>   10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_host", which dereferences it.
> /home/sbrivio/passt/fwd.c:1069:2:
>   10.1. dereference: Dereferencing pointer "rule".
> 
> >  	uint8_t tgtpif = PIF_NONE;
> >  
> >  	ASSERT(flow_new_entry == flow && f->state == FLOW_STATE_INI);
> > @@ -514,7 +515,7 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  		else
> >  			goto nofwd;
> >  
> > -		if (!fwd_rule_search(fwd, ini)) {
> > +		if (!(rule = fwd_rule_search(fwd, ini))) {
> >  			/* This shouldn't happen, because if there's no rule for
> >  			 * it we should have no listening socket that would let
> >  			 * us get here
> > @@ -531,11 +532,11 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  		break;
> >  
> >  	case PIF_SPLICE:
> > -		tgtpif = fwd_nat_from_splice(c, proto, ini, tgt);
> > +		tgtpif = fwd_nat_from_splice(rule, proto, ini, tgt);
> >  		break;
> >  
> >  	case PIF_HOST:
> > -		tgtpif = fwd_nat_from_host(c, proto, ini, tgt);
> > +		tgtpif = fwd_nat_from_host(c, rule, proto, ini, tgt);
> >  		fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac);
> >  		break;
> >  	default:
> > diff --git a/fwd.c b/fwd.c
> > index 633ba5db..7c4575ff 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -405,7 +405,6 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> >  		/* Fill in the legacy forwarding data structures to match the table */
> >  		if (!(new->flags & FWD_SCAN))
> >  			bitmap_set(fwd->map, port);
> > -		fwd->delta[port] = new->to - new->first;
> >  	}
> >  }
> >  
> > @@ -978,7 +977,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >  
> >  /**
> >   * fwd_nat_from_splice() - Determine to forward a flow from the splice interface
> > - * @c:		Execution context
> > + * @rule:	Forwarding rule to apply
> >   * @proto:	Protocol (IP L4 protocol number)
> >   * @ini:	Flow address information of the initiating side
> >   * @tgt:	Flow address information on the target side (updated)
> > @@ -986,7 +985,7 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >   * Return: pif of the target interface to forward the flow to, PIF_NONE if the
> >   *         flow cannot or should not be forwarded at all.
> >   */
> > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto,
> >  			    const struct flowside *ini, struct flowside *tgt)
> >  {
> >  	if (!inany_is_loopback(&ini->eaddr) ||
> > @@ -1010,11 +1009,7 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> >  		/* But for UDP preserve the source port */
> >  		tgt->oport = ini->eport;
> >  
> > -	tgt->eport = ini->oport;
> > -	if (proto == IPPROTO_TCP)
> > -		tgt->eport += c->tcp.fwd_out.delta[tgt->eport];
> > -	else if (proto == IPPROTO_UDP)
> > -		tgt->eport += c->udp.fwd_out.delta[tgt->eport];
> > +	tgt->eport = ini->oport - rule->first + rule->to;
> 
> This made me notice that the current documentation to struct fwd_rule
> doesn't really make it clear that n : 1 port mapping rules are not a
> thing,

Not yet, but that's a pretty obvious extension for later.

> so, maybe, back to 2/14, instead of:
> 
>  * @to:		Port number to forward port @first to.
> 
> we could have perhaps:
> 
>  * @to:		Target port for @first, port n maps to @to + (n - @first)
> 
> ?

Seems reasonable, done.

> By the way, I find this notation a bit complicated to figure out, I
> think that:
> 
> 	rule->to + (ini->oport - rule->first)

Good idea, done.

> (redundant parentheses included) is actually clearer. Or maybe even
> with a temporary 'delta' variable.
> 
> In both cases: the subtraction now happens in in_port_t, so I guess we
> should explicitly cast rule->first to int to be strictly correct.

No, it should be correct to do it within a u16.  At this point we know
that @first <= port <= @last, so the subtraction can't over or
underflow.

I guess the addition could, strictly speaking.  But 16-bit unsigned
overflow has the correct semantics for this.  I guess that does allow
remapping to port 0, which is problematic so maybe we should enforce
that (@to + @last - @first) < NUM_PORTS when we add rules.

> >  	return PIF_HOST;
> >  }
> > @@ -1058,6 +1053,7 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> >  /**
> >   * fwd_nat_from_host() - Determine to forward a flow from the host interface
> >   * @c:		Execution context
> > + * @rule:	Forwarding rule to apply
> >   * @proto:	Protocol (IP L4 protocol number)
> >   * @ini:	Flow address information of the initiating side
> >   * @tgt:	Flow address information on the target side (updated)
> > @@ -1065,15 +1061,12 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> >   * Return: pif of the target interface to forward the flow to, PIF_NONE if the
> >   *         flow cannot or should not be forwarded at all.
> >   */
> > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
> > +uint8_t fwd_nat_from_host(const struct ctx *c,
> > +			  const struct fwd_rule *rule, uint8_t proto,
> >  			  const struct flowside *ini, struct flowside *tgt)
> >  {
> >  	/* Common for spliced and non-spliced cases */
> > -	tgt->eport = ini->oport;
> > -	if (proto == IPPROTO_TCP)
> > -		tgt->eport += c->tcp.fwd_in.delta[tgt->eport];
> > -	else if (proto == IPPROTO_UDP)
> > -		tgt->eport += c->udp.fwd_in.delta[tgt->eport];
> > +	tgt->eport = ini->oport - rule->first + rule->to;
> 
> Same as above.

Done.

> >  	if (!c->no_splice && inany_is_loopback(&ini->eaddr) &&
> >  	    (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) {
> > diff --git a/fwd.h b/fwd.h
> > index a10bdbb4..cfe9ed46 100644
> > --- a/fwd.h
> > +++ b/fwd.h
> > @@ -82,7 +82,6 @@ enum fwd_ports_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
> >   * @listen_sock_count: Number of entries used in @listen_socks
> >   * @listen_socks: Listening sockets for forwarding
> >   */
> > @@ -93,7 +92,6 @@ struct fwd_ports {
> >  	unsigned count;
> >  	struct fwd_rule rules[MAX_FWD_RULES];
> >  	uint8_t map[PORT_BITMAP_SIZE];
> > -	in_port_t delta[NUM_PORTS];
> >  	unsigned listen_sock_count;
> >  	int listen_socks[MAX_LISTEN_SOCKS];
> >  };
> > @@ -117,9 +115,10 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> >  		 union inany_addr *translated);
> >  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >  			 const struct flowside *ini, struct flowside *tgt);
> > -uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
> > +uint8_t fwd_nat_from_splice(const struct fwd_rule *rule, uint8_t proto,
> >  			    const struct flowside *ini, struct flowside *tgt);
> > -uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
> > +uint8_t fwd_nat_from_host(const struct ctx *c,
> > +			  const struct fwd_rule *rule, uint8_t proto,
> >  			  const struct flowside *ini, struct flowside *tgt);
> >  void fwd_neigh_table_update(const struct ctx *c, const union inany_addr *addr,
> >  			    const uint8_t *mac, bool permanent);
> 
> -- 
> 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 --]