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

[Paul Holzinger <pholzing@redhat.com>]

On 27/11/2024 05:27, Stefano Brivio wrote:
> 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.
Tested v4 and it seems fine but I must say you confused a fair bit by 
already applying that patch. I was wondering why I was getting conflicts 
from git am until I noticed the patch is already in the tree.

-- 
Paul Holzinger