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

On Mon, Jul 08, 2024 at 11:26:55PM +0200, Stefano Brivio wrote:

On Fri, 5 Jul 2024 12:07:16 +1000 David Gibson 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 --- 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