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

[Stefano Brivio <sbrivio@redhat.com>]

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.

>  		if (inany_equals4(&tgt->eaddr, &c->ip4.gw))
>  			tgt->eaddr = inany_loopback4;
>  		else if (inany_equals6(&tgt->eaddr, &c->ip6.gw))
> @@ -191,6 +194,10 @@ uint8_t fwd_nat_from_tap(const struct ctx *c, uint8_t proto,
>  
>  	/* Let the kernel pick a host side source port */
>  	tgt->fport = 0;
> +	if (proto == IPPROTO_UDP) {
> +		/* But for UDP we preserve the source port */
> +		tgt->fport = ini->eport;
> +	}
>  
>  	return PIF_HOST;
>  }
> @@ -232,9 +239,14 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
>  	tgt->eport = ini->fport;
>  	if (proto == IPPROTO_TCP)
>  		tgt->eport += c->tcp.fwd_out.delta[tgt->eport];
> +	else if (proto == IPPROTO_UDP)
> +		tgt->eport += c->udp.fwd_out.f.delta[tgt->eport];
>  
>  	/* Let the kernel pick a host side source port */
>  	tgt->fport = 0;
> +	if (proto == IPPROTO_UDP)
> +		/* But for UDP preserve the source port */
> +		tgt->fport = ini->eport;
>  
>  	return PIF_HOST;
>  }
> @@ -256,20 +268,26 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  	tgt->eport = ini->fport;
>  	if (proto == IPPROTO_TCP)
>  		tgt->eport += c->tcp.fwd_in.delta[tgt->eport];
> +	else if (proto == IPPROTO_UDP)
> +		tgt->eport += c->udp.fwd_in.f.delta[tgt->eport];
>  
>  	if (c->mode == MODE_PASTA && inany_is_loopback(&ini->eaddr) &&
> -	    proto == IPPROTO_TCP) {
> +	    (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) {
>  		/* spliceable */
>  
>  		/* Preserve the specific loopback adddress used, but let the
>  		 * kernel pick a source port on the target side */
>  		tgt->faddr = ini->eaddr;
>  		tgt->fport = 0;
> +		if (proto == IPPROTO_UDP)
> +			/* But for UDP preserve the source port */
> +			tgt->fport = ini->eport;
>  
>  		if (inany_v4(&ini->eaddr))
>  			tgt->eaddr = inany_loopback4;
>  		else
>  			tgt->eaddr = inany_loopback6;
> +
>  		return PIF_SPLICE;
>  	}
>  

-- 
Stefano