From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=EbybtuOG; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 35D995A061B for ; Wed, 27 Nov 2024 05:27:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732681642; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=i12jFnfhOA+BKjLftxu6nqslrJ399sBw0ZRtMgY3gwU=; b=EbybtuOGWJXV1QURau/neUQSAf1OnF3BF/KuDZBivimJFkaPRj9Mn3C/+qJuK61QErypXn 82Tz0wFi7l/1QoEilvfgMuEaIOpI8G0OqZq7Z7s6c0720YhrWG892uHdX0Alo1VSzGsG19 BZHAdzo1Wd5NNjxrFpAzctALL1rnU48= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-408-8BixGMepM4-9hoAfxJrJpw-1; Tue, 26 Nov 2024 23:27:20 -0500 X-MC-Unique: 8BixGMepM4-9hoAfxJrJpw-1 X-Mimecast-MFC-AGG-ID: 8BixGMepM4-9hoAfxJrJpw Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-3822ec50b64so3473503f8f.1 for ; Tue, 26 Nov 2024 20:27:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732681639; x=1733286439; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=i12jFnfhOA+BKjLftxu6nqslrJ399sBw0ZRtMgY3gwU=; b=sEr86uMDxY6Ozjj9AAob/jA8j3bsxwPyfSl+rJ2EEW5eqmqLIp+lguZjJd0ebzoaMA 5wtpGkZy59D1Jf+OD5nLN4iy4/BeVoYt0Ggvqsq3Zm6/rxhPqwYZ5fBaBcGJWEBC2DYA tL2+KwYtJvwlRI+PYWO3l8OsaLDRi7vYSHSkDzW6S6JPBC7ss+sqIJXxoABgmMlYtN+9 2SOrqgMy9gY6EjG0q/Sf3n0qLLIrSQR2ySr/ogyDjeMURs3xZ0J/wCltUp6698gLD+LE twqyyAFyxHyCueqWPF3FJPrSAuyjuWhBO4zaaldqjHpyuQO8d1DYvFDeUpcgJhmWhDqE FwvA== X-Gm-Message-State: AOJu0Yz3Nxj81Z02/K5iYzSHj3JPz+eQ65649Cb+Ls+3QVnKL8hrAkEO LXWx2N0Egz8kewTaCdun5/v8lM7HqOySl+Vyd+qoMJLpYLWFoxkD198USg/mMmlePZEO/ioADCu vmOb4a45Btgj1U16pabfEr2p+RmY4MwIwrnh32OkkXiOqlQbdmQ== X-Gm-Gg: ASbGncu7XBs4HoI2QX7gx5nXIsvM9ZllkSRFbxGnUjHz4EGUj3N22zw4zRwQn5XnOBa HxW6SNMmrJspmWsbwoCW18n6jGKToGmYoCrwEOzWZQLg0QGpneNGJY63SI+4rh2zYAW2BPlYuLj FvtUXujjUczqksN4GJBOvNPqH2olscGOa2KAuU2l/PyOArMhrwQkcmVZDD9Zw46My3wU6qFojZN lUcSAQKD5jPcE7wX64vu8uGf/mavg5BDqWXXhaU5Cw2u+VrGXixZ0qxVtQbjdgIUBf1mrE8JLG5 5kw= X-Received: by 2002:a5d:6d08:0:b0:37d:45f0:b33 with SMTP id ffacd0b85a97d-385c6eb7d4cmr848504f8f.9.1732681639135; Tue, 26 Nov 2024 20:27:19 -0800 (PST) X-Google-Smtp-Source: AGHT+IE4dCPWFObFWmkcTy9kwFB4eV9NReYTfkafNfsbih2uiNRgs9nu2OtcdPouR2reDxiQxXYcbg== X-Received: by 2002:a5d:6d08:0:b0:37d:45f0:b33 with SMTP id ffacd0b85a97d-385c6eb7d4cmr848493f8f.9.1732681638772; Tue, 26 Nov 2024 20:27:18 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fafe147sm14975445f8f.30.2024.11.26.20.27.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Nov 2024 20:27:17 -0800 (PST) Date: Wed, 27 Nov 2024 05:27:15 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v3] treewide: Introduce 'local mode' for disconnected setups Message-ID: <20241127052715.751d536d@elisabeth> In-Reply-To: References: <20241126055429.1610735-1-sbrivio@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: uc5uOwleGiVtmXxFBzYNt-GlaI-pWHGgRSSsPUEZIEI_1732681639 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: L2ZOLZLJHRS5F6RUQZ45WZDAJXPDYGA6 X-Message-ID-Hash: L2ZOLZLJHRS5F6RUQZ45WZDAJXPDYGA6 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed, 27 Nov 2024 12:51:15 +1100 David Gibson 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 > > 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