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=QSuPeICm; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id A60CD5A004E for ; Wed, 27 Nov 2024 13:35:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1732710925; 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=82VMJLcb09/paM6QkdGhq+w9eptaXoaCDe68ofuTFsg=; b=QSuPeICmechKPQwHS/xwLDhk8CHInGkJBXKoL4Sqt62t1nS2iAfrPb3UTFCiiXJlJudNDc XrcS+RtMnAWMkHsCoGBWmPb/QVVwsaUMCP7HwGnlgwdSdqYzv1eDyeQueaTnDtw+oyL5zu n721BFwWndXw2bXgogX/cKdIsI31/FY= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-487-PKag8XLwPRu3OAXfxZthIA-1; Wed, 27 Nov 2024 07:35:21 -0500 X-MC-Unique: PKag8XLwPRu3OAXfxZthIA-1 X-Mimecast-MFC-AGG-ID: PKag8XLwPRu3OAXfxZthIA Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-434a467e970so16627725e9.2 for ; Wed, 27 Nov 2024 04:35:21 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1732710920; x=1733315720; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=82VMJLcb09/paM6QkdGhq+w9eptaXoaCDe68ofuTFsg=; b=jED3N9CMYk17lMxKsHId9MQ7xioyNFl+ugV4gqThV/6dA5cbdvP8GLOrD8+42q7IrJ GHjmZeLzbqtQef2g4b77hqOhQCmgH/m23/Wdv5tPT0bdp9mOXs3LVS15xnMPggHuzjwW cyXCgDrrROwqMdbsKfiQ1yeOx3kEe22PS+HnDxPKM3ChTuuP+uVAJD+CAMu/anyYB+jo uED92JWHfUV0KdJODX3gLzKHRkex39dVlUKDiHmHhizZh5E0xpmA/4s+nErc8lHRBhLm R5W2pXBhq8py8Z794mw9xB8x9NTnAqXmBvTvfui06AeOCYmkLR9ktcVjJYkui1N8I5PF /cdw== X-Gm-Message-State: AOJu0YycPi00gTdYZ2yB8PYlOnpxsspWWI1kfSn7cfaPI/XixeUgtgVn cjFbKTl9COwxUxQj8Cebj2h2eDus0VC6A8819yrwlFhZApRSxgW3lLVHo6wF0okzANG9GG/8HBv nbBGQd45tznqgdWOz+oq4T5OJOBOdthNpNuUI+r1VfdJTuiPwfw== X-Gm-Gg: ASbGnctiy94YU8QpkNf3DTLEe8mVvYy91WMs6oTkrd0FeA6asMBTT+ydYDJbI8ooora 6d05SlgnbVPDumkr+GxwXWQtL139YpwGao/0B1pencEhe5V1RuyKqDxDqeebg2WefieXa37LTB1 BQGS3j1PmpfFTLHfxdYf8+PfQ0K8e51igwwP03yiQpDKXgdTaBnsOICkhBMmWOL7yyc+YgfQ7kE BLroIhUsXhguNac9knf/PVfgrJemNCyMDuV0Cj53lnnaY3iZc5iBXw= X-Received: by 2002:a05:6000:402b:b0:382:5206:8b5d with SMTP id ffacd0b85a97d-385c6edd264mr2065624f8f.42.1732710920397; Wed, 27 Nov 2024 04:35:20 -0800 (PST) X-Google-Smtp-Source: AGHT+IGmegXqrkpnf3VBQnGgqRRCDgeamo6WA/KeaxdjFJPWP+1V4+ase8des5pyglnNHSkgSe9JUQ== X-Received: by 2002:a05:6000:402b:b0:382:5206:8b5d with SMTP id ffacd0b85a97d-385c6edd264mr2065604f8f.42.1732710919961; Wed, 27 Nov 2024 04:35:19 -0800 (PST) Received: from [192.168.188.25] ([80.243.52.133]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3825fbedf35sm16167074f8f.99.2024.11.27.04.35.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 27 Nov 2024 04:35:19 -0800 (PST) Message-ID: Date: Wed, 27 Nov 2024 13:35:18 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] treewide: Introduce 'local mode' for disconnected setups To: Stefano Brivio References: <20241126055429.1610735-1-sbrivio@redhat.com> <20241127052715.751d536d@elisabeth> From: Paul Holzinger In-Reply-To: <20241127052715.751d536d@elisabeth> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: FEhGrHIbg47aAgKFg38X37xbU38pWenVJF6feS6MWnk_1732710920 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Message-ID-Hash: Z5W6BFGMVN5QDM3JM22MM5ZMN36DTQEH X-Message-ID-Hash: Z5W6BFGMVN5QDM3JM22MM5ZMN36DTQEH X-MailFrom: pholzing@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, David Gibson 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 27/11/2024 05:27, Stefano Brivio wrote: > 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. 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