Re: [PATCH v3 07/25] fwd: Store forwarding tables indexed by (origin) pif

On Mon, 23 Mar 2026 18:37:14 +1100 David Gibson wrote:

Currently we store the inbound (PIF_HOST) and outbound (PIF_SPLICE) forwarding tables in separate fields of struct ctx. In a number of places this requires somewhat awkward if or switch constructs to select the right table for updates. Conceptually simplify that by using an index of forwarding tables by pif, which as a bonus keeps track generically which pifs have implemented forwarding tables so far.

For now this doesn't simplify a lot textually, because many places that need this also have other special cases to apply by pif. It does simplify a few crucial places though, and we expect it will become more useful as the flexibility of the forwarding table is improved.

Signed-off-by: David Gibson --- conf.c | 58 +++++++++++++++++++++++++++++++------------------- flow.c | 22 +++++++------------ fwd.c | 65 ++++++++++++++++++++++++++++++--------------------------- fwd.h | 4 ++-- passt.h | 6 ++---- 5 files changed, 83 insertions(+), 72 deletions(-)

diff --git a/conf.c b/conf.c index b1ebb4a4..6ca61b74 100644 --- a/conf.c +++ b/conf.c @@ -1252,11 +1252,17 @@ dns6: } }

- info("Inbound forwarding:"); - fwd_rules_print(&c->fwd_in); - if (c->mode == MODE_PASTA) { - info("Outbound forwarding:"); - fwd_rules_print(&c->fwd_out); + for (i = 0; i < PIF_NUM_TYPES; i++) { + const char *dir = "Outbound"; + + if (!c->fwd[i]) + continue; + + if (i == PIF_HOST) + dir = "Inbound"; + + info("%s forwarding rules (%s):", dir, pif_name(i)); + fwd_rules_print(c->fwd[i]); } }

@@ -2154,18 +2160,24 @@ void conf(struct ctx *c, int argc, char **argv)

/* Forwarding options can be parsed now, after IPv4/IPv6 settings */ fwd_probe_ephemeral(); + fwd_rule_init(c); optind = 0; do { name = getopt_long(argc, argv, optstring, options, NULL);

- if (name == 't') - conf_ports(c, name, optarg, &c->fwd_in, &tcp_in_mode); - else if (name == 'u') - conf_ports(c, name, optarg, &c->fwd_in, &udp_in_mode); - else if (name == 'T') - conf_ports(c, name, optarg, &c->fwd_out, &tcp_out_mode); - else if (name == 'U') - conf_ports(c, name, optarg, &c->fwd_out, &udp_out_mode); + if (name == 't') { + conf_ports(c, name, optarg, c->fwd[PIF_HOST], + &tcp_in_mode); + } else if (name == 'u') { + conf_ports(c, name, optarg, c->fwd[PIF_HOST], + &udp_in_mode); + } else if (name == 'T') { + conf_ports(c, name, optarg, c->fwd[PIF_SPLICE], + &tcp_out_mode); + } else if (name == 'U') { + conf_ports(c, name, optarg, c->fwd[PIF_SPLICE], + &udp_out_mode); + } } while (name != -1);

if (c->mode == MODE_PASTA) @@ -2224,20 +2236,24 @@ void conf(struct ctx *c, int argc, char **argv) udp_out_mode = fwd_default;

if (tcp_in_mode == FWD_MODE_AUTO) { - conf_ports_range_except(c, 't', "auto", &c->fwd_in, NULL, NULL, - 1, NUM_PORTS - 1, NULL, 1, FWD_SCAN); + conf_ports_range_except(c, 't', "auto", c->fwd[PIF_HOST], + NULL, NULL, 1, NUM_PORTS - 1, NULL, 1, + FWD_SCAN); } if (tcp_out_mode == FWD_MODE_AUTO) { - conf_ports_range_except(c, 'T', "auto", &c->fwd_out, NULL, "lo", - 1, NUM_PORTS - 1, NULL, 1, FWD_SCAN); + conf_ports_range_except(c, 'T', "auto", c->fwd[PIF_SPLICE], + NULL, "lo", 1, NUM_PORTS - 1, NULL, 1, + FWD_SCAN); } if (udp_in_mode == FWD_MODE_AUTO) { - conf_ports_range_except(c, 'u', "auto", &c->fwd_in, NULL, NULL, - 1, NUM_PORTS - 1, NULL, 1, FWD_SCAN); + conf_ports_range_except(c, 'u', "auto", c->fwd[PIF_HOST], + NULL, NULL, 1, NUM_PORTS - 1, NULL, 1, + FWD_SCAN); } if (udp_out_mode == FWD_MODE_AUTO) { - conf_ports_range_except(c, 'U', "auto", &c->fwd_out, NULL, "lo", - 1, NUM_PORTS - 1, NULL, 1, FWD_SCAN); + conf_ports_range_except(c, 'U', "auto", c->fwd[PIF_SPLICE], + NULL, "lo", 1, NUM_PORTS - 1, NULL, 1, + FWD_SCAN); }

