[PATCH 0/5] RFC: Fix bug 209

[PATCH 0/5] RFC: Fix bug 209

[David Gibson]

Here are a handful of changes to forwarding ant multicast handling,
that fix bug 209.  It also gets us closer to correct multicast
handling, though we're certainly not there yet - multicast things
currently only really work at all because IP_MULTICAST_ALL defaults to
on, and for the common protocols the host is likely to be already a
group member.

RFC, because a tmux change has broken the tests for me, and I wanted
to send this out before I sort out how to fix that.  I have run the
subset of tests covered by "make bats", which includes the podman
tests.

David Gibson (5):
  fwd: Clarify semantics of --host-lo-to-ns-lo
  udp: Validate that we have a unicast source address
  fwd: Rework default address logic for inbound flows
  fwd: Reorder DNAPT and SNAT steps in fwd_nat_from_host()
  fwd: Don't rewrite inbound multicast destinations

 conf.c     |  2 ++
 fwd.c      | 61 ++++++++++++++++++++++++++++++++++--------------------
 passt.1    |  9 ++++----
 udp_flow.c |  2 +-
 4 files changed, 46 insertions(+), 28 deletions(-)

-- 
2.55.0

[PATCH 1/5] fwd: Clarify semantics of --host-lo-to-ns-lo

[David Gibson]

The semantics of --host-lo-to-ns-lo as described in the man page don't
quite make sense: It says without the option forwarded packets will appear
to come _from_ the guest's public address, which is not usually true.
Instead the packets will arrive *to* the guest's public address.  The exact
semantics are also a bit confusing in general.

Rewrite both the man page and code to clarify this.  The new rule is that
it redirects connections addressed to a host loopback address to the same
loopback address in the guest.  This is notionally different from what we
had in two ways:
  * We can now deliver to nonstandard loopback addresses within the guest,
    not just the default one.  This is technically a behavioural change,
    but I think will be less surprising behaviour.
  * The decision is now made on the original _destination_ address, rather
    than source address.  That's different theoreically, but not in
    practice, since loopback packets must have loopback addresses for both
    source and destination.

We make it explicitly incompatible with --no-splice - previously it
was allowed, but would have no effect in that case.

As well as being more precise right now, these semantics will intersect
better with upcoming remapping of target address by forwarding rules.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c  |  2 ++
 fwd.c   | 22 ++++++++++------------
 passt.1 |  9 +++++----
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index 6d83daef..5b6cc2be 100644
--- a/conf.c
+++ b/conf.c
@@ -1796,6 +1796,8 @@ void conf(struct ctx *c, int argc, char **argv)
 		if (c->splice_only)
 			die("--splice-only is for pasta mode only");
 	}
+	if (c->no_splice && c->host_lo_to_ns_lo)
+		die("--host-lo-to-ns-lo is incompatible with --no-splice");
 
 	if (c->mode == MODE_PASTA && !c->pasta_conf_ns) {
 		if (copy_routes_opt)
diff --git a/fwd.c b/fwd.c
index 3ae25fde..84400948 100644
--- a/fwd.c
+++ b/fwd.c
@@ -1038,21 +1038,19 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 		 * In either case, let the kernel pick the source address to
 		 * match.
 		 */
-		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);
+		if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
+			tgt->eaddr = ini->oaddr;
+		else if (inany_v4(&ini->eaddr))
+			tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
+		else
+			tgt->eaddr.a6 = c->ip6.addr_seen;
+
+		/* Let the kernel pick source address and port */
+		if (inany_v4(&tgt->eaddr))
 			tgt->oaddr = inany_any4;
-		} else {
-			if (c->host_lo_to_ns_lo)
-				tgt->eaddr = inany_loopback6;
-			else
-				tgt->eaddr.a6 = c->ip6.addr_seen;
+		else
 			tgt->oaddr = inany_any6;
-		}
 
-		/* Let the kernel pick source port */
 		tgt->oport = 0;
 		if (proto == IPPROTO_UDP)
 			/* But for UDP preserve the source port */
diff --git a/passt.1 b/passt.1
index a8a06311..4f2a8e41 100644
--- a/passt.1
+++ b/passt.1
@@ -672,10 +672,11 @@ Default is \fBauto\fR.
 
 .TP
 .BR \-\-host-lo-to-ns-lo
-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.
+If specified, connections to a host loopback address forwarded with
+\fB\-t\fR or \fB\-u\fR will be delivered to the same loopback address
+on the guest.  Without this option such connections are forwarded to
+the guest's public address.  This option is incompatible with
+\fB--no-splice\fR.
 
 .TP
 .BR \-\-userns " " \fIspec
