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

On Tue, Jan 13, 2026 at 11:12:26PM +0100, Stefano Brivio wrote:

On Thu, 8 Jan 2026 13:29:46 +1100 David Gibson 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 --- 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