From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine 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=XeHZuScQ; 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 ESMTPS id 0CAF75A0271 for ; Tue, 18 Mar 2025 18:46:09 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1742319968; 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=RzJdDf9zluWUNvYtXLkc3tYjVED4N/Nb/H4iE0Exx2A=; b=XeHZuScQfkigkvlQqZ1dxkwlJ4/lrIuD0Pu3Z0uM7Pd6coSgD/DmagghuteBbVkrsyDBPc GWo9usDVk1bGDfOPNE2h+XbEwoEM6RjZ/hLsCas/ilHxG1EJawZRz3tcumeJJFQ0fPSH9i h8USOyPWROQnnYfrWOjEj0qmv9yhFTY= 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-629-1pAce3PmOkWUlGxW7o5xhg-1; Tue, 18 Mar 2025 13:46:07 -0400 X-MC-Unique: 1pAce3PmOkWUlGxW7o5xhg-1 X-Mimecast-MFC-AGG-ID: 1pAce3PmOkWUlGxW7o5xhg_1742319966 Received: by mail-wr1-f70.google.com with SMTP id ffacd0b85a97d-3912539665cso3422555f8f.1 for ; Tue, 18 Mar 2025 10:46:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1742319966; x=1742924766; 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=RzJdDf9zluWUNvYtXLkc3tYjVED4N/Nb/H4iE0Exx2A=; b=cQV1guGC3W3aZ8ipSWQUc7NKlfy2cXA6Z4SydVyAkOGwpdbKh2ehzdGJQ9gJKTbjxg iqmyfj2EAb5InGKyRSofx5IlK/U/nRK6l1oRqcfcyLvaBPHoTCR7pw31LWlsfiJkBJWV LNp/4yURfD20MUai6BT2fR0RAqUe0+7pgbZY0hlxfoVlrXo64d3oUqDgJO9oXI/ggm7L PH1+Stgab0SD3BHcO836YK+f7WWzXuEOUotO2qsuMDGTUYBVGMejeztTFBJniBSxWshn 0nlUT876WDM7F52E/fRfp/7hlXGwEMzVyQqLGuna8eDrmAYqOKRmZl4uDXI4bUlZV3KA PmvQ== X-Forwarded-Encrypted: i=1; AJvYcCWpsO4z6h5VY37/nX3Tx9ddKiZNiRE73R4Hgpm2aL8/opLEkmX2CZgd1Fbplu6eGj/E01LTKRZslA4=@passt.top X-Gm-Message-State: AOJu0YyyAFfTi0rzxbjNFKmbEwESsXdmvXmrARu9V/CGykAH3cSnD4Wn IWz52GDR9GNpChOCzKIEQgSy+sqKBK89MF9/JdsjFyVqvKFb6VIGaawQOsW6qbPxwVRbApAxmSF A98g5qb2Q0NAecLZCoKtTmwlw02tipeT3ss2jLMEsy85h2OnAGZm8yTIsSQ== X-Gm-Gg: ASbGnctGstZ9TKmJVUL+FPZxO3dR8PQGOiBDcbi28LB2tBGFX31z5Ic+lRnA8YjgwCt e80vKiobqAidpwN36HLG+DmrqmXnRdpaq4UxuRc+8/jUQByahxmd3yLooOgDkOUXuRPqVbUC8uY Co/SgokKzRC0Aw4+gal4KzLkpfRiA6RXpANwsL6FJvfWgAn2aAxrIYfpCaPV7ziWe7G19LaNskC dtTxqfVBFW2Z2P44132Y+xq9bTmDsGJK3VBEM7o4vvM6htmWvdWCLzsCRQKb8yKGNPzGuqyDD1N vltXO0yF2JcR7jvR4296YnBU6tcCSq7XZEf8dUdtVeXG X-Received: by 2002:a5d:5f93:0:b0:390:ec6e:43ea with SMTP id ffacd0b85a97d-3996bb51f57mr4011958f8f.15.1742319965999; Tue, 18 Mar 2025 10:46:05 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHH8XGN/lqcQ3c8dBDTxNmIFyIHDS9dlSuoIzvtrtoxiEnf3W4Vf7yVoxyLPSjljzoP8adYcw== X-Received: by 2002:a5d:5f93:0:b0:390:ec6e:43ea with SMTP id ffacd0b85a97d-3996bb51f57mr4011933f8f.15.1742319965448; Tue, 18 Mar 2025 10:46:05 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43d1fe60965sm141111905e9.25.2025.03.18.10.46.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Mar 2025 10:46:05 -0700 (PDT) Date: Tue, 18 Mar 2025 18:46:03 +0100 From: Stefano Brivio To: Paul Holzinger Subject: Re: [PATCH v2] conf: Unify several paths in conf_ports() Message-ID: <20250318184603.3b015c08@elisabeth> In-Reply-To: <9c80668b-acd9-4cb9-963e-f7411701c8da@redhat.com> References: <20250312034359.2776948-1-david@gibson.dropbear.id.au> <20250315005028.294ca8d3@elisabeth> <20250318175437.1d62bf37@elisabeth> <9c80668b-acd9-4cb9-963e-f7411701c8da@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: snIMrIW_JoS9kr8w4-Db3QI9yIQN0ZO0Mt2bjtAGGuA_1742319966 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: RZJLBFBHFKVELS6UY7NN34U2SMC2WY35 X-Message-ID-Hash: RZJLBFBHFKVELS6UY7NN34U2SMC2WY35 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: David Gibson , passt-dev@passt.top 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 Tue, 18 Mar 2025 18:15:27 +0100 Paul Holzinger wrote: > On 18/03/2025 17:54, Stefano Brivio wrote: > > On Mon, 17 Mar 2025 12:50:36 +0100 > > Paul Holzinger wrote: > > > >> On 15/03/2025 00:50, Stefano Brivio wrote: > >>> On Wed, 12 Mar 2025 14:43:59 +1100 > >>> David Gibson wrote: > >>> > >>>> In conf_ports() we have three different paths which actually do the setup > >>>> of an individual forwarded port: one for the "all" case, one for the > >>>> exclusions only case and one for the range of ports with possible > >>>> exclusions case. > >>>> > >>>> We can unify those cases using a new helper which handles a single range > >>>> of ports, with a bitmap of exclusions. Although this is slightly longer > >>>> (largely due to the new helpers function comment), it reduces duplicated > >>>> logic. It will also make future improvements to the tracking of port > >>>> forwards easier. > >>>> > >>>> The new conf_ports_range_except() function has a pretty prodigious > >>>> parameter list, but I still think it's an overall improvement in conceptual > >>>> complexity. > >>>> > >>>> Signed-off-by: David Gibson > >>>> --- > >>>> conf.c | 173 ++++++++++++++++++++++++++++++--------------------------- > >>>> 1 file changed, 90 insertions(+), 83 deletions(-) > >>>> > >>>> v2: > >>>> * Commit message updated slightly, but otherwise unmodified. > >>>> > >>>> > >>>> diff --git a/conf.c b/conf.c > >>>> index 065e7201..4e0099ba 100644 > >>>> --- a/conf.c > >>>> +++ b/conf.c > >>>> @@ -123,6 +123,75 @@ static int parse_port_range(const char *s, char **endptr, > >>>> return 0; > >>>> } > >>>> > >>>> +/** > >>>> + * conf_ports_range_except() - Set up forwarding for a range of ports minus a > >>>> + * bitmap of exclusions > >>>> + * @c: Execution context > >>>> + * @optname: Short option name, t, T, u, or U > >>>> + * @optarg: Option argument (port specification) > >>>> + * @fwd: Pointer to @fwd_ports to be updated > >>>> + * @addr: Listening address > >>>> + * @ifname: Listening interface > >>>> + * @first: First port to forward > >>>> + * @last: Last port to forward > >>>> + * @exclude: Bitmap of ports to exclude > >>>> + * @to: Port to translate @first to when forwarding > >>>> + * @weak: Ignore errors, as long as at least one port is mapped > >>>> + */ > >>>> +static void conf_ports_range_except(const struct ctx *c, char optname, > >>>> + const char *optarg, struct fwd_ports *fwd, > >>>> + const union inany_addr *addr, > >>>> + const char *ifname, > >>>> + uint16_t first, uint16_t last, > >>>> + const uint8_t *exclude, uint16_t to, > >>>> + bool weak) > >>>> +{ > >>>> + bool bound_one = false; > >>>> + unsigned i; > >>>> + int ret; > >>>> + > >>>> + if (first == 0) { > >>>> + die("Can't forward port 0 for option '-%c %s'", > >>>> + optname, optarg); > >>>> + } > >>> This introduces two subtle functional changes that are a bit unexpected > >>> given the commit message. Before: > >>> > >>> $ ./pasta -t 0 > >>> $ > >>> > >>> $ ./pasta -t 0-1025 > >>> Failed to bind port 1 (Permission denied) for option '-t 0-1025', exiting > >>> > >>> After: > >>> > >>> $ ./pasta -t 0 > >>> Can't forward port 0 for option '-t 0' > >>> > >>> $ ./pasta -t 0-1025 > >>> Can't forward port 0 for option '-t 0-1025' > >>> > >>> ...anyway, I doubt anybody would use -t 0 on purpose (to get a port > >>> automatically assigned), and while it probably works for TCP (check > >>> bound ports after starting pasta, use the assigned one), it wouldn't > >>> necessarily work as expected for UDP if the application relies on our > >>> flow tracking. > >> Why would this not work for UDP? bind() wise you can still bind 0 fine > >> and get a free port assigned? > > The bind() part itself would work, but with the current implementation > > we wouldn't be able to track flows corresponding to this specific port > > forwarding, so I expect that the "return" (outbound) traffic won't > > work. > > > > It's a matter of implementation (or lack thereof), we could get it to > > work with a getsockname() after bind(). > > > > Before this change, it happened to work *by mistake* for TCP, not for > > UDP. With this change, it doesn't work for TCP. We can add it back with > > a proper syntax (-t ...any?), as David mentioned. > > > >>> For TCP, actually, -t 0 might be useful, see e.g. random_free_port() in > >>> Podman tests (/test/system/helpers.network.bash). We should print the > >>> port number that was bound, though, and document the feature. > >>> > >>> More than that: that could actually be the only race-free possibility > >>> of picking and forwarding a port where the number doesn't matter. > >> Yes it could be useful for podman but then it should also work with udp. > > We can get it to work if needed. We would need, I guess: > > > > - that getsockname() for UDP, whatever is missing for the UDP case > > > > - a new configuration sub-option > > > > - documentation > > > >> I am less worried about the tests, this issue is in podman proper as you > >> can do "-p 80", then podman assigns a free host port. Except that this > >> is super broken in podman because we do this once when we create the > >> container so this is totally racy and non conflict free[1]. > > Ouch, I wasn't aware of that. For pasta it should be relatively easy to > > do that in a race-free way, because the kernel guarantees that. > > > >> The thing of > >> course is for podman we have to deal with like 4 other port forwarder > >> implementations that we would need to support. As such I don't see us > >> ever finding time to properly fix it unless it magically gets a ton of > >> priority. So if pasta does not support for it I have no problems with > >> that, however maybe one day we like to reconsider. > >> > >> [1] > >> https://github.com/containers/podman/issues/10205#issuecomment-1010055023 > > I would wait for David's feedback on this, but to me it looks like a > > small-ish thing we can add without much thinking and planning. > > > > I'm not sure you can close that issue if we implement it in pasta as > > long as forwarding is done like it's done now for custom networks, but > > the issue would look less serious I guess. > > > > I don't know about the Podman side of it, but probably that would look > > trivial to you (-t any:80 maybe? or -t :80 ?). > > I guess something like -t 0:80 could also work. That would allow me to > store unassigned ports as 0 and the the convert to pasta cli code would > not need any special handling for this case. > > Overall it will be more complicated in Podman which is why I never > bothered to take this on. The issue is that commands like podman inspect > or podman port need to know the actual ports that were assigned. Ah, right, I forgot about that. > So it > would need some form of interface from pasta that prints out which host > port it uses for each namespace port. Then podman must need to gain > support to store two different set of ports, dynamic and static > (currently) so we can keep track of the ports pasta returned to us. That doesn't really sound trivial, I was underestimating this. > All stuff we can implement but until someone pushes hard for it I don't > think it will ever make it. I think there are better network things we > can spend our time on. ...so we should rather defer this I guess. Probably it will make sense to revisit this together with the new forwarding configuration interface once we have it implemented. -- Stefano