-- 
2.55.0

[PATCH 2/5] udp: Validate that we have a unicast source address

[David Gibson]

When creating a UDP flow from a socket we, correctly, check that the source
address is unicast - multicast addresses are not valid as a source address.
However, when creating a flow from tap we only check the source address
is specified, not that it is unicast.  Correct this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/udp_flow.c b/udp_flow.c
index 5a59f7f3..f59649f6 100644
--- a/udp_flow.c
+++ b/udp_flow.c
@@ -307,7 +307,7 @@ flow_sidx_t udp_flow_from_tap(const struct ctx *c,
 	ini = flow_initiate_af(flow, PIF_TAP, af, saddr, srcport,
 			       daddr, dstport);
 
-	if (inany_is_unspecified(&ini->eaddr) || ini->eport == 0 ||
+	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0 ||
 	    inany_is_unspecified(&ini->oaddr) || ini->oport == 0) {
 		flow_dbg(flow, "Invalid endpoint on UDP packet");
 		flow_alloc_cancel(flow);
-- 
2.55.0

[PATCH 3/5] fwd: Rework default address logic for inbound flows

[David Gibson]

fwd_nat_from_host() needs to determine the guest side destination address
for the new flow.  In some cases that's controlled by the forwarding rule
or -host-lo-to-ns-lo logic, but by default we use the observed guest
address.  We need to pick the right one to match the source address,
though.

Currently this is done with similar, but not quite identical logic in the
spliced and non-spliced paths.  Introduce a new fwd_default_guest_addr()
helper to make explicit:
 * We have the same logic for splice and tap paths
 * This is a fallback path if nothing else determined the address (we
   use this default nearly all the time now, but it might change in future)
 * We're matching IP family and scope with the guest side source address

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 44 ++++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/fwd.c b/fwd.c
index 84400948..90297ef7 100644
--- a/fwd.c
+++ b/fwd.c
@@ -1005,6 +1005,27 @@ bool nat_inbound(const struct ctx *c, const union inany_addr *addr,
 	return true;
 }
 
+/**
+ * fwd_default_geuest_addr() - Get appropriate guest address to send to
+ * @c:		Execution context
+ * @guest_addr:	Updated with chosen guest address
+ * @template:	Address to match IP version and scope of
+ *
+ * Sets @guest_addr to have the address of the guest, matching the IP version
+ * and scope of @template where possible.
+ */
+static void fwd_default_guest_addr(const struct ctx *c,
+				   union inany_addr *guest_addr,
+				   const union inany_addr *template)
+{
+	if (inany_v4(template))
+		*guest_addr = inany_from_v4(c->ip4.addr_seen);
+	else if (inany_is_linklocal6(template))
+		guest_addr->a6 = c->ip6.addr_ll_seen;
+	else
+		guest_addr->a6 = c->ip6.addr_seen;
+}
+
 /**
  * fwd_nat_from_host() - Determine to forward a flow from the host interface
  * @c:		Execution context
@@ -1040,13 +1061,9 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 		 */
 		if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
 			tgt->eaddr = ini->oaddr;
-		else if (inany_v4(&ini->eaddr))
-			tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
-		else
-			tgt->eaddr.a6 = c->ip6.addr_seen;
 
 		/* Let the kernel pick source address and port */
-		if (inany_v4(&tgt->eaddr))
+		if (inany_v4(&ini->eaddr))
 			tgt->oaddr = inany_any4;
 		else
 			tgt->oaddr = inany_any6;
@@ -1056,6 +1073,9 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 			/* But for UDP preserve the source port */
 			tgt->oport = ini->eport;
 
+		/* Use guest address as destination, if otherwise unspecified */
+		if (inany_is_unspecified(&tgt->eaddr))
+			fwd_default_guest_addr(c, &tgt->eaddr, &tgt->oaddr);
 		return PIF_SPLICE;
 	}
 
@@ -1074,16 +1094,12 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 	}
 	tgt->oport = ini->eport;
 
-	if (!inany_is_unspecified(&rule->taddr)) {
+	if (!inany_is_unspecified(&rule->taddr))
 		tgt->eaddr = rule->taddr;
-	} else if (inany_v4(&tgt->oaddr)) {
-		tgt->eaddr = inany_from_v4(c->ip4.addr_seen);
-	} else {
-		if (inany_is_linklocal6(&tgt->oaddr))
-			tgt->eaddr.a6 = c->ip6.addr_ll_seen;
-		else
-			tgt->eaddr.a6 = c->ip6.addr_seen;
-	}
+
+	/* Use guest address as destination, if otherwise unspecified */
+	if (inany_is_unspecified(&tgt->eaddr))
+		fwd_default_guest_addr(c, &tgt->eaddr, &tgt->oaddr);
 
 	return PIF_TAP;
 }