if (!c->quiet) diff --git a/flow.c b/flow.c index c84857b2..2972ab87 100644 --- a/flow.c +++ b/flow.c @@ -503,10 +503,10 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, { char estr[INANY_ADDRSTRLEN], ostr[INANY_ADDRSTRLEN]; struct flow_common *f = &flow->f; + const struct fwd_table *fwd = c->fwd[f->pif[INISIDE]]; const struct flowside *ini = &f->side[INISIDE]; struct flowside *tgt = &f->side[TGTSIDE]; const struct fwd_rule *rule = NULL; - const struct fwd_table *fwd; uint8_t tgtpif = PIF_NONE;

assert(flow_new_entry == flow && f->state == FLOW_STATE_INI); @@ -514,6 +514,11 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, assert(f->pif[INISIDE] != PIF_NONE && f->pif[TGTSIDE] == PIF_NONE); assert(flow->f.state == FLOW_STATE_INI);

+ if (fwd) { + if (!(rule = fwd_rule_search(fwd, ini, proto, rule_hint))) + goto norule; + } + switch (f->pif[INISIDE]) { case PIF_TAP: memcpy(f->tap_omac, MAC_UNDEF, ETH_ALEN); @@ -521,20 +526,10 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow, break;

case PIF_SPLICE: - fwd = &c->fwd_out; - - if (!(rule = fwd_rule_search(fwd, ini, proto, rule_hint))) - goto norule; -

Coverity Scan doesn't like this: /home/sbrivio/passt/flow.c:528:3: Type: Explicit null dereferenced (FORWARD_NULL) /home/sbrivio/passt/flow.c:508:2: 1. assign_zero: Assigning: "rule" = "NULL". /home/sbrivio/passt/flow.c:511:2: 2. path: Condition "flow_new_entry == flow", taking true branch. /home/sbrivio/passt/flow.c:511:2: 3. path: Condition "f->state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:512:2: 4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch. /home/sbrivio/passt/flow.c:513:2: 5. path: Condition "f->pif[0] != PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:513:2: 6. path: Condition "f->pif[1] == PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:514:2: 7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:516:2: 8. path: Condition "fwd", taking false branch. /home/sbrivio/passt/flow.c:521:2: 9. path: Switch case value "PIF_SPLICE". /home/sbrivio/passt/flow.c:528:3: 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_splice", which dereferences it. /home/sbrivio/passt/fwd.c:1095:2: 10.1. path: Condition "!inany_is_loopback(&ini->eaddr)", taking false branch. /home/sbrivio/passt/fwd.c:1095:2: 10.2. path: Condition "!inany_is_loopback(&ini->oaddr)", taking false branch. /home/sbrivio/passt/fwd.c:1112:2: 10.3. path: Condition "proto == IPPROTO_UDP", taking true branch. /home/sbrivio/passt/fwd.c:1116:2: 10.4. dereference: Dereferencing pointer "rule".

tgtpif = fwd_nat_from_splice(rule, proto, ini, tgt); break;

case PIF_HOST: - fwd = &c->fwd_in; - - if (!(rule = fwd_rule_search(fwd, ini, proto, rule_hint))) - goto norule; -

...and not this either: /home/sbrivio/passt/flow.c:532:3: Type: Explicit null dereferenced (FORWARD_NULL) /home/sbrivio/passt/flow.c:508:2: 1. assign_zero: Assigning: "rule" = "NULL". /home/sbrivio/passt/flow.c:511:2: 2. path: Condition "flow_new_entry == flow", taking true branch. /home/sbrivio/passt/flow.c:511:2: 3. path: Condition "f->state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:512:2: 4. path: Condition "f->type == FLOW_TYPE_NONE", taking true branch. /home/sbrivio/passt/flow.c:513:2: 5. path: Condition "f->pif[0] != PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:513:2: 6. path: Condition "f->pif[1] == PIF_NONE", taking true branch. /home/sbrivio/passt/flow.c:514:2: 7. path: Condition "flow->f.state == FLOW_STATE_INI", taking true branch. /home/sbrivio/passt/flow.c:516:2: 8. path: Condition "fwd", taking false branch. /home/sbrivio/passt/flow.c:521:2: 9. path: Switch case value "PIF_HOST". /home/sbrivio/passt/flow.c:532:3: 10. var_deref_model: Passing null pointer "rule" to "fwd_nat_from_host", which dereferences it. /home/sbrivio/passt/fwd.c:1173:2: 10.1. dereference: Dereferencing pointer "rule". I haven't checked why.

tgtpif = fwd_nat_from_host(c, rule, proto, ini, tgt); fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac); break;

[...]

-- Stefano