Re: [PATCH 3/5] fwd: Rework default address logic for inbound flows

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

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

On Mon, Jul 13, 2026 at 02:39:42AM +0200, Stefano Brivio wrote:
> On Fri, 10 Jul 2026 16:56:09 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > fwd_nat_from_host() needs to determine the guest side destination address
> > for the new flow.  In some cases that's controlled by the forwarding rule
> > or -host-lo-to-ns-lo logic, but by default we use the observed guest
> 
> --host-lo-to-ns-lo (I kid you not, I sometimes grep for option names in
> commit logs).

Fixed.

> > address.  We need to pick the right one to match the source address,
> > though.
> > 
> > Currently this is done with similar, but not quite identical logic in the
> > spliced and non-spliced paths.  Introduce a new fwd_default_guest_addr()
> > helper to make explicit:
> >  * We have the same logic for splice and tap paths
> >  * This is a fallback path if nothing else determined the address (we
> >    use this default nearly all the time now, but it might change in future)
> >  * We're matching IP family and scope with the guest side source address
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd.c | 44 ++++++++++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fwd.c b/fwd.c
> > index 84400948..90297ef7 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -1005,6 +1005,27 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
> >  	return true;
> >  }
> >  
> > +/**
> > + * fwd_default_geuest_addr() - Get appropriate guest address to send to
> 
> fwd_default_guest_addr()

Fixed.

> > + * @c:		Execution context
> > + * @guest_addr:	Updated with chosen guest address
> > + * @template:	Address to match IP version and scope of
> > + *
> > + * Sets @guest_addr to have the address of the guest, matching the IP version
> > + * and scope of @template where possible.
> 
> Could we return the right @guest_addr instead? Or you want to avoid
> that because we can't use a pointer to inany_addr in that case?

I avoided it because of a) fear of extra copies of the IPv6 address
and b) stylistically we rarely return structs or unions or values by
value, although that is allowed in modern C.

But you're right, a return by value would be clearer here.  It's per
flow, not per packet so it's not *super* hot, and I'm by no means
certain that the compiler won't turn the return by value into
something as good or better than the return via pointer.

Changed to return the addres by value.

> It would make:
> 
> > + */
> > +static void fwd_default_guest_addr(const struct ctx *c,
> > +				   union inany_addr *guest_addr,
> > +				   const union inany_addr *template)
> > +{
> > +	if (inany_v4(template))
> > +		*guest_addr = inany_from_v4(c->ip4.addr_seen);
> > +	else if (inany_is_linklocal6(template))
> > +		guest_addr->a6 = c->ip6.addr_ll_seen;
> > +	else
> > +		guest_addr->a6 = c->ip6.addr_seen;
> > +}
> > +
> >  /**
> >   * fwd_nat_from_host() - Determine to forward a flow from the host interface
> >   * @c:		Execution context
> > @@ -1040,13 +1061,9 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
> >  		 */
> >  		if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
> >  			tgt->eaddr = ini->oaddr;
> > -		else if (inany_v4(&ini->eaddr))
> > -			tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> > -		else
> > -			tgt->eaddr.a6 = c->ip6.addr_seen;
> >  
> >  		/* Let the kernel pick source address and port */
> > -		if (inany_v4(&tgt->eaddr))
> > +		if (inany_v4(&ini->eaddr))
> >  			tgt->oaddr = inany_any4;
> >  		else
> >  			tgt->oaddr = inany_any6;
> > @@ -1056,6 +1073,9 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
> >  			/* But for UDP preserve the source port */
> >  			tgt->oport = ini->eport;
> >  
> > +		/* Use guest address as destination, if otherwise unspecified */
> > +		if (inany_is_unspecified(&tgt->eaddr))
> > +			fwd_default_guest_addr(c, &tgt->eaddr, &tgt->oaddr);
> >  		return PIF_SPLICE;
> >  	}
> >  
> > @@ -1074,16 +1094,12 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
> >  	}
> >  	tgt->oport = ini->eport;
> >  
> > -	if (!inany_is_unspecified(&rule->taddr)) {
> > +	if (!inany_is_unspecified(&rule->taddr))
> >  		tgt->eaddr = rule->taddr;
> > -	} else if (inany_v4(&tgt->oaddr)) {
> > -		tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> > -	} else {
> > -		if (inany_is_linklocal6(&tgt->oaddr))
> > -			tgt->eaddr.a6 = c->ip6.addr_ll_seen;
> > -		else
> > -			tgt->eaddr.a6 = c->ip6.addr_seen;
> > -	}
> > +
> > +	/* Use guest address as destination, if otherwise unspecified */
> > +	if (inany_is_unspecified(&tgt->eaddr))
> > +		fwd_default_guest_addr(c, &tgt->eaddr, &tgt->oaddr);
> 
> this usage more intuitive, I think.
> 
> >  
> >  	return PIF_TAP;
> >  }
> 
> -- 
> 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 --]