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

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

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

On Wed, Mar 25, 2026 at 01:54:24AM +0100, Stefano Brivio wrote:
> On Mon, 23 Mar 2026 18:37:14 +1100
> David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>

[snip]
> /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;

Oops.  That's because we're relying on the fact that we _do_ have
populated tables for HOST and SPLICE.  That's constructed a very long
way away, so of course coverity can't figure that out.  I've fixed it
with assert()s for now, and those should go away if/when we implement
forwarding tables for TAP.

-- 
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 --]