Re: [PATCH 17/22] fwd: Split notion of "our tap address" from gateway for IPv4

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 16 Aug 2024 15:39:58 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> ip4.gw conflates 3 conceptually different things, which (for now) have the
> same value:
>   1. The router/gateway address as seen by the guest
>   2. An address to NAT to the host with --no-map-gw isn't specified
>   3. An address to use as source when nothing else makes sense
> 
> Case 3 occurs in two situations:
> 
> a) for our DHCP responses - since they come from passt internally there's
>    no naturally meaningful address for them to come from
> b) for forwarded connections coming from an address that isn't guest
>    accessible (localhost or the guest's own address).
> 
> (b) occurs even with --no-map-gw, and the expected behaviour of forwarding
> local connections requires it.
> 
> For IPv6 role (3) is now taken by ip6.our_tap_ll (which usually has the
> same value as ip6.gw).  For future flexibility we may want to make this
> "address of last resort" different from the gateway address, so split them
> logically for IPv4 as well.
> 
> Specifically, add a new ip4.our_tap_addr field for the address with this
> role, and initialise it to ip4.gw for now.  Unlike IPv6 where we can always
> get a link-local address, we might not be able to get a (non 0.0.0.0)
> address here.  In that case we have to disable DHCP

It's not entirely clear to me in which case we would not be able to
get any address, but at least RFC 2131 doesn't have a problem with this:

diff --git a/dhcp.c b/dhcp.c
index aa9f59d..3de8a6e 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -282,6 +282,7 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	struct in_addr mask;
 	unsigned int i;
 	struct msg *m;
+	struct in_addr zeroes = { 0 };
 
 	eh  = packet_get(p, 0, offset, sizeof(*eh),  NULL);
 	offset += sizeof(*eh);
@@ -378,7 +379,7 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opt_set_dns_search(c, sizeof(m->o));
 
 	dlen = offsetof(struct msg, o) + fill(m);
-	tap_udp4_send(c, c->ip4.gw, 67, c->ip4.addr, 68, m, dlen);
+	tap_udp4_send(c, zeroes, 67, c->ip4.addr, 68, m, dlen);
 
 	return 1;
 }

and:

$ ./pasta -p dhcp.pcap
Saving packet capture to dhcp.pcap
# dhclient
# tshark -r dhcp.pcap
Running as user "root" and group "root". This could be dangerous.
    1   0.000000           :: → ff02::16     ICMPv6 90 Multicast Listener Report Message v2
    2   0.016265      0.0.0.0 → 255.255.255.255 DHCP 342 DHCP Discover - Transaction ID 0x75759d11
    3   0.016361      0.0.0.0 → 88.198.0.164 DHCP 342 DHCP Offer    - Transaction ID 0x75759d11
    4   0.016479      0.0.0.0 → 255.255.255.255 DHCP 342 DHCP Request  - Transaction ID 0x75759d11
    5   0.016493      0.0.0.0 → 88.198.0.164 DHCP 342 DHCP ACK      - Transaction ID 0x75759d11
[...]

so this could be a reasonable fallback.

> and forwarding of
> inbound connections with guest-inaccessible source addresses.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c  |  7 ++++++-
>  dhcp.c  |  4 ++--
>  fwd.c   | 10 +++++++---
>  passt.h |  2 ++
>  4 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 954f20ea..9f962fc8 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -660,6 +660,8 @@ static unsigned int conf_ip4(unsigned int ifi,
>  
>  	ip4->addr_seen = ip4->addr;
>  
> +	ip4->our_tap_addr = ip4->gw;
> +
>  	if (MAC_IS_ZERO(mac)) {
>  		int rc = nl_link_get_mac(nl_sock, ifi, mac);
>  		if (rc < 0) {
> @@ -1666,7 +1668,10 @@ void conf(struct ctx *c, int argc, char **argv)
>  		die("External interface not usable");
>  
>  	if (c->ifi4 && IN4_IS_ADDR_UNSPECIFIED(&c->ip4.gw))
> -		c->no_map_gw = c->no_dhcp = 1;
> +		c->no_map_gw = 1;
> +
> +	if (c->ifi4 && IN4_IS_ADDR_UNSPECIFIED(&c->ip4.our_tap_addr))
> +		c->no_dhcp = 1;
>  
>  	if (c->ifi6 && IN6_IS_ADDR_UNSPECIFIED(&c->ip6.gw))
>  		c->no_map_gw = 1;
> diff --git a/dhcp.c b/dhcp.c
> index acc5b03e..a935dc94 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -347,7 +347,7 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  	mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len));
>  	memcpy(opts[1].s,  &mask,        sizeof(mask));
>  	memcpy(opts[3].s,  &c->ip4.gw,   sizeof(c->ip4.gw));
> -	memcpy(opts[54].s, &c->ip4.gw,   sizeof(c->ip4.gw));
> +	memcpy(opts[54].s, &c->ip4.our_tap_addr, sizeof(c->ip4.our_tap_addr));

Nit: this was supposed to look like a table, so it would be nice to add
extra whitespace in the lines above this one.

-- 
Stefano