On Tue, Aug 20, 2024 at 09:56:24PM +0200, Stefano Brivio wrote: > On Fri, 16 Aug 2024 15:39:58 +1000 > David Gibson 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, Currently, when we don't have a gateway address on the host: no connectivity, or a point-to-point link with no gateway, or the like. We used to absolutely require it, but that restriction has been eased and may ease further in future. > 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. Fair point. I've removed the disabling of DHCP in this case. > > > and forwarding of > > inbound connections with guest-inaccessible source addresses. > > > > Signed-off-by: David Gibson > > --- > > 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. Makes sense, done. -- 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