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=CRcGBIP8; 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 ESMTPS id CF2755A065C for ; Mon, 17 Mar 2025 12:50:41 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1742212240; 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=nnkKk+VqxBbZTdcn3z1z4yyYScSXILE1k9AWGUx/qE4=; b=CRcGBIP8TVbbmI5BDStCxwd1gx4Zc9HXmjZgYMZXUbYozRGf2b+xAaMuYzNfcdQ2tAZlWO QSox6yEGHJm8ZIiJc4EzrNXVlYhJAnixPdbZaJVcENmllOjHC5P1wjLiP1/+zRc4HIwrOb x+Colu03ast3BTun1cU2i02Q/3OyBTk= 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-386-vLrf74URPAOXrh6RCZQNhg-1; Mon, 17 Mar 2025 07:50:39 -0400 X-MC-Unique: vLrf74URPAOXrh6RCZQNhg-1 X-Mimecast-MFC-AGG-ID: vLrf74URPAOXrh6RCZQNhg_1742212238 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-43d0a037f97so13985825e9.2 for ; Mon, 17 Mar 2025 04:50:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1742212238; x=1742817038; 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=nnkKk+VqxBbZTdcn3z1z4yyYScSXILE1k9AWGUx/qE4=; b=HvhdBdl+NlEarozWX0GhUlOG1lpYPzBcgWen/cXiUdqhMMx4KkRmS+Ne7YNzEVHuDs ZiFG51QVxnoHotoSHkBG6K40grhpEVI/es3sL74D/RqUYkLUkuYrWymy7Nb2uv5Qe/6U vkuNLixj3KotudP6pjojZ2o1KUSYmrEB57r7KfCANMqavB3GxcBhTEZRe88piCB8OBZH UUycppusmI+8tLW/9g7mwflPO0U/gwkaz2oHfQkSyToaw6sgf36WMNzIlQ90zanIoxXO zmF/g+JB9WO1UfPzgT/4tf0eDnvbRogARWpMNx4CaQqAUCic/vl38Y5z6fSjcuLr5Zuz pLmA== X-Gm-Message-State: AOJu0YweiwWYp3YLEMrsUt+Kd5LzfWms/NWz0/yBGbChzdR/tatdV095 xj2QYDK70eSYWKUjRPWUUKAF06P0xrRCBreTii/O/2D1kqjQA99zHavje0Bw8CLrelT2klzVDH+ wt8kpkznCCvP50kw3/VRZxDXJu8VZVweJeSjq4ZGv1ZYpC8Slin0k723DhVq/ X-Gm-Gg: ASbGnctNlbI+jItYOIJ/7fGg29c0eDO9/NEaFw0VEBsDlCYi6lVC2HEA9/YIsnRLIWn NoxVziWo9KoNx+mD5kDPjvGrGVIUNho0SwZEe8UfVR5W1Oh4OaMw/7FfxSzhP3iLnixF7ZRbKXv Zcn/eCFdyh7K93/ql9PqiifOzC9cz5co+xMQ4i49gF+w3hHDFf5MkiV5aRXU50kKOhoYHqQzIqS WE7f0sTKOEtrXFNWQjVESQFOQmIWjVLA1wO7M7NVPDU1yVPeS8xKN+z3w3wEmYAYcJ9ydGEGDTh xJZB4LPjASE8tbtDSsUW X-Received: by 2002:a05:600c:3c8b:b0:43c:f332:703a with SMTP id 5b1f17b1804b1-43d1ed22601mr122835365e9.31.1742212237910; Mon, 17 Mar 2025 04:50:37 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHHgTUwX2cPZCoqcbxfisYiacpELuqEuczg0eoBY6ox+nQw7dg6HraaCZstPQ/FXxpYxJoz9A== X-Received: by 2002:a05:600c:3c8b:b0:43c:f332:703a with SMTP id 5b1f17b1804b1-43d1ed22601mr122835125e9.31.1742212237436; Mon, 17 Mar 2025 04:50:37 -0700 (PDT) Received: from [192.168.188.25] ([80.243.52.134]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43d1fe659b3sm101404935e9.34.2025.03.17.04.50.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Mar 2025 04:50:37 -0700 (PDT) Message-ID: Date: Mon, 17 Mar 2025 12:50:36 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] conf: Unify several paths in conf_ports() To: Stefano Brivio , David Gibson References: <20250312034359.2776948-1-david@gibson.dropbear.id.au> <20250315005028.294ca8d3@elisabeth> From: Paul Holzinger In-Reply-To: <20250315005028.294ca8d3@elisabeth> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: r-2PLGBpRuDyF_f-d_l0a9F9MkZsQShgJyQ3zczt0Ds_1742212238 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: 4S36NLCWUUBGLO363TLXTCU5Z4PYAQTD X-Message-ID-Hash: 4S36NLCWUUBGLO363TLXTCU5Z4PYAQTD 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 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 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? > > 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. 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]. 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 > > In any case, given that it works by mistake now and it's undocumented, > let me go ahead and apply this for the moment. We can add the > "functionality" back later if it makes sense. > -- Paul Holzinger