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=KeUdJ429; 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 BD4805A0271 for ; Tue, 18 Mar 2025 17:54:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1742316883; 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=StIg7JDgK2bvsTaYLNHAQZXqPb8Nf+yKcpVQB+RMEkQ=; b=KeUdJ4294mm6z/RAMZz3gYtEQaFSzv9MZrI/2n/7czMyc3c2b0ERS8pFmnTwWuvSeBwta2 0WpJUS3+uv6tyP/vOaHMYnpXI97vpo3J8q84Ysdqf6PXyNoXPdyOThcnklfXMgJmoDdK5c 8tzvCBK9F+Yhqe+wh6YoMCQ5ino/nq8= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-135-cBHd8-IPMr2_EX9uYCeUWg-1; Tue, 18 Mar 2025 12:54:42 -0400 X-MC-Unique: cBHd8-IPMr2_EX9uYCeUWg-1 X-Mimecast-MFC-AGG-ID: cBHd8-IPMr2_EX9uYCeUWg_1742316881 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-391345e3aa3so3513079f8f.0 for ; Tue, 18 Mar 2025 09:54:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1742316880; x=1742921680; 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=StIg7JDgK2bvsTaYLNHAQZXqPb8Nf+yKcpVQB+RMEkQ=; b=DMPmt7Y5qOYpkX6EXEcwM/nAPI/4soDnlEdIzKKrv7YuEzB12RGRgjO0foxjaWMxA1 jVeWdL4jR4u2Ga18FdkwGLscdAadAuZPbD4VXzNUJn5aQccV3cjgci/IBV2bZYExYgof enI8X/3LzihsWSmduV/eiVJ46wh/WMDVN5NGalgAR4TgbYTehArECEE/zmB5auxfqrsv ELVUJG1xj4rjSLwtqExYwFaq2sKJ89oOz7ru/0Kj82U3nrIeyIczp+Jy41N1o1v8Ykuq Tleq3jqeUgam093IZl8EWlJ1daFHjRGNE0OVXzu7aOIxVZvRo66h6jmJLQOo6GzONxk7 gsYg== X-Forwarded-Encrypted: i=1; AJvYcCXqWI+noOdKyCpH9jx3gWAaT54+7pnnP3Yq3IP+E3stepDcVQmunZNpWurQQdKFLwta0iPKUbsp1TA=@passt.top X-Gm-Message-State: AOJu0YzcGAcAAPfuuc4S9/4T2WW8wB78JkL6LqAY/nWeDP2BKXtawkE+ ehJEMGOktS8VW9bfV1zaj2hkUKY4sQfeOW/D5zg5JwbENK6ls3f6u/hM9pO9wRC7Al1UcELuqwB u70LlKI26TtXSELfE3yPZ0cSccxpRqc7RY/t/GSYSUZAu81wdrQSssYBvnA== X-Gm-Gg: ASbGncvLb7/kZC5S4cnzX4EI5XaY0XFCrOwSG8ZcK6o9GS7vULHhuvuXX+RxTv5WSH+ aRiBMlH1U1SanTKg9HavCd9Oo3TBTIr0R+GoinY1iGrCqLskxFghLEvtjd2VEh6vk4Kd6AUlY8c Q9RO9CPtsuRzfRyExhr60eS+JFCC8biTs7v4ZjNKNcZcsBBJa+7IcyfCrjo0Vt7OvybaMvJhrLT cA/M3VZu3FDQxWfkvGCGF0nvf0PcmK5gV7qEtRVVuy5Avee6nZL6uR8dTTavrdvhbiI2XnGFG87 bVSWL13YZCOSAxOAg2dk0JYXkGmj7xnyxZOBgudIf/Uv X-Received: by 2002:adf:9b97:0:b0:399:6d53:68d9 with SMTP id ffacd0b85a97d-3996d536aabmr3569996f8f.38.1742316880591; Tue, 18 Mar 2025 09:54:40 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHf9MDFHdVhBFzVZkxkbFwYVz1CvLjhDbPy8raI0QUAQvmbvbkpyyJJI6Vw7K0uluKHvZsAyg== X-Received: by 2002:adf:9b97:0:b0:399:6d53:68d9 with SMTP id ffacd0b85a97d-3996d536aabmr3569969f8f.38.1742316880113; Tue, 18 Mar 2025 09:54:40 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-395c83b6e87sm19225729f8f.32.2025.03.18.09.54.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Mar 2025 09:54:39 -0700 (PDT) Date: Tue, 18 Mar 2025 17:54:37 +0100 From: Stefano Brivio To: Paul Holzinger Subject: Re: [PATCH v2] conf: Unify several paths in conf_ports() Message-ID: <20250318175437.1d62bf37@elisabeth> In-Reply-To: References: <20250312034359.2776948-1-david@gibson.dropbear.id.au> <20250315005028.294ca8d3@elisabeth> 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: e5P3zdYexr5oRh_O3lRFBG77rgBYUsgtZpK515Tn7cU_1742316881 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: CNJAO4QHOWCBDNWNX2FP4JDJDITO36EV X-Message-ID-Hash: CNJAO4QHOWCBDNWNX2FP4JDJDITO36EV 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 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 ?). -- Stefano