Re: [PATCH v2 4/4] fwd: Direct inbound spliced forwards to the guest's external address

[Stefano Brivio <sbrivio@redhat.com>]

Some nits, but the series looks overall good to me and I would also
apply it like this:

On Tue,  1 Oct 2024 16:25:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> In pasta mode, where addressing permits we "splice" connections, forwarding
> directly from host socket to guest/container socket without any L2 or L3
> processing.  This gives us a very large performance improvement when it's
> possible.
> 
> Since the traffic is from a local socket within the guest, it will go over
> the guest's 'lo' interface, and accordingly we set the guest side address
> to be the loopback address.  However this has a surprising side effect:
> sometimes guests will run services that are only supposed to be used within
> the guest and are therefore bound to only 127.0.0.1 and/or ::1.  pasta's
> forwarding exposes those services to the host, which isn't generally what
> we want.
> 
> Correct this by instead forwarding inbound "splice" flows to the guest's
> external address.
> 
> Link: https://github.com/containers/podman/issues/24045
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c  |  9 +++++++++
>  fwd.c   | 31 +++++++++++++++++++++++--------
>  passt.1 | 23 +++++++++++++++++++----
>  passt.h |  1 +
>  4 files changed, 52 insertions(+), 12 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6e62510..b5318f3 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -908,6 +908,9 @@ pasta_opts:
>  		"  -U, --udp-ns SPEC	UDP port forwarding to init namespace\n"
>  		"    SPEC is as described above\n"
>  		"    default: auto\n"
> +		"  --host-lo-to-ns-lo	DEPRECATED:\n"

For me, this is harder to type (and remember) than --expose-ns-lo or
similar. It doesn't matter so much as it's a deprecated option anyway.

--help lines are precious, I wouldn't use one to say DEPRECATED. On the
other hand, it's deprecated :) so I don't care that much (and it has
the advantage of making it very clear for users).

> +		"			Translate host-loopback forwards to\n"
> +		"			namespace loopback\n"
>  		"  --userns NSPATH 	Target user namespace to join\n"
>  		"  --netns PATH|NAME	Target network namespace to join\n"
>  		"  --netns-only		Don't join existing user namespace\n"
> @@ -1284,6 +1287,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"netns-only",	no_argument,		NULL,		20 },
>  		{"map-host-loopback", required_argument, NULL,		21 },
>  		{"map-guest-addr", required_argument,	NULL,		22 },
> +		{"host-lo-to-ns-lo", no_argument, 	NULL,		23 },
>  		{ 0 },
>  	};
>  	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> @@ -1461,6 +1465,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  			conf_nat(optarg, &c->ip4.map_guest_addr,
>  				 &c->ip6.map_guest_addr, NULL);
>  			break;
> +		case 23:
> +			if (c->mode != MODE_PASTA)
> +				die("--host-lo-to-ns-lo is for pasta mode only");
> +			c->host_lo_to_ns_lo = 1;
> +			break;
>  		case 'd':
>  			c->debug = 1;
>  			c->quiet = 0;
> diff --git a/fwd.c b/fwd.c
> index a505098..c71f5e1 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -447,20 +447,35 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  	    (proto == IPPROTO_TCP || proto == IPPROTO_UDP)) {
>  		/* spliceable */
>  
> -		/* Preserve the specific loopback adddress used, but let the
> -		 * kernel pick a source port on the target side
> +		/* The traffic will go over the guest's 'lo' interface, but by
> +		 * default use its external address, so we don't inadvertently
> +		 * expose services that listen only on the guest's loopback
> +		 * address.  That can be overridden by --host-lo-to-ns-lo which
> +		 * will instead forward to the loopback address in the guest.
> +		 *
> +		 * In either case, let the kernel pick the source address to
> +		 * match.
>  		 */
> -		tgt->oaddr = ini->eaddr;
> +		if (inany_v4(&ini->eaddr)) {
> +			if (c->host_lo_to_ns_lo)
> +				tgt->eaddr = inany_loopback4;
> +			else
> +				tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
> +			tgt->oaddr = inany_any4;
> +		} else {
> +			if (c->host_lo_to_ns_lo)
> +				tgt->eaddr = inany_loopback6;
> +			else
> +				tgt->eaddr.a6 = c->ip6.addr_seen;
> +			tgt->oaddr = inany_any6;
> +		}
> +
> +		/* Let the kernel pick source port */
>  		tgt->oport = 0;
>  		if (proto == IPPROTO_UDP)
>  			/* But for UDP preserve the source port */
>  			tgt->oport = ini->eport;
>  
> -		if (inany_v4(&ini->eaddr))
> -			tgt->eaddr = inany_loopback4;
> -		else
> -			tgt->eaddr = inany_loopback6;
> -
>  		return PIF_SPLICE;
>  	}
>  
> diff --git a/passt.1 b/passt.1
> index 11104e1..332384c 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -586,6 +586,13 @@ Configure UDP port forwarding from target namespace to init namespace.
>  
>  Default is \fBauto\fR.
>  
> +.TP
> +.BR \-\-host-lo-to-ns-lo " " (DEPRECATED)
> +If specified, connections forwarded with \fB\-t\fR and \fB\-u\fR from
> +the host's loopback address will appear on the loopback address in the
> +guest as well.  Without this option such forwarded packets will appear
> +to come from the guest's public address.
> +
>  .TP
>  .BR \-\-userns " " \fIspec
>  Target user namespace to join, as a path. If PID is given, without this option,
> @@ -874,8 +881,9 @@ interfaces, and it would also be impossible for guest or target
>  namespace to route answers back.
>  
>  For convenience, the source address on these packets is translated to
> -the address specified by the \fB\-\-map-host-loopback\fR option.  If
> -not specified this defaults, somewhat arbitrarily, to the address of
> +the address specified by the \fB\-\-map-host-loopback\fR option (with
> +some exceptions in pasta mode, see next section below).  If not
> +specified this defaults, somewhat arbitrarily, to the address of
>  default IPv4 or IPv6 gateway (if any) -- this is known to be an
>  existing, valid address on the same subnet.  If \fB\-\-no-map-gw\fR or
>  \fB\-\-map-host-loopback none\fR are specified this translation is
> @@ -912,8 +920,15 @@ and the new socket using the \fBsplice\fR(2) system call, and for UDP, a pair
>  of \fBrecvmmsg\fR(2) and \fBsendmmsg\fR(2) system calls deals with packet
>  transfers.
>  
> -This bypass only applies to local connections and traffic, because it's not
> -possible to bind sockets to foreign addresses.
> +Because it's not possible to bind sockets to foreign addresses, this
> +bypass only applies to local connections and traffic.  It also means
> +that the address translation differs slightly from passt mode.
> +Connections from loopback to loopback on the host will appear to come
> +from the target namespace's public address within the guest, unless
> +\fB\-\-host-lo-to-ns-lo\fR is specified, in which case they will
> +appear to come from loopback in the namespace as well.  The latter
> +behaviour used to be the default, but is usually undesirable, since it
> +can unintentionally expose namespace local services to the host.
>  
>  .SS Binding to low numbered ports (well-known or system ports, up to 1023)
>  
> diff --git a/passt.h b/passt.h
> index 031c9b6..3b7b281 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -284,6 +284,7 @@ struct ctx {
>  	int no_dhcpv6;
>  	int no_ndp;
>  	int no_ra;
> +	int host_lo_to_ns_lo;

This is missing from the struct comment, but... again, it's deprecated,
so it doesn't matter much.

-- 
Stefano