-- 
2.55.0

[PATCH 4/5] fwd: Reorder DNAPT and SNAT steps in fwd_nat_from_host()

[David Gibson]

The order in which we translate source and destination addresses is a bit
unclear in fwd_nat_from_host().  Reorder things to make it clearer:
   1. Pick guest-side destination address, where options require it
   2. Pick guest-side source address (needs to be different for SPLICE and
      TAP)
   3. If (1) didn't determine destination, pick a fallback to match family
      and scope of source address from (2).

As a small bonus this lets us make step (1) common between SPLICE and TAP
paths.  The value of this is a bit dubious right now, but it will make some
future changes clearer.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fwd.c b/fwd.c
index 90297ef7..7d39898e 100644
--- a/fwd.c
+++ b/fwd.c
@@ -1041,8 +1041,12 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 			  const struct fwd_rule *rule, uint8_t proto,
 			  const struct flowside *ini, struct flowside *tgt)
 {
-	/* Common for spliced and non-spliced cases */
+	/* DNAPT: Common for splice and non-spliced where possible */
 	tgt->eport = rule->to + (ini->oport - rule->first);
+	if (!inany_is_unspecified(&rule->taddr))
+		tgt->eaddr = rule->taddr;
+	else if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
+		tgt->eaddr = ini->oaddr;
 
 	/* TODO: Allow splicing with specified target address */
 	if (!c->no_splice && inany_is_unspecified(&rule->taddr) &&
@@ -1059,10 +1063,8 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 		 * In either case, let the kernel pick the source address to
 		 * match.
 		 */
-		if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
-			tgt->eaddr = ini->oaddr;
 
-		/* Let the kernel pick source address and port */
+		/* SNAT: (implicit) let the kernel pick source addr/port */
 		if (inany_v4(&ini->eaddr))
 			tgt->oaddr = inany_any4;
 		else
@@ -1082,6 +1084,7 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 	if (c->splice_only)
 		return PIF_NONE;
 
+	/* SNAT: translate source address if necessary */
 	if (!nat_inbound(c, &ini->eaddr, &tgt->oaddr)) {
 		if (inany_v4(&ini->eaddr)) {
 			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.our_tap_addr))
@@ -1094,9 +1097,6 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 	}
 	tgt->oport = ini->eport;
 
-	if (!inany_is_unspecified(&rule->taddr))
-		tgt->eaddr = rule->taddr;
-
 	/* Use guest address as destination, if otherwise unspecified */
 	if (inany_is_unspecified(&tgt->eaddr))
 		fwd_default_guest_addr(c, &tgt->eaddr, &tgt->oaddr);
-- 
2.55.0

[PATCH 5/5] fwd: Don't rewrite inbound multicast destinations

[David Gibson]

fwd_nat_from_host() (nearly) always rewrites the destination address for
inbound flows to the observed guest address.  Usually, that makes sense:
regardless of the host address to which the new flow arrived, we want to
direct it to the guest.  However, that clearly does not make sense for
multicast - it should still appear as a multicast transmission to the
guest.

In particular this can work very badly for multicast protocols which use
the same source and destination ports by convention (e.g. mDNS).  In this
case, we will attempt to forword multicast packets to our own socket,
causing a forwarding loop (see bug 209 for more details).

While it's certainly not enough to make us handle multicast correctly in
all circumstances, not translating multicast destinations is closer to
correct, and prevents bug 209 at least.

Link: https://bugs.passt.top/show_bug.cgi?id=209
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fwd.c b/fwd.c
index 7d39898e..e59413b6 100644
--- a/fwd.c
+++ b/fwd.c
@@ -1045,7 +1045,8 @@ uint8_t fwd_nat_from_host(const struct ctx *c,
 	tgt->eport = rule->to + (ini->oport - rule->first);
 	if (!inany_is_unspecified(&rule->taddr))
 		tgt->eaddr = rule->taddr;
-	else if (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr))
+	else if (inany_is_multicast(&ini->oaddr) ||
+		 (c->host_lo_to_ns_lo && inany_is_loopback(&ini->oaddr)))
 		tgt->eaddr = ini->oaddr;
 
 	/* TODO: Allow splicing with specified target address */
-- 
2.55.0