From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=fail (p=quarantine dis=none) header.from=redhat.com 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 E27AD5A0265 for ; Wed, 28 Oct 2026 23:38:19 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1793227098; 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=gHrKEnVuyqJaLwl8uAsPVuoEzFZHfOxYWf4Pniv5czI=; b=CUESzlsECaAVkQRd6iqsQEt0U7sAEuxVCiiUPBKf+KMuhF83kZNgdzYw96x/uKMQGr3Vra GHZd9frDAODNXWwB++N0TIjfJhfK5caz8QIckxMIP3tPxoxJLbFr+7yF5YBSiLqikKhqia TeFu6OzJP31OXgIvEGYNJhzovWX0eSA= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-7-bv-YY2BcOq6mKYcv1J-52Q-1; Wed, 28 Oct 2026 18:38:17 -0400 X-MC-Unique: bv-YY2BcOq6mKYcv1J-52Q-1 X-Mimecast-MFC-AGG-ID: bv-YY2BcOq6mKYcv1J-52Q_1793227096 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4a1f03f2e8aso13595955e9.1 for ; Wed, 28 Oct 2026 15:38:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1793227096; x=1793831896; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=gHrKEnVuyqJaLwl8uAsPVuoEzFZHfOxYWf4Pniv5czI=; b=PWdjPzmSQJOsfvyBAm16XNTX5lQHdlYSj4ufWzHYfj1EZ0yirGq5Hiu7zoa8yvWlx2 xDeArAyIvnw3CjApwI2N4w3OUOWl2hVzlb3scktfjT/2+hgPB2qsV8RdPeV7sdMcj+Yc XT1cWI/J44mI6DTM6DbnNxOz301n2ZrfLFwM9pAx95RqCdrB4BswBxEkAKwk09fB5xxW rmFORdsGd18QAGqT9FgMseeJOVXyhy7IbHwB0QPkAECfi0LJMcNmYm0GBNWZmUbHfjec 1jrdkvjtb3RVaQ/avfEsDFTNzxpwVKfK9sF8wdOcHBWUmWXyxETQGYWFNwlx7Cb+nfnD mSYA== X-Gm-Message-State: AFq9FYK8oUcfNVvdT1bbaMg2i6CFKbqzLPIXur5hKUznr7i+0xlh0IA9 NAQlRBcjYp9491EARyPRgEYGm1/5+2bs7Stq0pfDtEMFR+ejxoSmCo3tDGKBUxHehp+v/jV10v+ QmxJAH5hNhk612C1TDoVY4iAm43Vph23Ugc6IHddi1SgnaOrvQrgzNvpKofZ5I/0bpX7ws5SEmg h93RXxEmeEuq9+eUHw/Kst0fB8JDIakwMBUlEZ X-Gm-Gg: AYBFou1qTGHpxXVNFzyj4axjdtFazviAUY5yQ0GF7mZv/U4kuZKuWjqf/AMb2ls+PmJ dzfdrQU+sSg0xI0meBrxadRQfEQ/0hY/auKrfLpgb1zFwImSW83YoCaC0BX6i+ACZxxp3KtqvsZ yhVA3GWdBP2ZXyT2e/OYkp5p+NnIsbilBi/fAZTZCtx4h0hS9851SUwdDNjM0fB1nJwbpgAK5l7 NpQppdOsVYs48hwCI1JQ4b+sJF5nNBdFxHM8YL94miAo5W9eiB1tKCEfZIiQUocbZYjUPdhZbL1 LZ1ZG22l7S7K2eoCYR6jPo62CjeLCDIuwub0VRoghNb7NVz0Z2xgD8kR8vyce5PRxyBgp6b4ul3 af4WZL3ogWcreICLOPVk7vKtPm/F332pLBdKN0PiXRgmZZi4+rpRA5Siwq3iv9GhkkOv4DYppLe FbPGIcrHP4 X-Received: by 2002:a05:600c:37c3:b0:4a2:1867:10e8 with SMTP id 5b1f17b1804b1-4a96701830cmr82439495e9.25.1793227095700; Wed, 28 Oct 2026 15:38:15 -0700 (PDT) X-Received: by 2002:a05:600c:37c3:b0:4a2:1867:10e8 with SMTP id 5b1f17b1804b1-4a96701830cmr82439045e9.25.1793227095039; Wed, 28 Oct 2026 15:38:15 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4a94e4be4b2sm48669245e9.2.2026.10.28.15.38.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Oct 2026 15:38:14 -0700 (PDT) From: Stefano Brivio To: Richard Lawrence Subject: Re: [PATCH v3] pasta: Add --pass-fds option to skip closing file descriptors Message-ID: <20261028233813.1bf26f42@elisabeth> In-Reply-To: <20261026170345.634781-1-rlawrence@tamu.edu> References: <20261026170345.634781-1-rlawrence@tamu.edu> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 28 Oct 2026 23:38:14 +0100 (CET) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 6HXX-xRCE_z5pJ3URCg7w_H8SDAvQWL0_t2Z-ZUoJw8_1793227096 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: F3IYGAUBJSCHLMHS7UBNV3MEXTXLZ6HR X-Message-ID-Hash: F3IYGAUBJSCHLMHS7UBNV3MEXTXLZ6HR 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 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, 26 Oct 2026 12:03:45 -0500 Richard Lawrence wrote: > When pasta spawns a command (`pasta [options] -- [command]`), all open file > descriptors inherited from the parent process except standard streams (0-2) > and the tap descriptor (`--fd`) are normally closed by `isolate_fds()`. > > However, when pasta is used for testing, development, or launching tools like > Bubblewrap (`bwrap`), the spawned command may require file descriptors > passed directly from the parent process. In this case, closing inherited file > descriptors breaks expected behavior. > > Add a boolean `--pass-fds` flag for pasta mode that skips the `close_range()` > cleanup step in `isolate_fds()`, leaving all open file descriptors intact > for the child command. > > Summary of changes: > > - Update `conf_tap_fd()` to `conf_fds()`, allowing early parsing of both > `-F`/`--fd` and `--pass-fds` before main option processing. > - Update `isolate_fds()` to check `c->pasta_pass_fds`: if set, skip calling > `close_range()` while still relocating the tap fd out of standard stream > range (0-2) if needed. > - Enforce validation in `conf()` ensuring `--pass-fds` is restricted to pasta > mode when spawning a command (incompatible with attaching to an > existing netns). > - Update man page and usage help output. > > Link: https://bugs.passt.top/show_bug.cgi?id=204 > Signed-off-by: Richard Lawrence > --- > conf.c | 56 +++++++++++++++++++++++++--------------- > conf.h | 2 +- > isolation.c | 73 +++++++++++++++++++++++++++++++++-------------------- > isolation.h | 2 +- > passt.1 | 5 ++++ > passt.c | 2 +- > passt.h | 2 ++ > 7 files changed, 91 insertions(+), 51 deletions(-) > > diff --git a/conf.c b/conf.c > index 149a4c7..30edef9 100644 > --- a/conf.c > +++ b/conf.c > @@ -750,7 +750,8 @@ pasta_opts: > " Don't copy all addresses to namespace\n" > " --ns-mac-addr ADDR Set MAC address on tap interface\n" > " --no-splice Disable inbound socket splicing\n" > - " --splice-only Only enable loopback forwarding\n"); > + " --splice-only Only enable loopback forwarding\n" > + " --pass-fds Preserve open file descriptors\n"); > > passt_exit(status); > } > @@ -1137,35 +1138,41 @@ static void conf_sock_listen(const struct ctx *c) > } > > /** > - * conf_tap_fd() - Read tap fd as supplied by -F command line option > + * conf_fds() - Read fds as supplied by -F/--fd and --pass-fds options > + * @c: Execution context > * @argc: Argument count > * @argv: Command line options > * > - * Return: fd number from --fd option, or -1 if not supplied > + * Should: > + * - Set the fd_tap and pasta_pass_fds values in the execution context Those "Should:" comments are something specific in isolation.c where we have clearly defined subsequent steps. Here it doesn't make a lot of sense, the comment as you changed it above ("Read fds...") already makes this very clear. More comments means more to read, so they should be worth it. > */ > -int conf_tap_fd(int argc, char **argv) > +void conf_fds(struct ctx *c, int argc, char **argv) > { > - const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, > - { 0 }, }; > - const char *fdarg = NULL, *p; > - unsigned long val; > + const struct option opt[] = { > + { "fd", required_argument, NULL, 'F' }, > + { "pass-fds", no_argument, NULL, 2 }, Harmless, but still mildly confusing: why 2 instead of 1? > + { 0 }, > + }; > int name; > > + c->fd_tap = -1; > + c->pasta_pass_fds = false; > optind = 0; > do { > - name = getopt_long(argc, argv, "-:F:", optfd, NULL); > - if (name == 'F') > - fdarg = optarg; > - } while (name != -1); > - > - if (!fdarg) > - return -1; > + name = getopt_long(argc, argv, "-:F:", opt, NULL); > + if (name == 'F') { > + const char *p = optarg; > + unsigned long val; > > - p = fdarg; > - if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || val > INT_MAX) > - die("Invalid --fd: %s", fdarg); > + if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || > + val > INT_MAX) > + die("Invalid --fd: %s", optarg); > > - return val; > + c->fd_tap = (int)val; /* May be overwritten in isolate_fd() */ Not just in isolate_fds(). Same consideration here: this comment doesn't really add much, so I'd rather drop it. > + } else if (name == 2) { > + c->pasta_pass_fds = true; > + } > + } while (name != -1); > } > > /** > @@ -1317,6 +1324,7 @@ void conf(struct ctx *c, int argc, char **argv) > {"conf-path", required_argument, NULL, 'c' }, > {"chroot-fallback", no_argument, NULL, 32 }, > {"no-pidns", no_argument, NULL, 33 }, > + {"pass-fds", required_argument, NULL, 34 }, > { 0 }, > }; > const char *optstring = "+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:"; > @@ -1567,6 +1575,11 @@ void conf(struct ctx *c, int argc, char **argv) > > c->no_pidns = true; > break; > + case 34: > + /* --pass-fds was parsed early */ > + if (c->mode != MODE_PASTA) > + die("--pass-fds is for pasta mode only"); > + break; > case 'd': > c->debug = 1; > c->quiet = 0; > @@ -1603,7 +1616,7 @@ void conf(struct ctx *c, int argc, char **argv) > c->fd_control_listen = c->fd_control = -1; > break; > case 'F': > - /* --fd was parsed early and c->fd_tap set in main() */ > + /* --fd was parsed early */ > c->one_off = true; > *c->sock_path = 0; > break; > @@ -1929,6 +1942,9 @@ void conf(struct ctx *c, int argc, char **argv) > else if (optind != argc) > die("Extra non-option argument: %s", argv[optind]); > > + if (c->pasta_pass_fds && *netns) > + die("--pass-fds can only be used when spawning a command"); > + > if (c->no_pidns && *netns) > die("--no-pidns is incompatible with PID or --netns"); > > diff --git a/conf.h b/conf.h > index 19bf9bc..654fd16 100644 > --- a/conf.h > +++ b/conf.h > @@ -7,7 +7,7 @@ > #define CONF_H > > enum passt_modes conf_mode(int argc, char *argv[]); > -int conf_tap_fd(int argc, char **argv); > +void conf_fds(struct ctx *c, int argc, char **argv); > void conf(struct ctx *c, int argc, char **argv); > void conf_listen_handler(struct ctx *c, uint32_t events); > void conf_handler(struct ctx *c, uint32_t events); > diff --git a/isolation.c b/isolation.c > index a00475f..766c297 100644 > --- a/isolation.c > +++ b/isolation.c > @@ -250,46 +250,63 @@ void isolate_initial(void) > } > > /* > - * isolate_fds() - Close leaked files, but not --fd, stdin, stdout, stderr > + * isolate_fds() - Close leaked files if applicable > + * @c: Execution context > * @argc: Argument count > - * @argv: Command line options, as we need to skip any file given via --fd > + * @argv: Command line options > * > * Should: > - * - close all open files except for standard streams and the one from --fd > + * - close all open files except for standard streams and the one from --fd, > + unless --pass-fds is used, in which case all fds are preserved > * - move the --fd descriptor out of the range 0-2 > - * > - * Return: new fd number for descriptor from --fd, or -1 if not specified > + * - update the execution context > */ > -int isolate_fds(int argc, char **argv) > +void isolate_fds(struct ctx *c, int argc, char **argv) > { > - int fd, close_from = STDERR_FILENO + 1; > + conf_fds(c, argc, argv); > > - fd = conf_tap_fd(argc, argv); > + if (c->fd_tap >= 0 && c->fd_tap < STDERR_FILENO + 1) { STDERR_FILENO + 1 used to be 'close_from' but now you're dropping that variable for some reason I didn't understand. However, this changes the behaviour (in a way that's rather unexpected as it's not mentioned in the commit message): instead of moving the --fd descriptor unconditionally, it's only done if the descriptor number is less than 3. Is this additional condition needed for Bubblewrap usage? If it is: - you could keep the simplification made possible by commit fd5b0807f5d0 ("isolation: Move --fd descriptor to a number of our choosing") in the general case, that is, move it unconditionally to 3, so that close_range() is simple - on --pass-fds, you don't need to call close_range(), so that simplification is not needed, and there's no need to move the file descriptor to 3 if it's already => 3 So this could simply be: /* Move the passed fd to a more convenient location (3): * - on --pass-fds, only if it might conflict with standard streams * - otherwise, unconditionally, to keep close_range() below simple */ if (c->fd_tap >= 0 && (!c->pass_fds || c->fd_tap < close_from)) { ...and all the rest would remain as is (except that 'fd' becomes 'c->fd_tap'), until: > + /* Move the tap fd to a safer location */ > + int new_fd = fcntl(c->fd_tap, F_DUPFD, STDERR_FILENO + 1); > > - if (fd >= 0) { > - /* Move the passed fd to a more convenient location */ > - if (fd != close_from && > - (dup2(fd, close_from) != close_from || > - close(fd))) > - die_perror("Could not move --fd descriptor"); > - fd = close_from++; > + if (new_fd < 0) > + die_perror("Could not relocate --fd descriptor"); > + > + close(c->fd_tap); > + c->fd_tap = new_fd; > } ...here, where we could simply return early on c->pass_fds (can we drop 'pasta' from there? I think it's redundant). Right? > > - if (close_range(close_from, ~0U, CLOSE_RANGE_UNSHARE)) { > - if (errno == ENOSYS || errno == EINVAL) { > - /* This probably means close_range() or the > - * CLOSE_RANGE_UNSHARE flag is not supported by the > - * kernel. Not much we can do here except carry on and > - * hope for the best. > - */ > - warn( > -"Can't use close_range() to ensure no files leaked by parent"); > - } else { > - die_perror("Failed to close files leaked by parent"); > + /* Close leaked file descriptors unless --pass-fds is used */ > + if (!c->pasta_pass_fds) { > + int rc = 0; > + /* Keep standard streams and the tap fd (if given) */ > + int keep = c->fd_tap >= STDERR_FILENO ? c->fd_tap : STDERR_FILENO; > + > + if (keep > STDERR_FILENO + 1) { > + /* Tap fd was relocated to above some other leaked fd(s)*/ > + /* Close fds between stderr and the tap fd */ > + if (close_range(STDERR_FILENO + 1, keep - 1, CLOSE_RANGE_UNSHARE)) > + rc = -1; > } > - } > > - return fd; > + /* Close all other fds */ > + if (close_range(keep + 1, ~0U, CLOSE_RANGE_UNSHARE)) > + rc = -1; > + > + if (rc) { > + if (errno == ENOSYS || errno == EINVAL) { > + /* This probably means close_range() or the > + * CLOSE_RANGE_UNSHARE flag is not supported by the > + * kernel. Not much we can do here except carry on and > + * hope for the best. > + */ > + warn( > + "Can't use close_range() to ensure no files leaked by parent"); > + } else { > + die_perror("Failed to close files leaked by parent"); > + } > + } > + } > } > > /** > diff --git a/isolation.h b/isolation.h > index ec47038..fe8d7d5 100644 > --- a/isolation.h > +++ b/isolation.h > @@ -11,7 +11,7 @@ > #include > > void isolate_initial(void); > -int isolate_fds(int argc, char **argv); > +void isolate_fds(struct ctx *c, int argc, char **argv); > void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns, > const char *userns); > int isolate_prefork(const struct ctx *c); > diff --git a/passt.1 b/passt.1 > index 0b780fe..56e241f 100644 > --- a/passt.1 > +++ b/passt.1 > @@ -766,6 +766,11 @@ of local traffic in pasta\fR in the \fBNOTES\fR for more details. > Do not create a tap device in the namespace. In this mode, \fIpasta\fR only > forwards loopback traffic between namespaces. > > +.TP > +.BR \-\-pass\-fds > +Preserve all open file descriptors and pass them to the spawned command. > +This option may only be used when \fBpasta\fR spawns a command. > + > .SH EXAMPLES > > .SS \fBpasta > diff --git a/passt.c b/passt.c > index 5054551..fa7afc5 100644 > --- a/passt.c > +++ b/passt.c > @@ -345,7 +345,7 @@ int main(int argc, char **argv) > arch_avx2_exec(argv); > > isolate_initial(); > - c->fd_tap = isolate_fds(argc, argv); > + isolate_fds(c, argc, argv); > > if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0) > die_perror("Failed to open /dev/null"); > diff --git a/passt.h b/passt.h > index afd8e9f..b065d41 100644 > --- a/passt.h > +++ b/passt.h > @@ -196,6 +196,7 @@ struct ip6_ctx { > * @pasta_ifi: Index of namespace interface for pasta > * @pasta_conf_ns: Configure namespace after creating it > * @no_pidns: Don't create a new PID namespace for spawned command > + * @pasta_pass_fds: Do not close leaked file descriptors As I was suggesting on v2, quoting: --- Does it really need to have 'pasta' in the name? Sure, it's pasta-only, but this is already documented in man page, usage, and conf(), so we could probably save some typing by calling it 'pass_fds'. --- > * @fwd: Forwarding tables > * @fwd_pending: Pending forward tables > * @no_tcp: Disable TCP operation > @@ -280,6 +281,7 @@ struct ctx { > unsigned int pasta_ifi; > int pasta_conf_ns; > bool no_pidns; > + bool pasta_pass_fds; > > struct fwd_table *fwd[PIF_NUM_TYPES]; > struct fwd_table *fwd_pending[PIF_NUM_TYPES]; -- Stefano