Re: [PATCH v2] pasta: Add --pass-fds option to skip closing file descriptors

Richard, thanks for the new version and for your patience! The logic looks good to me, but I think we can make the implementation a bit simpler. A few comments inline: On Thu, 22 Oct 2026 12:08:23 -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:

- Introduce `struct fd_opts` and 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 `opts.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 | 58 ++++++++++++++++++++++++++--------------- conf.h | 7 ++++- isolation.c | 74 +++++++++++++++++++++++++++++++++-------------------- isolation.h | 2 +- passt.1 | 5 ++++ passt.c | 2 +- passt.h | 2 ++ 7 files changed, 99 insertions(+), 51 deletions(-)

diff --git a/conf.c b/conf.c index 149a4c7..4479467 100644 --- a/conf.c +++ b/conf.c @@ -750,7 +750,9 @@ 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 and\n" + " pass them to the spawned command\n");

The usage message is already very long, and it's not meant as a comprehensive documentation, just a quick reference, so I guess we could simply say "Preserve open file descriptors" here, and save one line of output.

passt_exit(status); } @@ -1136,36 +1138,43 @@ static void conf_sock_listen(const struct ctx *c) die_perror("Couldn't add configuration socket to epoll"); }

+ + +

Unrelated blank lines.

/** - * 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 * @argc: Argument count * @argv: Command line options - * - * Return: fd number from --fd option, or -1 if not supplied + + * Return: fd_opts { fd_tap, pass_fds }

Returning a struct works, and it might even look elegant, but it's not something we do frequently in this codebase, or not at all, with the general reasoning being that structs eventually grow and, in non-trivial cases, we risk having copies on return that might grow well beyond what was intended. On top of that, if you want to signal failure at some point, you would need a small rework. It's absolutely fine to do this here, but I would still have a slight preference for consistency (make it a void if it never fails, and pass the two arguments as pointers). However, I don't think we need this at all. You are already passing a struct ctx to the caller now, isolate_fds(). It contains (almost) the whole configuration. This function reads configuration. It could very naturally store it there and you're done with it: just set c->fd_tap and c->pasta_pass_fds here (with a minor caveat around c->fd_tap, see below).

*/ -int conf_tap_fd(int argc, char **argv) +struct fd_opts conf_fds(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' },

You're leaving a bit of space here because it's done in conf(), so that arguments can be align and this becomes a more readable form of table, but:

+ { "pass-fds", no_argument, NULL, 34 },

...then it's not actually aligned here. And 34 just happens to be the matching index in getopt_long() in conf(), but it's kind of confusing to have it here: for the purposes of this function, it's the first option index without short-hand, so it should be 1. That is, I think this should be: { "fd", required_argument, NULL, 'F' }, { "pass-fds", no_argument, NULL, 1 },

+ { 0 }, + }; + struct fd_opts opts = { .fd_tap = -1, .pass_fds = false }; int name;

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; + opts.fd_tap = (int)val; + } else if (name == 34) {

(don't forget to change this to '1' if you go for the version I suggested above)

+ opts.pass_fds = true; + } + } while (name != -1);

Nit: extra newline here for consistency (separate return statement from function logic).

+ return opts; }

/** @@ -1317,6 +1326,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 +1577,11 @@ void conf(struct ctx *c, int argc, char **argv)

c->no_pidns = true; break; + case 34: + /* --pass-fds was parsed early and c->pasta_pass_fds set in main() */

It's not actually set in main(). But the relevant fact here is simply that --pass-fds was parsed early, so that would be enough. If it's not enough for some reason, you should wrap this comment to 80 columns.

+ if (c->mode != MODE_PASTA) + die("--pass-fds is for pasta mode only"); + break; case 'd': c->debug = 1; c->quiet = 0; @@ -1929,6 +1944,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..e11661f 100644 --- a/conf.h +++ b/conf.h @@ -6,8 +6,13 @@ #ifndef CONF_H #define CONF_H

+struct fd_opts { + int fd_tap; + bool pass_fds; +}; +

This could go away, if we pass struct ctx to conf_fds().

enum passt_modes conf_mode(int argc, char *argv[]); -int conf_tap_fd(int argc, char **argv); +struct fd_opts conf_fds(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..4105dc3 100644 --- a/isolation.c +++ b/isolation.c @@ -250,46 +250,64 @@ 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; + struct fd_opts opts = conf_fds(argc, argv); + if (opts.fd_tap >= 0 && opts.fd_tap < 3) { + /* Move the tap fd to a safer location */

Do we really need to do this unconditionally, even if we're not closing file descriptors? I don't see a reason. That is, I guess we could make this much simpler, let conf_fds() set c->pasta_pass_fds, and return early here if it's set. No logic changes needed in this function, just something to adapt, in case you go the way I suggested with conf_fds() setting c->fd_tap:

+ int new_fd = fcntl(opts.fd_tap, F_DUPFD, STDERR_FILENO + 1);

- fd = conf_tap_fd(argc, argv); + if (new_fd < 0) + die_perror("Could not relocate --fd descriptor");

- if (fd >= 0) { - /* Move the passed fd to a more convenient location */ - if (fd != close_from &&

...this would need to use c->fd_tap, and...

- (dup2(fd, close_from) != close_from || - close(fd))) - die_perror("Could not move --fd descriptor"); - fd = close_from++; + close(opts.fd_tap); + opts.fd_tap = new_fd; } + c->fd_tap = opts.fd_tap;

...this would go away as well.

+ c->pasta_pass_fds = opts.pass_fds; + + /* Close leaked file descriptors unless --pass-fds is used */ + if (!opts.pass_fds) { + int rc = 0; + /* Keep standard streams and the tap fd (if given) */ + int keep = opts.fd_tap >= STDERR_FILENO ? opts.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; + }

- 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 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"); + } } } - - return fd; }

/** 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

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