From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=fail reason="key not found in DNS" header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202312 header.b=J9ng/Sqn; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 0C5FA5A026E for ; Wed, 21 Aug 2024 04:51:23 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1724208670; bh=2UO8AcXlzOPbSOlU3dQpyRZkbu2iz83oUXHpBnPP7nk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=J9ng/SqnJNZ1e3J8sV/pctiAj0GftIYISFcHdtBpFgRuv+FNDDduDw9AjgQ7WKC8X v6BzKEqJR6Sr06OASFG/I0/FDFdy6Lci6PKbia5fZkIX1rSZqDR0LtLIJDLNcJvseF yNW0MQUadRtl6tY6n6Q0uq+TSZNA9u/JK02ksJCdWI3bES5v7F+LnANr7xkn6pjEIr WVQuKCaKfZZGYt97RNa8HLcSzQ/FZS/5osqK90YJyzFkjET/1sbLDK1/RPnlT1LAfE 8fZvl1eeKqKBu2QUwYYC7fkFGEbOQKuZCbCdem8UbybKoH45lZdTLIwswLFNg+mkGn uQRj0N4jWMUJQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4WpW721sRHz4x8H; Wed, 21 Aug 2024 12:51:10 +1000 (AEST) Date: Wed, 21 Aug 2024 12:23:36 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 20/22] conf: Allow address remapped to host to be configured Message-ID: References: <20240816054004.1335006-1-david@gibson.dropbear.id.au> <20240816054004.1335006-21-david@gibson.dropbear.id.au> <20240820215634.556666ec@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wlS7bTF0u1Jx6BFU" Content-Disposition: inline In-Reply-To: <20240820215634.556666ec@elisabeth> Message-ID-Hash: 7CV4YFKO4WUSPNUZ5PQJK7PFVGBF73FO X-Message-ID-Hash: 7CV4YFKO4WUSPNUZ5PQJK7PFVGBF73FO X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --wlS7bTF0u1Jx6BFU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 20, 2024 at 09:56:34PM +0200, Stefano Brivio wrote: > On Fri, 16 Aug 2024 15:40:01 +1000 > David Gibson wrote: >=20 > > 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 t= o be > > redirected to the host, appearing there as though they have both source= and > > destination address of loopback. > >=20 > > 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-loop= back > > option. > >=20 > > 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. > >=20 > > Signed-off-by: David Gibson > > --- > > 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 > >=20 > > 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 s= tatus) > > fprintf(f, " --no-dhcp-search No list in DHCP/DHCPv6/NDP\n"); > > =20 > > 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" >=20 > 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. Good idea, done. > 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 This is primarily about translation of outbound connections, so loopback is more the destination address than the source here. > --nat-guest-addr from 22/22, I start wondering: what if we had a single > option taking, optionally, an arbitrary (within limits) source address? I'd like to see that, but it's a more complex exercise - we'd need a table of NATs to step through. This series is just aiming to handle the most common cases for now. > 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): >=20 > --map-host [source,]address >=20 > 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). There's no particular reason it has to fail if non-local. Even if we have this in future, I think --map-guest-addr would still be useful because it avoids the user having to spell out what host address they expect the guest to take. > 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. Yeah, I see this more as a future extension. > > + " ADDR can be 'none', in which case nothing is mapped\n" >=20 > 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. Exactly. The idea here is that we can eventually deprecate --no-map-gw in favour of --map-host-loopback=3Dnone. >=20 > > + " Can be specified zero to two (for IPv4 and IPv6)\n" >=20 > "can" (for consistency, but also because the subject is still the > option, this is not a separate sentence). Done. > ...times. And done. >=20 > > + " default: gateway address, or none if --no-map-gw is also\n" > > + " specified\n" >=20 > I don't think we need to mention here that --no-map-gw implies none, > doing it in the man page is enough. Done. >=20 > > " --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))= ); > > =20 > > 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; > > =20 > > @@ -989,6 +1002,11 @@ static void conf_print(const struct ctx *c) > > } > > =20 > > 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, g= id_t *gid) > > } > > } > > =20 > > +/** > > + * 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") =3D=3D 0) { > > + c->ip4.nat_host_loopback =3D in4addr_any; > > + c->ip6.nat_host_loopback =3D in6addr_any; > > + *no_map_gw =3D 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 =3D (c->mode =3D=3D MODE_PASTA) ? "pasta" : "pass= t"; > > @@ -1400,6 +1448,9 @@ void conf(struct ctx *c, int argc, char **argv) > > netns_only =3D 1; > > *userns =3D 0; > > break; > > + case 21: > > + conf_nat(c, optarg, &no_map_gw); > > + break; > > case 'd': > > c->debug =3D 1; > > c->quiet =3D 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"); > > =20 > > - 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 =3D c->ip4.guest_gw; > > =20 > > - 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 =3D c->ip6.guest_gw; > > =20 > > 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. > > =20 > > +.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 >=20 > 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. Done. > > +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 >=20 > Why "also"? You're describing the default, so I guess this option is > not actually given in that case. Good point, fixed. > > +default. > > + > > .TP > > .BR \-\-no-map-gw > > Don't remap TCP connections and untracked UDP traffic, with the gatewa= y 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=3D"${__opts} -d" > > [ ${TRACE} -eq 1 ] && __opts=3D"${__opts} --trace" > > =20 > > - context_run_bg pasta "./pasta ${__opts} -t 10001,10002,10011,10012 -T= 10003,10013 -u 10001,10002,10011,10012 -U 10003,10013 -P ${STATESETUP}/pas= ta.pid --config-net ${NSTOOL} hold ${STATESETUP}/ns.hold" > > + __nat_host4=3D192.0.2.1 > > + __nat_host6=3D2001:db8:9a55::1 > > + __nat_ns4=3D192.0.2.2 > > + __nat_ns6=3D2001: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}/pas= ta.pid --nat-host-loopback ${__nat_host4} --nat-host-loopback ${__nat_host6= } --config-net ${NSTOOL} hold ${STATESETUP}/ns.hold" > > wait_for [ -f "${STATESETUP}/pasta.pid" ] > > =20 > > 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=3D$((4 * 1024 * 1024= )) --trace-children=3Dyes --vgdb=3Dno --error-exitcode=3D1 --suppressions= =3Dtest/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=3D$((4 * 1024 * 1024= )) --trace-children=3Dyes --vgdb=3Dno --error-exitcode=3D1 --suppressions= =3Dtest/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.so= cket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}= /passt.pid" > > + context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.so= cket -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" ] > > =20 > > 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 >=20 > ...how did this happen? This file already exists. No, it didn't. Previously we reused passt/dhcp for the passt_in_ns tests. With the change to the tests exercising the new option that doesn't work any more, because we need slightly different checks for DHCP to match what we expect when --map-host-loopback is used. --=20 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 --wlS7bTF0u1Jx6BFU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbFT6cACgkQzQJF27ox 2GdMVA/+NpCcY1aQHJCPVasSB9tXe7Snuem62qTmgQgDliDP6CuvgR0YNK54k641 YkDfGBAql5nhWT1DhTGhKsCPf1rOKy/xH/4d1W/sJKvqdaUFBWjAeGucQasFvSwN kII5Yspj11aSAtdvJQkBm45orOoVp8W1+9yZoL32h4pOX7dnNskndyqEmyUtb/U5 EmSkxvSnKGAvBkoVzNxvtmySgfHYanR2f3vBV5Btpv/O0/sYj92Qwo0WuL6eIjx+ mAprAiu4xeDPDu5+K/6/GkBJDdeD3Za1mt2xU5tiYddMBIOsg1I04qp9l02mIAY8 llSInyPHi03fQ8yLq21n05V+57DJDflnZFeJwuDIb6JPsP7Rcoru/RvyuNOdsj9e 4bJUGEO1SSvwLMIBDuN3n/WrVw3KBJCC2n7c7dvoOt1I46pwoSua+Fc6l8sXKE50 SLxoQXo2xUunDpE8LKIGv8Tfj8pEbLxzDjS+ymrPRuMXIHCJ+BrW9oS4oMNenGy7 4oHzDP8r9w9y2QZraA86yBWmE26TuE9Fe3Vo8+m9V6FYDhdh2+B4XCyrdgWdNUkS cZgFV6HhnXrObcRlQPhHP1pT+zLDnJ2EGRdRlosgZwBDj0Gl3PCjTJyWlFu38aHe dYmtAL61zojT9VrO3rx/7tBzqLTiJ9kwRKMIlutWYoTiNkCGqfA= =iGlU -----END PGP SIGNATURE----- --wlS7bTF0u1Jx6BFU--