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