Re: [PATCH v2 3/5] passt, tap: Add --fd option

["Richard W.M. Jones" <rjones@redhat.com>]

On Thu, Nov 17, 2022 at 04:26:40PM +0100, Stefano Brivio wrote:
> From: "Richard W.M. Jones" <rjones@redhat.com>
> 
> This passes a fully connected stream socket to passt.
> 
> Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> [sbrivio: reuse fd_tap instead of adding a new descriptor,
>  imply --one-off on --fd, add to optstring and usage()]
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> v2:
>  - reuse fd_tap, we don't need a separate file descriptor
>  - add F to optstring and usage(), for both passt and pasta
>  - imply --one-off, we can't do much once the socket is closed
> 
> With this, the trick from 5/5 goes a bit further: passt reads
> from the file descriptor passed by the wrapper.

Thanks for the v2 .. I'll add it to my series and play with it.

> However, we get EPOLLRDHUP right away, from the close() on
> one end of the socket pair I guess. Should we just ignore
> all EPOLLRDHUP events, just the first one...?

Does it see the event out-of-band or does it get an in-band
read(2) == 0 after finishing reading the data?

Ideally what should happen is that passt reads and parses all or as
much of the data as it can, and then it responds to the closed socket
by exiting.  The reason for this is so that we have as much
opportunity as possible to break the parser and crash passt.

If passt was exiting before fully reading/parsing everything that it
could, then we wouldn't be fuzzing the parser as deeply as we can.

Note that the fuzzer will (quite routinely and normally) create test
cases which are total nonsense, eg. packets with incorrect length
field, truncated final packet, etc.

Rich.

>  conf.c  | 28 ++++++++++++++++++++++++++--
>  passt.1 | 10 ++++++++++
>  passt.c |  1 -
>  passt.h |  2 +-
>  tap.c   |  9 +++++++++
>  5 files changed, 46 insertions(+), 4 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index a995eb7..2c374bd 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -719,6 +719,7 @@ static void usage(const char *name)
>  		     UNIX_SOCK_PATH, 1);
>  	}
>  
> +	info(   "  -F, --fd FD		Use FD as pre-opened connected socket");
>  	info(   "  -p, --pcap FILE	Log tap-facing traffic to pcap file");
>  	info(   "  -P, --pid FILE	Write own PID to the given file");
>  	info(   "  -m, --mtu MTU	Assign MTU via DHCP/NDP");
> @@ -1079,6 +1080,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"log-file",	required_argument,	NULL,		'l' },
>  		{"help",	no_argument,		NULL,		'h' },
>  		{"socket",	required_argument,	NULL,		's' },
> +		{"fd",		required_argument,	NULL,		'F' },
>  		{"ns-ifname",	required_argument,	NULL,		'I' },
>  		{"pcap",	required_argument,	NULL,		'p' },
>  		{"pid",		required_argument,	NULL,		'P' },
> @@ -1138,9 +1140,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  	if (c->mode == MODE_PASTA) {
>  		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
> -		optstring = "dqfel:hI:p:P:m:a:n:M:g:i:D:S:46t:u:T:U:";
> +		optstring = "dqfel:hf:I:p:P:m:a:n:M:g:i:D:S:46t:u:T:U:";
>  	} else {
> -		optstring = "dqfel:hs:p:P:m:a:n:M:g:i:D:S:461t:u:";
> +		optstring = "dqfel:hs:f:p:P:m:a:n:M:g:i:D:S:461t:u:";
>  	}
>  
>  	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = 0;
> @@ -1355,6 +1357,23 @@ void conf(struct ctx *c, int argc, char **argv)
>  				err("Invalid socket path: %s", optarg);
>  				usage(argv[0]);
>  			}
> +			break;
> +		case 'F':
> +			if (c->fd_tap >= 0) {
> +				err("Multiple --fd options given");
> +				usage(argv[0]);
> +			}
> +
> +			errno = 0;
> +			c->fd_tap = strtol(optarg, NULL, 0);
> +
> +			if (c->fd_tap < 0 || errno) {
> +				err("Invalid --fd: %s", optarg);
> +				usage(argv[0]);
> +			}
> +
> +			c->one_off = true;
> +
>  			break;
>  		case 'I':
>  			if (*c->pasta_ifn) {
> @@ -1590,6 +1609,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  		usage(argv[0]);
>  	}
>  
> +	if (*c->sock_path && c->fd_tap >= 0) {
> +		err("Options --socket and --fd are mutually exclusive");
> +		usage(argv[0]);
> +	}
> +
>  	ret = conf_ugid(runas, &uid, &gid);
>  	if (ret)
>  		usage(argv[0]);
> diff --git a/passt.1 b/passt.1
> index e34a3e0..528763b 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -297,6 +297,16 @@ Path for UNIX domain socket used by \fBqemu\fR(1) or \fBqrap\fR(1) to connect to
>  Default is to probe a free socket, not accepting connections, starting from
>  \fI/tmp/passt_1.socket\fR to \fI/tmp/passt_64.socket\fR.
>  
> +.TP
> +.BR \-F ", " \-\-fd " " \fIFD
> +Pass a pre-opened, connected socket to \fBpasst\fR. Usually the socket is opened
> +in the parent process and \fBpasst\fR inherits it when run as a child. This
> +allows the parent process to open sockets using another address family or
> +requiring special privileges.
> +
> +This option implies the behaviour described for \-\-one-off, once this socket
> +is closed.
> +
>  .TP
>  .BR \-1 ", " \-\-one-off
>  Quit after handling a single client connection, that is, once the client closes
> diff --git a/passt.c b/passt.c
> index 7d323c2..8b2c50d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -255,7 +255,6 @@ int main(int argc, char **argv)
>  
>  	quit_fd = pasta_netns_quit_init(&c);
>  
> -	c.fd_tap = c.fd_tap_listen = -1;
>  	tap_sock_init(&c);
>  
>  	clock_gettime(CLOCK_MONOTONIC, &now);
> diff --git a/passt.h b/passt.h
> index 6649c0a..ca25b90 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -159,7 +159,7 @@ struct ip6_ctx {
>   * @proc_net_udp:	Stored handles for /proc/net/udp{,6} in init and ns
>   * @epollfd:		File descriptor for epoll instance
>   * @fd_tap_listen:	File descriptor for listening AF_UNIX socket, if any
> - * @fd_tap:		File descriptor for AF_UNIX socket or tuntap device
> + * @fd_tap:		AF_UNIX socket, tuntap device, or pre-opened socket
>   * @mac:		Host MAC address
>   * @mac_guest:		MAC address of guest or namespace, seen or configured
>   * @ifi4:		Index of routable interface for IPv4, 0 if IPv4 disabled
> diff --git a/tap.c b/tap.c
> index d26af58..9998127 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1069,6 +1069,15 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->fd_tap != -1) {
> +		if (c->one_off) {	/* Passed as --fd */
> +			struct epoll_event ev = { 0 };
> +
> +			ev.data.fd = c->fd_tap;
> +			ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
> +			epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
> +			return;
> +		}
> +
>  		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
>  		close(c->fd_tap);
>  		c->fd_tap = -1;
> -- 
> 2.35.1

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html