Re: [PATCH v3] feat: Add cli option '--pass-fds' for pasta mode.

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11116 bytes --]

On Sun, Jul 19, 2026 at 03:03:00PM -0500, Richard Lawrence wrote:
> From: Richard Lawrence <rarensu@tamu.edu>
> 
> When pasta mode is used to launch an executable (`pasta [COMMAND]`) and that executable accepts inputs in the form of arbitrary file descriptors (such as `bwrap`), then passt should not stand in the way of the parent process handing off those file descriptors to the child process. See bug 204 for additional discussion.

Nit: wrap commit messages at 80 columns, please.

> The `pass-fds` option accepts a comma-separated list of file
> descriptor numbers. `conf_pass_fds()` parses the command line
> argument, then `isolate_fds()` skips closing the specified fds by
> calling `close_range()` on the gaps between them.

I'm still not really seeing the value of this option, over starting
the namespace separately, passing the fds you want, and then
connecting pasta.  Or you can do it the other way around: start with a
placeholder process, then use nsenter to put the "real" workload into
the same ns, along with whatever fds you want (this approach can be a
bit fiddly to discover the right pid in the parent namespace, though).

In other words, neither here, nor on the bug have I seen any response
to my comments in https://bugs.passt.top/show_bug.cgi?id=204#c3

I have comments on the implementation below, but addressing all those
would not be enough for me to approve this patch, without a case for
why the feature is sufficiently valuable in the first place.

> Additionally, the tap fd is safely relocated to the lowest unused fd
> number which it at least 3, to avoid accidentally overwriting an
> existing fd.

Thank you for removing the confusing Co-authored tags.  Obviously I
can't be certain but this patch still reads like it was LLM assisted,
which should be acknowledged in some way.  My personal preference
would be the `Assisted-by` tag recently adopted by the Linux kernel,
but Stefano and I haven't discussed that yet.

