Re: [PATCH v3] treewide: Introduce 'local mode' for disconnected setups

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 27 Nov 2024 12:51:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Nov 26, 2024 at 06:54:29AM +0100, Stefano Brivio wrote:
> > There are setups where no host interface is available or configured
> > at all, intentionally or not, temporarily or not, but users expect
> > (Podman) containers to run in any case as they did with slirp4netns,
> > and we're now getting reports that we broke such setups at a rather
> > alarming rate.
> > 
> > To this end, if we don't find any usable host interface, instead of
> > exiting:
> > 
> > - for IPv4, use 169.254.2.1 as guest/container address and 169.254.2.2
> >   as default gateway
> > 
> > - for IPv6, don't assign any address (forcibly disable DHCPv6), and
> >   use the *first* link-local address we observe to represent the
> >   guest/container. Advertise fe80::1 as default gateway
> > 
> > - use 'tap0' as default interface name for pasta
> > 
> > Change ifi4 and ifi6 in struct ctx to int and accept a special -1
> > value meaning that no host interface was selected, but the IP family
> > is enabled. The fact that the kernel uses unsigned int values for
> > those is not an issue as 1. one can't create so many interfaces
> > anyway and 2. we otherwise handle those values transparently.
> > 
> > Fix a botched conditional in conf_print() to actually skip printing
> > DHCPv6 information if DHCPv6 is disabled (and skip printing NDP
> > information if NDP is disabled).
> > 
> > Link: https://github.com/containers/podman/issues/24614
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> One remaining concern noted below.  Sorry I didn't pick it up on the
> previous round.
> 
> > ---
> > v3: Coverity reports that, in conf(), we might supply a negative
> >     c->ifi4 to if_indextoname() after checking that (!*c->pasta_ifn).
> > 
> >     That's a false positive, because if c->ifi4 is -1, we already set
> >     c->pasta_ifn to "tap0", so we won't call if_indextoname() at all,
> >     but, to make my life simpler, add a redundant check on c->ifi4
> >     and c->ifi6 before calling if_indextoname() on them.
> > 
> >  conf.c  | 96 ++++++++++++++++++++++++++++++++++++++++++++-------------
> >  passt.1 | 33 +++++++++++++++++---
> >  passt.h |  8 ++---
> >  pasta.c |  7 +++--
> >  tap.c   |  3 ++
> >  5 files changed, 115 insertions(+), 32 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 86566db..c7aabeb 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -48,6 +48,20 @@
> >  
> >  #define NETNS_RUN_DIR	"/run/netns"
> >  
> > +#define IP4_LL_GUEST_ADDR	(struct in_addr){ htonl_constant(0xa9fe0201) }
> > +				/* 169.254.2.1, libslirp default: 10.0.2.1 */
> > +
> > +#define IP4_LL_GUEST_GW		(struct in_addr){ htonl_constant(0xa9fe0202) }
> > +				/* 169.254.2.2, libslirp default: 10.0.2.2 */
> > +
> > +#define IP4_LL_PREFIX_LEN	16
> > +
> > +#define IP6_LL_GUEST_GW		(struct in6_addr)			\
> > +				{{{ 0xfe, 0x80, 0, 0, 0, 0, 0, 0,	\
> > +				       0, 0, 0, 0, 0, 0, 0, 0x01 }}}
> > +
> > +const char *pasta_default_ifn = "tap0";
> > +
> >  /**
> >   * next_chunk - Return the next piece of a string delimited by a character
> >   * @s:		String to search
> > @@ -631,7 +645,7 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
> >  		ifi = nl_get_ext_if(nl_sock, AF_INET);
> >  
> >  	if (!ifi) {
> > -		info("Couldn't pick external interface: disabling IPv4");
> > +		debug("Failed to detect external interface for IPv4");
> >  		return 0;
> >  	}
> >  
> > @@ -639,8 +653,8 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
> >  		int rc = nl_route_get_def(nl_sock, ifi, AF_INET,
> >  					  &ip4->guest_gw);
> >  		if (rc < 0) {
> > -			err("Couldn't discover IPv4 gateway address: %s",
> > -			    strerror(-rc));
> > +			debug("Couldn't discover IPv4 gateway address: %s",
> > +			      strerror(-rc));
> >  			return 0;
> >  		}
> >  	}
> > @@ -649,8 +663,8 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
> >  		int rc = nl_addr_get(nl_sock, ifi, AF_INET,
> >  				     &ip4->addr, &ip4->prefix_len, NULL);
> >  		if (rc < 0) {
> > -			err("Couldn't discover IPv4 address: %s",
> > -			    strerror(-rc));
> > +			debug("Couldn't discover IPv4 address: %s",
> > +			      strerror(-rc));
> >  			return 0;
> >  		}
> >  	}
> > @@ -677,6 +691,19 @@ static unsigned int conf_ip4(unsigned int ifi, struct ip4_ctx *ip4)
> >  	return ifi;
> >  }
> >  
> > +/**
> > + * conf_ip4_local() - Configure IPv4 addresses and attributes for local mode
> > + * @ip4:	IPv4 context (will be written)
> > + */
> > +static void conf_ip4_local(struct ip4_ctx *ip4)
> > +{
> > +	ip4->addr_seen = ip4->addr = IP4_LL_GUEST_ADDR;
> > +	ip4->our_tap_addr = ip4->guest_gw = IP4_LL_GUEST_GW;
> > +	ip4->prefix_len = IP4_LL_PREFIX_LEN;
> > +
> > +	ip4->no_copy_addrs = ip4->no_copy_routes = true;
> > +}
> > +
> >  /**
> >   * conf_ip6() - Verify or detect IPv6 support, get relevant addresses
> >   * @ifi:	Host interface to attempt (0 to determine one)
> > @@ -693,15 +720,15 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
> >  		ifi = nl_get_ext_if(nl_sock, AF_INET6);
> >  
> >  	if (!ifi) {
> > -		info("Couldn't pick external interface: disabling IPv6");
> > +		debug("Failed to detect external interface for IPv6");
> >  		return 0;
> >  	}
> >  
> >  	if (IN6_IS_ADDR_UNSPECIFIED(&ip6->guest_gw)) {
> >  		rc = nl_route_get_def(nl_sock, ifi, AF_INET6, &ip6->guest_gw);
> >  		if (rc < 0) {
> > -			err("Couldn't discover IPv6 gateway address: %s",
> > -			    strerror(-rc));
> > +			debug("Couldn't discover IPv6 gateway address: %s",
> > +			      strerror(-rc));
> >  			return 0;
> >  		}
> >  	}
> > @@ -710,7 +737,7 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
> >  			 IN6_IS_ADDR_UNSPECIFIED(&ip6->addr) ? &ip6->addr : NULL,
> >  			 &prefix_len, &ip6->our_tap_ll);
> >  	if (rc < 0) {
> > -		err("Couldn't discover IPv6 address: %s", strerror(-rc));
> > +		debug("Couldn't discover IPv6 address: %s", strerror(-rc));
> >  		return 0;
> >  	}
> >  
> > @@ -726,6 +753,17 @@ static unsigned int conf_ip6(unsigned int ifi, struct ip6_ctx *ip6)
> >  	return ifi;
> >  }
> >  
> > +/**
> > + * conf_ip6_local() - Configure IPv6 addresses and attributes for local mode
> > + * @ip6:	IPv6 context (will be written)
> > + */
> > +static void conf_ip6_local(struct ip6_ctx *ip6)
> > +{
> > +	ip6->our_tap_ll = ip6->guest_gw = IP6_LL_GUEST_GW;
> > +
> > +	ip6->no_copy_addrs = ip6->no_copy_routes = true;
> > +}
> > +
> >  /**
> >   * usage() - Print usage, exit with given status code
> >   * @name:	Executable name
> > @@ -948,12 +986,14 @@ static void conf_print(const struct ctx *c)
> >  	char bufmac[ETH_ADDRSTRLEN], ifn[IFNAMSIZ];
> >  	int i;
> >  
> > -	info("Template interface: %s%s%s%s%s",
> > -	     c->ifi4 ? if_indextoname(c->ifi4, ifn) : "",
> > -	     c->ifi4 ? " (IPv4)" : "",
> > -	     (c->ifi4 && c->ifi6) ? ", " : "",
> > -	     c->ifi6 ? if_indextoname(c->ifi6, ifn) : "",
> > -	     c->ifi6 ? " (IPv6)" : "");
> > +	if (c->ifi4 > 0 || c->ifi6 > 0) {
> > +		info("Template interface: %s%s%s%s%s",
> > +		     c->ifi4 > 0 ? if_indextoname(c->ifi4, ifn) : "",
> > +		     c->ifi4 > 0 ? " (IPv4)" : "",
> > +		     (c->ifi4 && c->ifi6) ? ", " : "",
> > +		     c->ifi6 > 0 ? if_indextoname(c->ifi6, ifn) : "",
> > +		     c->ifi6 > 0 ? " (IPv6)" : "");
> > +	}
> >  
> >  	if (*c->ip4.ifname_out || *c->ip6.ifname_out) {
> >  		info("Outbound interface: %s%s%s%s%s",
> > @@ -1024,9 +1064,9 @@ static void conf_print(const struct ctx *c)
> >  
> >  		if (!c->no_ndp && !c->no_dhcpv6)
> >  			info("NDP/DHCPv6:");
> > -		else if (!c->no_ndp)
> > -			info("DHCPv6:");
> >  		else if (!c->no_dhcpv6)
> > +			info("DHCPv6:");
> > +		else if (!c->no_ndp)
> >  			info("NDP:");
> >  		else
> >  			goto dns6;
> > @@ -1735,8 +1775,20 @@ void conf(struct ctx *c, int argc, char **argv)
> >  		c->ifi6 = conf_ip6(ifi6, &c->ip6);
> >  	if ((!c->ifi4 && !c->ifi6) ||
> >  	    (*c->ip4.ifname_out && !c->ifi4) ||
> > -	    (*c->ip6.ifname_out && !c->ifi6))
> > -		die("External interface not usable");
> > +	    (*c->ip6.ifname_out && !c->ifi6)) {  
> 
> The fallback path makes sense for !c->ifi4 && !c->ifi6, but I don't
> think it makes sense for the other conditions on this if.  Those cover
> where the user explicitly gave an interface, but we couldn't use it
> for whatever reason.  In those cases I think we should outright fail,
> rather than falling back to local mode.

Oops, right, for some reason I read "*c->ip4.ifname_out" as something
on the lines of "we found an interface but it can't be used", which is
obviously wrong. Sending v4.

Paul, I'm dropping your Tested-by: tag, even if the difference to v3 is
really small, a quick re-test would be appreciated as I can't exclude
I'm breaking something completely unexpected with it.

-- 
Stefano