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

[Stefano Brivio <sbrivio@redhat.com>]

On Mon, 26 Oct 2026 12:03:45 -0500
Richard Lawrence <rlawrence@tamu.edu> 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 <rlawrence@tamu.edu>
> ---
>  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 <unistd.h>
>  
>  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