public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top, Paul Holzinger <pholzing@redhat.com>
Subject: Re: [PATCH 20/22] conf: Allow address remapped to host to be configured
Date: Tue, 20 Aug 2024 21:56:34 +0200	[thread overview]
Message-ID: <20240820215634.556666ec@elisabeth> (raw)
In-Reply-To: <20240816054004.1335006-21-david@gibson.dropbear.id.au>

On Fri, 16 Aug 2024 15:40:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Because the host and guest share the same IP address with passt/pasta, it's
> not possible for the guest to directly address the host.  Therefore we
> allow packets from the guest going to a special "NAT to host" address to be
> redirected to the host, appearing there as though they have both source and
> destination address of loopback.
> 
> Currently that special address is always the address of the default
> gateway (or none).  That can be a problem if we want that gateway to be
> addressable by the guest.  Therefore, allow the special "NAT to host"
> address to be overridden on the command line with a new --nat-host-loopback
> option.
> 
> In order to exercise and test it, update the passt_in_ns and perf
> tests to use this option and give different mapping addresses for the
> two layers of the environment.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c                | 57 +++++++++++++++++++++++++++++++--
>  passt.1               | 16 ++++++++++
>  test/lib/setup        | 11 +++++--
>  test/passt_in_ns/dhcp | 73 +++++++++++++++++++++++++++++++++++++++++++
>  test/passt_in_ns/tcp  | 38 +++++++++++-----------
>  test/passt_in_ns/udp  | 22 +++++++------
>  test/perf/passt_tcp   | 33 +++++++++----------
>  test/perf/passt_udp   | 31 +++++++++---------
>  test/perf/pasta_tcp   | 29 ++++++++---------
>  test/perf/pasta_udp   | 25 ++++++++-------
>  test/run              |  4 +--
>  11 files changed, 244 insertions(+), 95 deletions(-)
>  create mode 100644 test/passt_in_ns/dhcp
> 
> diff --git a/conf.c b/conf.c
> index 26373584..c5831e82 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -817,6 +817,14 @@ static void usage(const char *name, FILE *f, int status)
>  		fprintf(f, "  --no-dhcp-search	No list in DHCP/DHCPv6/NDP\n");
>  
>  	fprintf(f,
> +		"  --nat-host-loopback ADDR	NAT ADDR to refer to host\n"
> +		"    Packets from the guest to ADDR will be redirected to the\n"
> +		"    host.  On the host such packets will appear to have both\n"
> +		"    source and destination of loopback (127.0.0.1 or ::1).\n"

I would leave these three lines to the man page. The help message is
already 90 lines long. This should be a quick guide/reminder, not a
full description.

This reminds me that 127.0.0.1 isn't the only IPv4 loopback address. I
don't know if anybody will ever have a use case where they would need
a different, specific, loopback source address, but, together with
--nat-guest-addr from 22/22, I start wondering: what if we had a single
option taking, optionally, an arbitrary (within limits) source address?

Now, given that we plan to add a configurable flow table at some point
in the future, it makes no sense to make this exceedingly flexible. But
I just wanted to bring this up for consideration, in case it's doable
at a small cost (I'm really not sure):

  --map-host [source,]address

where "source" would default to 127.0.0.1, but it could also be another
loopback address, or another address altogether (and we'll fail if it's
not local, of course).

If we want (can?) go that way and keep equivalent functionality as you
have now, we would have the additional problem that this option could
be given up to two times (one for loopback, one for non-loopback), and
not more (we don't have a data structure ready for an arbitrary number
of those), so it's not as generic as it might look like, and I'm not
sure if it's a good idea. But we could also expand on it in the future.

> +		"    ADDR can be 'none', in which case nothing is mapped\n"

This is a nice feature by the way as it should eventually allow us to
get consistent options in Podman instead of "--map-gw": Podman could
add by default '--map-host-loopback none', unless the user overrides
that with an actual address.

> +	        "    Can be specified zero to two (for IPv4 and IPv6)\n"

"can" (for consistency, but also because the subject is still the
option, this is not a separate sentence).

...times.

> +		"    default: gateway address, or none if --no-map-gw is also\n"
> +		"             specified\n"

I don't think we need to mention here that --no-map-gw implies none,
doing it in the man page is enough.

>  		"  --dns-forward ADDR	Forward DNS queries sent to ADDR\n"
>  		"    can be specified zero to two times (for IPv4 and IPv6)\n"
>  		"    default: don't forward DNS queries\n"
> @@ -959,6 +967,11 @@ static void conf_print(const struct ctx *c)
>  	info("    host: %s", eth_ntop(c->our_tap_mac, bufmac, sizeof(bufmac)));
>  
>  	if (c->ifi4) {
> +		if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.nat_host_loopback))
> +			info("    NAT to host 127.0.0.1: %s",
> +			     inet_ntop(AF_INET, &c->ip4.nat_host_loopback,
> +				       buf4, sizeof(buf4)));
> +
>  		if (!c->no_dhcp) {
>  			uint32_t mask;
>  
> @@ -989,6 +1002,11 @@ static void conf_print(const struct ctx *c)
>  	}
>  
>  	if (c->ifi6) {
> +		if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.nat_host_loopback))
> +			info("    NAT to host ::1: %s",
> +			     inet_ntop(AF_INET6, &c->ip6.nat_host_loopback,
> +				       buf6, sizeof(buf6)));
> +
>  		if (!c->no_ndp && !c->no_dhcpv6)
>  			info("NDP/DHCPv6:");
>  		else if (!c->no_ndp)
> @@ -1122,6 +1140,35 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  	}
>  }
>  
> +/**
> + * conf_nat() - Parse --nat-host-loopback option
> + * @c:		Execution context
> + * @arg:	String argument to --nat-host-loopback
> + * @no_map_gw:	--no-map-gw flag, updated for "none" argument
> + */
> +static void conf_nat(struct ctx *c, const char *arg, int *no_map_gw)
> +{
> +	if (strcmp(arg, "none") == 0) {
> +		c->ip4.nat_host_loopback = in4addr_any;
> +		c->ip6.nat_host_loopback = in6addr_any;
> +		*no_map_gw = 1;
> +	}
> +
> +	if (inet_pton(AF_INET6, arg, &c->ip6.nat_host_loopback) &&
> +	    !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.nat_host_loopback)	&&
> +	    !IN6_IS_ADDR_LOOPBACK(&c->ip6.nat_host_loopback)	&&
> +	    !IN6_IS_ADDR_MULTICAST(&c->ip6.nat_host_loopback))
> +		return;
> +
> +	if (inet_pton(AF_INET, arg, &c->ip4.nat_host_loopback)	&&
> +	    !IN4_IS_ADDR_UNSPECIFIED(&c->ip4.nat_host_loopback)	&&
> +	    !IN4_IS_ADDR_LOOPBACK(&c->ip4.nat_host_loopback)	&&
> +	    !IN4_IS_ADDR_MULTICAST(&c->ip4.nat_host_loopback))
> +		return;
> +
> +	die("Invalid address to remap to host: %s", optarg);
> +}
> +
>  /**
>   * conf_open_files() - Open files as requested by configuration
>   * @c:		Execution context
> @@ -1231,6 +1278,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"no-copy-routes", no_argument,		NULL,		18 },
>  		{"no-copy-addrs", no_argument,		NULL,		19 },
>  		{"netns-only",	no_argument,		NULL,		20 },
> +		{"nat-host-loopback", required_argument, NULL,		21 },
>  		{ 0 },
>  	};
>  	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> @@ -1400,6 +1448,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  			netns_only = 1;
>  			*userns = 0;
>  			break;
> +		case 21:
> +			conf_nat(c, optarg, &no_map_gw);
> +			break;
>  		case 'd':
>  			c->debug = 1;
>  			c->quiet = 0;
> @@ -1639,10 +1690,12 @@ void conf(struct ctx *c, int argc, char **argv)
>  	    (*c->ip6.ifname_out && !c->ifi6))
>  		die("External interface not usable");
>  
> -	if (c->ifi4 && !no_map_gw)
> +	if (c->ifi4 && !no_map_gw &&
> +	    IN4_IS_ADDR_UNSPECIFIED(&c->ip4.nat_host_loopback))
>  		c->ip4.nat_host_loopback = c->ip4.guest_gw;
>  
> -	if (c->ifi6 && !no_map_gw)
> +	if (c->ifi6 && !no_map_gw &&
> +	    IN6_IS_ADDR_UNSPECIFIED(&c->ip6.nat_host_loopback))
>  		c->ip6.nat_host_loopback = c->ip6.guest_gw;
>  
>  	if (c->ifi4 && IN4_IS_ADDR_UNSPECIFIED(&c->ip4.our_tap_addr))
> diff --git a/passt.1 b/passt.1
> index dca433b6..3680056a 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -327,6 +327,22 @@ namespace will be silently dropped.
>  Disable Router Advertisements. Router Solicitations coming from guest or target
>  namespace will be ignored.
>  
> +.TP
> +.BR \-\-nat-host-loopback " " \fIaddr
> +Translate \fIaddr\fR to refer to the host. Packets from the guest to
> +\fIaddr\fR will be redirected to the host.  On the host such packets
> +will appear to have both source and destination of loopback (127.0.0.1

I would skip "of loopback" and just say "127.0.0.1 or ::1", to avoid
implying that there's a single loopback address for IPv4.

> +or ::1).
> +
> +If \fIaddr\fR is 'none', no address is mapped (this implies
> +\fB--no-map-gw\fR).  Only one IPv4 and one IPv6 address can be
> +translated, if the option is specified multiple times, the last one
> +takes effect.
> +
> +Default is to translate the guest's default gateway address, unless
> +\fB--no-map-gw\fR is also given, in which case no address is mapped by

Why "also"? You're describing the default, so I guess this option is
not actually given in that case.

> +default.
> +
>  .TP
>  .BR \-\-no-map-gw
>  Don't remap TCP connections and untracked UDP traffic, with the gateway address
> diff --git a/test/lib/setup b/test/lib/setup
> index 9b39b9fe..061bf997 100755
> --- a/test/lib/setup
> +++ b/test/lib/setup
> @@ -124,7 +124,12 @@ setup_passt_in_ns() {
>  	[ ${DEBUG} -eq 1 ] && __opts="${__opts} -d"
>  	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
>  
> -	context_run_bg pasta "./pasta ${__opts} -t 10001,10002,10011,10012 -T 10003,10013 -u 10001,10002,10011,10012 -U 10003,10013 -P ${STATESETUP}/pasta.pid --config-net ${NSTOOL} hold ${STATESETUP}/ns.hold"
> +        __nat_host4=192.0.2.1
> +        __nat_host6=2001:db8:9a55::1
> +        __nat_ns4=192.0.2.2
> +        __nat_ns6=2001:db8:9a55::2
> +
> +	context_run_bg pasta "./pasta ${__opts} -t 10001,10002,10011,10012 -T 10003,10013 -u 10001,10002,10011,10012 -U 10003,10013 -P ${STATESETUP}/pasta.pid --nat-host-loopback ${__nat_host4} --nat-host-loopback ${__nat_host6} --config-net ${NSTOOL} hold ${STATESETUP}/ns.hold"
>  	wait_for [ -f "${STATESETUP}/pasta.pid" ]
>  
>  	context_setup_nstool qemu ${STATESETUP}/ns.hold
> @@ -139,11 +144,11 @@ setup_passt_in_ns() {
>  	if [ ${VALGRIND} -eq 1 ]; then
>  		context_run passt "make clean"
>  		context_run passt "make valgrind"
> -		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid"
> +		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --nat-host-loopback ${__nat_ns4} --nat-host-loopback ${__nat_ns6}"
>  	else
>  		context_run passt "make clean"
>  		context_run passt "make"
> -		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid"
> +		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --nat-host-loopback ${__nat_ns4} --nat-host-loopback ${__nat_ns6}"
>  	fi
>  	wait_for [ -f "${STATESETUP}/passt.pid" ]
>  
> diff --git a/test/passt_in_ns/dhcp b/test/passt_in_ns/dhcp
> new file mode 100644
> index 00000000..48c7d197
> --- /dev/null
> +++ b/test/passt_in_ns/dhcp

...how did this happen? This file already exists.

-- 
Stefano


  reply	other threads:[~2024-08-20 19:56 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-16  5:39 [PATCH 00/22] RFC: Allow configuration of special case NATs David Gibson
2024-08-16  5:39 ` [PATCH 01/22] treewide: Use "our address" instead of "forwarding address" David Gibson
2024-08-18 15:44   ` Stefano Brivio
2024-08-19  1:28     ` David Gibson
2024-08-16  5:39 ` [PATCH 02/22] util: Helper for formatting MAC addresses David Gibson
2024-08-18 15:44   ` Stefano Brivio
2024-08-19  1:29     ` David Gibson
2024-08-16  5:39 ` [PATCH 03/22] treewide: Rename MAC address fields for clarity David Gibson
2024-08-18 15:45   ` Stefano Brivio
2024-08-19  1:36     ` David Gibson
2024-08-16  5:39 ` [PATCH 04/22] treewide: Use struct assignment instead of memcpy() for IP addresses David Gibson
2024-08-18 15:45   ` Stefano Brivio
2024-08-19  1:38     ` David Gibson
2024-08-16  5:39 ` [PATCH 05/22] conf: Use array indices rather than pointers for DNS array slots David Gibson
2024-08-16  5:39 ` [PATCH 06/22] conf: More accurately count entries added in get_dns() David Gibson
2024-08-16  5:39 ` [PATCH 07/22] conf: Move DNS array bounds checks into add_dns[46] David Gibson
2024-08-16  5:39 ` [PATCH 08/22] conf: Move adding of a nameserver from resolv.conf into subfunction David Gibson
2024-08-16  5:39 ` [PATCH 09/22] conf: Correct setting of dns_match address in add_dns6() David Gibson
2024-08-16  5:39 ` [PATCH 10/22] conf: Treat --dns addresses as guest visible addresses David Gibson
2024-08-16  5:39 ` [PATCH 11/22] conf: Remove incorrect initialisation of addr_ll_seen David Gibson
2024-08-16  5:39 ` [PATCH 12/22] util: Correct sock_l4() binding for link local addresses David Gibson
2024-08-20  0:14   ` Stefano Brivio
2024-08-20  1:29     ` David Gibson
2024-08-16  5:39 ` [PATCH 13/22] treewide: Change misleading 'addr_ll' name David Gibson
2024-08-20  0:15   ` Stefano Brivio
2024-08-20  1:30     ` David Gibson
2024-08-16  5:39 ` [PATCH 14/22] Clarify which addresses in ip[46]_ctx are meaningful where David Gibson
2024-08-16  5:39 ` [PATCH 15/22] Initialise our_tap_ll to ip6.gw when suitable David Gibson
2024-08-16  5:39 ` [PATCH 16/22] fwd: Helpers to clarify what host addresses aren't guest accessible David Gibson
2024-08-20 19:56   ` Stefano Brivio
2024-08-21  1:40     ` David Gibson
2024-08-16  5:39 ` [PATCH 17/22] fwd: Split notion of "our tap address" from gateway for IPv4 David Gibson
2024-08-20 19:56   ` Stefano Brivio
2024-08-21  1:56     ` David Gibson
2024-08-16  5:39 ` [PATCH 18/22] Don't take "our" MAC address from the host David Gibson
2024-08-16  5:40 ` [PATCH 19/22] conf, fwd: Split notion of gateway/router from guest-visible host address David Gibson
2024-08-20 19:56   ` Stefano Brivio
2024-08-21  1:59     ` David Gibson
2024-08-16  5:40 ` [PATCH 20/22] conf: Allow address remapped to host to be configured David Gibson
2024-08-20 19:56   ` Stefano Brivio [this message]
2024-08-21  2:23     ` David Gibson
2024-08-16  5:40 ` [PATCH 21/22] fwd: Distinguish translatable from untranslatable addresses on inbound David Gibson
2024-08-16  5:40 ` [PATCH 22/22] fwd, conf: Allow NAT of the guest's assigned address David Gibson
2024-08-20 19:56   ` Stefano Brivio
2024-08-21  2:28     ` David Gibson
2024-08-16 14:45 ` [PATCH 00/22] RFC: Allow configuration of special case NATs Paul Holzinger
2024-08-16 15:03   ` Stefano Brivio
2024-08-17  8:01     ` David Gibson
2024-08-19  8:46 ` David Gibson
2024-08-19  9:27   ` Stefano Brivio
2024-08-19  9:52     ` David Gibson
2024-08-19 13:01       ` Stefano Brivio
2024-08-20  0:42         ` David Gibson
2024-08-20 20:39           ` Stefano Brivio
2024-08-21  2:51             ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240820215634.556666ec@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=passt-dev@passt.top \
    --cc=pholzing@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).