> Signed-off-by: Richard Lawrence <rlawrence@tamu.edu>
> ---
>  conf.c      | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  conf.h      |  3 +++
>  isolation.c | 73 ++++++++++++++++++++++++++++++++++++++++++----------
>  passt.1     |  5 ++++
>  4 files changed, 140 insertions(+), 15 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 0fcba5c..729816d 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -732,7 +732,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 FDS	Comma-separated list of fds to pass to\n"
> +		"			the spawned command\n");
>  
>  	passt_exit(status);
>  }
> @@ -1183,6 +1185,71 @@ int conf_tap_fd(int argc, char **argv)
>  	return val;
>  }
>  
> +/**
> + * conf_pass_fds() - Read fds as supplied by --pass-fds command line option
> + * @argc:	Argument count
> + * @argv:	Command line options
> + * @fds:	Array where we store the parsed fds
> + * @max_fds:	Maximum size of the array
> + *
> + * Return: number of parsed fds, or -1 if option not specified
> + */
> +int conf_pass_fds(int argc, char **argv, int *fds, int max_fds)
> +{
> +	const struct option opt[] = { { "pass-fds", required_argument, NULL, 33 },
> +				      { 0 }, };

I'd be inclined to parse --fd and --pass-fds in a single early pass,
rather than in two functions
since we need them at the same time

> +	const char *fdsarg = NULL;
> +	int name, fds_cnt = 0;
> +	int old_opterr;
> +
> +	old_opterr = opterr;

Um.. why?  getopt_long() is thoroughly non-reentrant and saving the
error code isn't going to make it so.

> +	opterr = 0;
> +	optind = 0;
> +	do {
> +		name = getopt_long(argc, argv, "-:", opt, NULL);
> +		if (name == 33)
> +			fdsarg = optarg;
> +	} while (name != -1);
> +	opterr = old_opterr;
> +
> +	if (!fdsarg)
> +		return -1;
> +
> +	const char *orig_fdsarg = fdsarg;

Although it's permitted by C11 we don't use inline declarations, by
convention.

> +	while (*fdsarg) {
> +		unsigned long val;
> +		char *endptr;
> +
> +		val = strtoul(fdsarg, &endptr, 10);
> +		if (fdsarg == endptr) {
> +			die("Invalid character in --pass-fds option '%s' (near '%s')",
> +			    orig_fdsarg, fdsarg);
> +		}

The recently added parse_unsigned() and related functions can do this
more nicely.  But.. it also occurs to me that having the moderately
complex parsing of a comma separated list is unnecessary.  Seems
simpler to change it to --pass-fd, which takes only a single fd, and
can be specified multiple times.

> +
> +		if (val > INT_MAX) {
> +			die("Invalid file descriptor in --pass-fds option '%s' (near '%s')",
> +			    orig_fdsarg, fdsarg);
> +		}
> +
> +		if (fds_cnt >= max_fds)
> +			die("Too many file descriptors in --pass-fds");
> +
> +		fds[fds_cnt++] = (int)val;
> +
> +		if (*endptr == ',')
> +			fdsarg = endptr + 1;
> +		else if (*endptr == '\0')
> +			fdsarg = endptr;
> +		else {
> +			die("Invalid character in --pass-fds option '%s' (near '%s')",
> +			    orig_fdsarg, endptr);
> +		}
> +	}
> +
> +	return fds_cnt;
> +}
> +
>  /**
>   * conf_addr() - Configure guest address with -a option
>   * @c:		Execution context
> @@ -1331,6 +1398,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"stats", required_argument,		NULL,		31 },
>  		{"conf-path",	required_argument,	NULL,		'c' },
>  		{"chroot-fallback", no_argument,	NULL, 		32 },
> +		{"pass-fds",	required_argument,	NULL,		33 },
>  		{ 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:";
> @@ -1572,6 +1640,10 @@ void conf(struct ctx *c, int argc, char **argv)
>  		case 32:
>  			c->chroot_fallback = true;
>  			break;
> +		case 33:
> +			if (c->mode != MODE_PASTA)
> +				die("--pass-fds is for pasta mode only");
> +			break;
>  		case 'd':
>  			c->debug = 1;
>  			c->quiet = 0;
> diff --git a/conf.h b/conf.h
> index 19bf9bc..28a9acc 100644
> --- a/conf.h
> +++ b/conf.h
> @@ -6,7 +6,10 @@
>  #ifndef CONF_H
>  #define CONF_H
>  
> +#define PASS_FDS_MAX	1024
> +
>  enum passt_modes conf_mode(int argc, char *argv[]);
> +int conf_pass_fds(int argc, char **argv, int *fds, int max_fds);
>  int conf_tap_fd(int argc, char **argv);
>  void conf(struct ctx *c, int argc, char **argv);
>  void conf_listen_handler(struct ctx *c, uint32_t events);
> diff --git a/isolation.c b/isolation.c
> index 94cbe7f..25f599b 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -249,32 +249,77 @@ void isolate_initial(void)
>  }
>  
>  /*
> - * isolate_fds() - Close leaked files, but not --fd, stdin, stdout, stderr
> + * isolate_fds() - Close leaked files, but not --fd, --pass-fds, standard streams
>   * @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, --fd, and --pass-fds
>   *  - move the --fd descriptor out of the range 0-2
>   *
>   * Return: new fd number for descriptor from --fd, or -1 if not specified
>   */
>  int isolate_fds(int argc, char **argv)
>  {
> -	int fd, close_from = STDERR_FILENO + 1;
> +	int fds[PASS_FDS_MAX + 1];
> +	int fds_cnt = 0;
> +	int prev_fd;
> +	int next_fd;
> +	int tap_fd;
> +	int rc = 0;
> +	int i;
> +
> +	tap_fd = conf_tap_fd(argc, argv);
> +	if (tap_fd >= 0 && tap_fd < 3) {

Use STD*_FILENO constants.

> +		/* Move the tap fd to a safer location */
> +		int new_fd = fcntl(tap_fd, F_DUPFD, STDERR_FILENO + 1);
> +
> +		if (new_fd < 0)
> +			die_perror("Could not relocate --fd descriptor");
> +
> +		close(tap_fd);
> +		tap_fd = new_fd;
> +	}
> +
> +	if (tap_fd >= 0)
> +		/* Keep the tap fd */
> +		fds[fds_cnt++] = tap_fd;
> +
> +	rc = conf_pass_fds(argc, argv, fds + fds_cnt, PASS_FDS_MAX);

This is only correct because fds_cnt can't exceed 1 at this point,
which is dangerously subtle.  It would probably be more natural to
parse --pass-fds first, then append --fd.

> +	if (rc > 0)
> +		/* Keep the pass-fds */
> +		fds_cnt += rc;
>  
> -	fd = conf_tap_fd(argc, argv);
> +	rc = 0;
>  
> -	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++;
> +	/* Keep standard streams */
> +	prev_fd = STDERR_FILENO;
> +
> +	while (1) {
> +		next_fd = -1;
> +
> +		/* Find the next-lowest fd to keep */
> +		for (i = 0; i < fds_cnt; i++) {
> +			if (fds[i] > prev_fd && (next_fd == -1 || fds[i] < next_fd))
> +				next_fd = fds[i];
> +		}
> +
> +		if (next_fd == -1)
> +			break;
> +
> +		if (next_fd > prev_fd + 1) {
> +			/* Close fds between two kept fds */
> +			if (close_range(prev_fd + 1, next_fd - 1, CLOSE_RANGE_UNSHARE))
> +				rc = -1;
> +		}
> +		prev_fd = next_fd;
>  	}
> +	
> +	/* Close all other fds */
> +	if (close_range(prev_fd + 1, ~0U, CLOSE_RANGE_UNSHARE))
> +		rc = -1;
>  
> -	if (close_range(close_from, ~0U, CLOSE_RANGE_UNSHARE)) {
> +	if (rc) {
>  		if (errno == ENOSYS || errno == EINVAL) {

If the failure was not in the last close_range(), errno has been
clobbered before you test it here.

It's a bit simpler than the earlier version, but this still seems very
involved for something that can be avoided by a different way of
invoking your program.

>  			/* This probably means close_range() or the
>  			 * CLOSE_RANGE_UNSHARE flag is not supported by the
> @@ -288,7 +333,7 @@ int isolate_fds(int argc, char **argv)
>  		}
>  	}
>  
> -	return fd;
> +	return tap_fd;
>  }
>  
>  /**
> diff --git a/passt.1 b/passt.1
> index 995590a..bcd3aff 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -755,6 +755,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 " " \fIfds\fR
> +Pass a comma-separated list of file descriptors to the spawned command.
> +These file descriptors will be kept open.

"Will be kept open" is not necessarily very clear to the audience of
this document.

It's also true in a way that isn't ideal - even after we've fork()ed
off the command, we don't close() the remaining fds, weakening more
than strictly necessary the security benefits of isolate_fds().

> +
>  .SH EXAMPLES
>  
>  .SS \fBpasta
> -- 
> 2.52.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]