Re: [PATCH v7 19/27] fwd: Update flow forwarding logic for UDP

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

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

On Mon, Jul 08, 2024 at 11:26:55PM +0200, Stefano Brivio wrote:
> On Fri,  5 Jul 2024 12:07:16 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Add logic to the fwd_nat_from_*() functions to forwarding UDP packets.  The
> > logic here doesn't exactly match our current forwarding, since our current
> > forwarding has some very strange and buggy edge cases.  Instead it's
> > attempting to replicate what appears to be the intended logic behind the
> > current forwarding.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  fwd.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fwd.c b/fwd.c
> > index 5731a536..4377de44 100644
> > --- a/fwd.c
> > +++ b/fwd.c
> > @@ -169,12 +169,15 @@ void fwd_scan_ports_init(struct ctx *c)
> >  uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
> >  			 const struct flowside *ini, struct flowside *tgt)
> >  {
> > -	(void)proto;
> > -
> >  	tgt->eaddr = ini->faddr;
> >  	tgt->eport = ini->fport;
> >  
> > -	if (!c->no_map_gw) {
> > +	if (proto == IPPROTO_UDP && tgt->eport == 53) {
> > +		if (inany_equals4(&tgt->eaddr, &c->ip4.dns_match))
> > +			tgt->eaddr = inany_from_v4(c->ip4.dns_host);
> > +		else if (inany_equals6(&tgt->eaddr, &c->ip6.dns_match))
> > +			tgt->eaddr.a6 = c->ip6.dns_host;
> > +	} else if (!c->no_map_gw) {
> 
> There's a subtle difference here compared to the logic you dropped in
> 23/27 (udp_tap_handler()), which doesn't look correct to me.
> 
> Earlier, with neither c->ip4.dns_match nor c->ip6.dns_match matching,
> we would let UDP traffic directed to port 53 be mapped to the host, if
> (!c->no_map_gw). That is, the logic was rather equivalent to this:
> 
> 	if (proto == IPPROTO_UDP && tgt->eport == 53 &&
> 	    (inany_equals4(&tgt->eaddr, &c->ip4.dns_match) ||
> 	     inany_equals6(&tgt->eaddr, &c->ip6.dns_match)) {
> 		if (inany_equals4(&tgt->eaddr, &c->ip4.dns_match))
> 			tgt->eaddr = inany_from_v4(c->ip4.dns_host);
> 		else if (inany_equals6(&tgt->eaddr, &c->ip6.dns_match))
> 			tgt->eaddr.a6 = c->ip6.dns_host;
> 	} else if (!c->no_map_gw) {
> 		...
> 
> and I think we should maintain it, because if dns_match doesn't match,
> DNS traffic considerations shouldn't affect NAT decisions at all.

Good catch, I've adjusted that.

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