Re: [PATCH v3 11/14] flow, fwd: Consult rules table when forwarding a new flow from socket

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

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

On Tue, Jan 13, 2026 at 11:12:01PM +0100, Stefano Brivio wrote:
> Silly nits only and a couple of remarks that will probably be clarified
> at a later point:
> 
> On Thu,  8 Jan 2026 13:29:45 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We now have a formal array of forwarding rules.  However, we don't actually
> > consult it when we forward a new flow.  Instead we rely on (a) implicit
> > information (we wouldn't be here if there wasn't a listening socket for the
> > rule) and (b) the legacy delta[] data structure.
> > 
> > Start addressing this, by searching for a matching forwarding rule when
> > attempting to forward a new flow.  For now this is incomplete:
> >  * We only do this for socket-initiated flows
> >  * We make a potentially costly linear lookup
> >  * We don't actually use the matching rule for anything yet
> > 
> > We'll address each of those in later patches.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  flow.c | 43 ++++++++++++++++++++++++++++++++++---------
> >  fwd.c  | 33 +++++++++++++++++++++++++++++++++
> >  fwd.h  |  2 ++
> >  3 files changed, 69 insertions(+), 9 deletions(-)
> > 
> > diff --git a/flow.c b/flow.c
> > index 4f534865..045e9712 100644
> > --- a/flow.c
> > +++ b/flow.c
> > @@ -489,7 +489,7 @@ struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif,
> >  struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  			     uint8_t proto)
> >  {
> > -	char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN];
> > +	char estr[INANY_ADDRSTRLEN], ostr[INANY_ADDRSTRLEN];
> >  	struct flow_common *f = &flow->f;
> >  	const struct flowside *ini = &f->side[INISIDE];
> >  	struct flowside *tgt = &f->side[TGTSIDE];
> > @@ -500,6 +500,30 @@ 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 (pif_is_socket(f->pif[INISIDE])) {
> > +		const struct fwd_ports *fwd;
> > +
> > +		if (f->pif[INISIDE] == PIF_HOST && proto == IPPROTO_TCP)
> > +			fwd = &c->tcp.fwd_in;
> > +		else if (f->pif[INISIDE] == PIF_HOST && proto == IPPROTO_UDP)
> > +			fwd = &c->udp.fwd_in;
> > +		else if (f->pif[INISIDE] == PIF_SPLICE && proto == IPPROTO_TCP)
> > +			fwd = &c->tcp.fwd_out;
> > +		else if (f->pif[INISIDE] == PIF_SPLICE && proto == IPPROTO_UDP)
> > +			fwd = &c->udp.fwd_out;
> > +		else
> > +			goto nofwd;
> > +
> > +		if (!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
> > +			 */
> > +			flow_dbg(flow, "Unexpected missing forward rule");
> > +			goto nofwd;
> > +		}
> > +	}
> > +
> >  	switch (f->pif[INISIDE]) {
> >  	case PIF_TAP:
> >  		memcpy(f->tap_omac, MAC_UNDEF, ETH_ALEN);
> > @@ -514,22 +538,23 @@ struct flowside *flow_target(const struct ctx *c, union flow *flow,
> >  		tgtpif = fwd_nat_from_host(c, proto, ini, tgt);
> >  		fwd_neigh_mac_get(c, &tgt->oaddr, f->tap_omac);
> >  		break;
> > -
> >  	default:
> > -		flow_err(flow, "No rules to forward %s [%s]:%hu -> [%s]:%hu",
> > -			 pif_name(f->pif[INISIDE]),
> > -			 inany_ntop(&ini->eaddr, estr, sizeof(estr)),
> > -			 ini->eport,
> > -			 inany_ntop(&ini->oaddr, fstr, sizeof(fstr)),
> > -			 ini->oport);
> > +		goto nofwd;
> >  	}
> >  
> >  	if (tgtpif == PIF_NONE)
> > -		return NULL;
> > +		goto nofwd;
> >  
> >  	f->pif[TGTSIDE] = tgtpif;
> >  	flow_set_state(f, FLOW_STATE_TGT);
> >  	return tgt;
> > +
> > +nofwd:
> > +	flow_err(flow, "No rules to forward %s %s [%s]:%hu -> [%s]:%hu",
> > +		 pif_name(f->pif[INISIDE]), ipproto_name(proto),
> > +		 inany_ntop(&ini->eaddr, estr, sizeof(estr)), ini->eport,
> > +		 inany_ntop(&ini->oaddr, ostr, sizeof(ostr)), ini->oport);
> 
> This assumes we're still using this function for inbound forwards only
> (eaddr / eport -> oaddr / oport), perhaps we'll want a macro once it
> starts being used for the other way around as well (if at all).

No, that's correct for outbound as well.  Both the addresses are from
the initiating side (there is no target side, because we can't
forward).  It's always src -> dst for the exchange initiating the flow.

> By the way, for rules, earlier in this series, you used "=>" to separate
> source and target of the forward, here it's still "->", I guess we
> should settle on one version (it just occurred to me while testing
> stuff: it might be useful to grep in logs).

As above, this is actually consistent.  => separates sides, both
addresses are on the initiating side here.

> 
> > +	return NULL;
> >  }
> >  
> >  /**
> > diff --git a/fwd.c b/fwd.c
> > index 3f088fd2..633ba5db 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -409,6 +409,39 @@ void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> >  	}
> >  }
> >  
> > +/**
> > + * fwd_rule_match() - Does a prospective flow match a given forwarding rule
> 
> Does [...]?

Fixed.

> > + * @rule:	Forwarding rule
> > + * @ini:	Initiating side flow information
> > + *
> > + * Returns: true if the rule applies to the flow, false otherwise
> > + */
> > +static bool fwd_rule_match(const struct fwd_rule *rule,
> > +			   const struct flowside *ini)
> > +{
> > +	return inany_matches(&ini->oaddr, fwd_rule_addr(rule)) &&
> > +		ini->oport >= rule->first && ini->oport <= rule->last;
> 
> The usual alignment is:
> 
> 	return inany_matches(&ini->oaddr, fwd_rule_addr(rule)) &&
> 	       ini->oport >= rule->first && ini->oport <= rule->last;

Huh.  My emacs config almost always matches the passt style, but not
in this case (I think it treats bare expressions like this differently
from function parameters).

> > +}
> > +
> > +/**
> > + * fwd_rule_search() - Find a rule which matches a prospective flow
> > + * @fwd:	Forwarding table
> > + * @ini:	Initiating side flow information
> > + *
> > + * Returns: first matching rule, or NULL if there is none
> 
> I guess that this will eventually need to become a function matching
> the most specific rule first, tie breakers could be:

Maybe, but I'm hoping not.  The current model I have in mind is always
first rule matches - if you want most specific first, then you need to
order them like that.  Potentially re-ordering by specificity is
something the update client could do.

In any case, I think that's something we want to avoid doing at the
point we actually look up the table.

> 1. specific address given vs. wildcard
> 2. specific interface given vs. no interface
> 3. the day we support/need it: specific port/range vs. no port
> 4. smallest port range
> 5. the day we support/need something like this: longest prefix length

I'm not sure specificity rules will always give a total order.  Having
an explicit order lets is disambiguate such cases.

> and after this we should actually have an error on insertion (already
> guaranteed I think).

That's a good idea.  Even while it's first-rule-wins, we could give an
error/warning if a rule is added that's unreachable because an earlier
one will always win.  I think that's slightly more complicated than
the conflicting rules checking I already implemented, but not
dramatically so.

> > + */
> > +const struct fwd_rule *fwd_rule_search(const struct fwd_ports *fwd,
> > +				       const struct flowside *ini)
> > +{
> > +	unsigned i;
> > +
> > +	for (i = 0; i < fwd->count; i++) {
> > +		if (fwd_rule_match(&fwd->rules[i], ini))
> > +			return &fwd->rules[i];
> > +	}
> 
> Extra newline here to clearly separate the two outcomes.

Done.

> > +	return NULL;
> > +}
> > +
> >  /**
> >   * fwd_rules_print() - Print forwarding rules for debugging
> >   * @fwd:	Table to print
> > diff --git a/fwd.h b/fwd.h
> > index f84e7c01..a10bdbb4 100644
> > --- a/fwd.h
> > +++ b/fwd.h
> > @@ -103,6 +103,8 @@ struct fwd_ports {
> >  void fwd_rule_add(struct fwd_ports *fwd, uint8_t flags,
> >  		  const union inany_addr *addr, const char *ifname,
> >  		  in_port_t first, in_port_t last, in_port_t to);
> > +const struct fwd_rule *fwd_rule_search(const struct fwd_ports *fwd,
> > +				       const struct flowside *ini);
> >  void fwd_rules_print(const struct fwd_ports *fwd);
> >  
> >  void fwd_scan_ports_init(struct ctx *c);
> 
> -- 
> 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 --]