[PATCH v5 00/15] Reduce differences between inbound and outbound socket binding

[PATCH v5 00/15] Reduce differences between inbound and outbound socket binding

[David Gibson]

This series is based on my series fixing bug 176 (regression in auto
forwarding).

The fact that outbound forwarding sockets are bound to the loopback
address, whereas inbound forwarding sockets are (by default) bound to
the unspecified address leads to some unexpected differences between
the paths setting up each of them.

An idea for tackling bug 100 suggested a different approach which will
also reduce some of those differences and allow more code to be shared
between the two paths.  I've since discovered that this approach
doesn't help for bug 100, but I think it's still worthwhile for other reasons.

v5:
 - Combine with SO_BINDTODEVICE and bug 113 patch series
 - Add fallback handling for kernels without SO_BINDTODEVICE
 - Add missing struct field documentation for no_bindtodevice
v4:
 - Add cleanup patch removing unused structure field
 - Rebase, fixing conflicts with Laurent's changes
 - A bunch of spelling and other cosmetic fixes
 - Clarify relation to bug 100 and bug 113
v3:
 - A number of additional fixes covering the handling of IPV6_V6ONLY sockopt
 - Assorted trivial changes
v2:
 - Some rearrangements and rewordings for clarity

David Gibson (15):
  util: Correct error message on SO_BINDTODEVICE failure
  util: Extend sock_probe_mem() to sock_probe_features()
  conf: More useful errors for kernels without SO_BINDTODEVICE
  flow: Remove bogus @path field from flowside_sock_args
  inany: Let length of sockaddr_inany be implicit from the family
  util, flow, pif: Simplify sock_l4_sa() interface
  tcp: Merge tcp_ns_sock_init[46]() into tcp_sock_init_one()
  udp: Unify some more inbound/outbound parts of udp_sock_init()
  udp: Move udp_sock_init() special case to its caller
  util: Fix setting of IPV6_V6ONLY socket option
  tcp, udp: Remove fallback if creating dual stack socket fails
  tcp, udp: Bind outbound listening sockets by interface instead of
    address
  util: Rename sock_l4_dualstack() to sock_l4_dualstack_any()
  tcp: Always populate oaddr field for socket initiated flows
  fwd: Preserve non-standard loopback address when splice forwarding

 conf.c       |  16 ++++-
 flow.c       |  22 +++----
 fwd.c        |   4 +-
 icmp.c       |   3 +-
 inany.h      |  17 ++++++
 passt.c      |   2 +-
 passt.h      |   2 +
 pif.c        |  27 ++-------
 pif.h        |   2 +-
 tcp.c        | 162 +++++++++++++++++++++------------------------------
 tcp.h        |   5 +-
 tcp_splice.c |   5 +-
 udp.c        | 122 +++++++++++++++++++++-----------------
 udp.h        |   5 +-
 util.c       | 102 +++++++++++++++++++++++++++-----
 util.h       |  10 ++--
 16 files changed, 289 insertions(+), 217 deletions(-)

-- 
2.52.0

[PATCH v5 01/15] util: Correct error message on SO_BINDTODEVICE failure

[David Gibson]

The warning message we print of SO_BINDTODEVICE fails is incorrect: we
include EPOLL_TYPE_STR(proto), but the proto variable is not an epoll type
so this will generate misleading garbage.  This has been wrong ever since
340164445 but the message is rare enough that we never noticed.

Correct that, and reword the message a bit for clarity while we're
there.

Fixes: 340164445341 ("epoll: Generalize epoll_ref to cover things other than sockets")

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

diff --git a/util.c b/util.c
index ab23463b..347f34f5 100644
--- a/util.c
+++ b/util.c
@@ -126,9 +126,10 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 			char str[SOCKADDR_STRLEN];
 
 			ret = -errno;
-			warn("Can't bind %s socket for %s to %s, closing",
-			     EPOLL_TYPE_STR(proto),
-			     sockaddr_ntop(sa, str, sizeof(str)), ifname);
+			warn("SO_BINDTODEVICE %s failed for %s on %s: %s",
+			     ifname, EPOLL_TYPE_STR(type),
+			     sockaddr_ntop(sa, str, sizeof(str)),
+			     strerror_(-ret));
 			close(fd);
 			return ret;
 		}
-- 
2.52.0

[PATCH v5 02/15] util: Extend sock_probe_mem() to sock_probe_features()

[David Gibson]

sock_probe_mem() currently checks whether we're able to allocate large
socket buffers.  Extend it to also check whether the SO_BINDTODEVICE
socket option is available.  Rename to sock_probe_features() to reflect the
new functionality.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c |  2 +-
 passt.h |  2 ++
 util.c  | 19 +++++++++++++++++--
 util.h  |  2 +-
 4 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/passt.c b/passt.c
index 4964427d..0b84ac6c 100644
--- a/passt.c
+++ b/passt.c
@@ -381,7 +381,7 @@ int main(int argc, char **argv)
 	if (setrlimit(RLIMIT_NOFILE, &limit))
 		die_perror("Failed to set current limit for open files");
 
-	sock_probe_mem(&c);
+	sock_probe_features(&c);
 
 	conf(&c, argc, argv);
 	trace_init(c.trace);
diff --git a/passt.h b/passt.h
index 15801b44..79d01ddb 100644
--- a/passt.h
+++ b/passt.h
@@ -204,6 +204,7 @@ struct ip6_ctx {
  * @freebind:		Allow binding of non-local addresses for forwarding
  * @low_wmem:		Low probed net.core.wmem_max
  * @low_rmem:		Low probed net.core.rmem_max
+ * @no_bindtodevice:	Unprivileged SO_BINDTODEVICE not available
  * @vdev:		vhost-user device
  * @device_state_fd:	Device state migration channel
  * @device_state_result: Device state migration result
@@ -281,6 +282,7 @@ struct ctx {
 
 	int low_wmem;
 	int low_rmem;
+	int no_bindtodevice;
 
 	struct vu_dev *vdev;
 
diff --git a/util.c b/util.c
index 347f34f5..bad38129 100644
--- a/util.c
+++ b/util.c
@@ -233,12 +233,13 @@ int sock_unix(char *sock_path)
 }
 
 /**
- * sock_probe_mem() - Check if setting high SO_SNDBUF and SO_RCVBUF is allowed
+ * sock_probe_features() - Probe for socket features we might use
  * @c:		Execution context
  */
-void sock_probe_mem(struct ctx *c)
+void sock_probe_features(struct ctx *c)
 {
 	int v = INT_MAX / 2, s;
+	const char lo[] = "lo";
 	socklen_t sl;
 
 	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
@@ -247,6 +248,7 @@ void sock_probe_mem(struct ctx *c)
 		return;
 	}
 
+	/* Check if setting high SO_SNDBUF and SO_RCVBUF is allowed */
 	sl = sizeof(v);
 	if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v))	||
 	    getsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, &sl) ||
@@ -259,6 +261,19 @@ void sock_probe_mem(struct ctx *c)
 	    (size_t)v < RCVBUF_BIG)
 		c->low_rmem = 1;
 
+	/* Check if SO_BINDTODEVICE is available
+	 *
+	 * Supported since kernel version 5.7, commit c427bfec18f2 ("net: core:
+	 * enable SO_BINDTODEVICE for non-root users").  Some distro kernels may
+	 * have backports, of course.  Record whether we can use it so that we
+	 * can give more useful diagnostics.
+	 */
+	if (setsockopt(s, SOL_SOCKET, SO_BINDTODEVICE, lo, sizeof(lo)-1)) {
+		if (errno != EPERM)
+			warn_perror("Unexpected error probing SO_BINDTODEVICE");
+		c->no_bindtodevice = 1;
+	}
+
 	close(s);
 }
 
diff --git a/util.h b/util.h
index a0b2ada6..200b6408 100644
--- a/util.h
+++ b/util.h
@@ -211,7 +211,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 	       const void *sa, socklen_t sl,
 	       const char *ifname, bool v6only);
 int sock_unix(char *sock_path);
-void sock_probe_mem(struct ctx *c);
+void sock_probe_features(struct ctx *c);
 long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
 int64_t timespec_diff_us(const struct timespec *a, const struct timespec *b);
 void bitmap_set(uint8_t *map, unsigned bit);
-- 
2.52.0

[PATCH v5 03/15] conf: More useful errors for kernels without SO_BINDTODEVICE

[David Gibson]

Before 5.7, the kernel didn't allow SO_BINDTODEVICE to be called
unprivileged.  That means for earlier kernels, we can't implement binding
listening sockets to a specific interface (e.g. -t %eth0/80).

Currently we'll generate an error on this at the point we actually attempt
the SO_BINDTODEVICE setsockopt(), at which point the connection to the
command line option might not be entirely clear.

Use the fact we now probe for SO_BINDTODEVICE support to make a clearer
error message at the time we parse the forwarding option.

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

diff --git a/conf.c b/conf.c
index 66b9e634..43bcaf31 100644
--- a/conf.c
+++ b/conf.c
@@ -156,6 +156,12 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		    optname, optarg);
 	}
 
+	if (ifname && c->no_bindtodevice) {
+		die(
+"Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
+		    optname, optarg);
+	}
+
 	for (i = first; i <= last; i++) {
 		if (bitmap_isset(exclude, i))
 			continue;
-- 
2.52.0

[PATCH v5 04/15] flow: Remove bogus @path field from flowside_sock_args

[David Gibson]

This was there when the structure was created, but never used.  Looks like
a copy-pasta error.

Fixes: 781164e25bdf ("flow: Helper to create sockets based on flowside")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/flow.c b/flow.c
index 278a9cf0..743860d6 100644
--- a/flow.c
+++ b/flow.c
@@ -172,7 +172,6 @@ struct flowside_sock_args {
 	enum epoll_type type;
 	const struct sockaddr *sa;
 	socklen_t sl;
-	const char *path;
 };
 
 /** flowside_sock_splice() - Create and bind socket for PIF_SPLICE based on flowside
-- 
2.52.0

[PATCH v5 05/15] inany: Let length of sockaddr_inany be implicit from the family

[David Gibson]

sockaddr_inany can contain either an IPv4 or IPv6 socket address, so the
relevant length for bind() or connect() can vary.  In pif_sockaddr() we
return that length, and in sock_l4_sa() we take it as an extra parameter.

However, sockaddr_inany always contains exactly a sockaddr_in or a
sockaddr_in6 each with a fixed size.  Therefore we can derive the relevant
length from the family, and don't need to pass it around separately.

Make a tiny helper to get the relevant address length, and update all
interfaces to use that approach instead.

In the process, fix a buglet in tcp_flow_repair_bind(): we passed
sizeof(union sockaddr_inany) to bind() instead of the specific length for
the address family.  Since the sizeof() is always longer than the specific
length, this is probably fine, but not theoretically correct.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c       | 21 ++++++++-------------
 icmp.c       |  3 ++-
 inany.h      | 17 +++++++++++++++++
 pif.c        | 15 +++++----------
 pif.h        |  2 +-
 tcp.c        | 20 +++++++++-----------
 tcp_splice.c |  5 ++---
 udp.c        |  8 +++-----
 util.c       |  8 +++-----
 util.h       |  4 ++--
 10 files changed, 52 insertions(+), 51 deletions(-)

diff --git a/flow.c b/flow.c
index 743860d6..11e3f283 100644
--- a/flow.c
+++ b/flow.c
@@ -163,15 +163,13 @@ static void flowside_from_af(struct flowside *side, sa_family_t af,
  * @err:	Filled in with errno if something failed
  * @type:	Socket epoll type
  * @sa:		Socket address
- * @sl:		Length of @sa
  */
 struct flowside_sock_args {
 	const struct ctx *c;
 	int fd;
 	int err;
 	enum epoll_type type;
-	const struct sockaddr *sa;
-	socklen_t sl;
+	const union sockaddr_inany *sa;
 };
 
 /** flowside_sock_splice() - Create and bind socket for PIF_SPLICE based on flowside
@@ -185,8 +183,8 @@ static int flowside_sock_splice(void *arg)
 
 	ns_enter(a->c);
 
-	a->fd = sock_l4_sa(a->c, a->type, a->sa, a->sl, NULL,
-	                   a->sa->sa_family == AF_INET6);
+	a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL,
+			   a->sa->sa_family == AF_INET6);
 	a->err = errno;
 
 	return 0;
@@ -207,11 +205,10 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 {
 	const char *ifname = NULL;
 	union sockaddr_inany sa;
-	socklen_t sl;
 
 	ASSERT(pif_is_socket(pif));
 
-	pif_sockaddr(c, &sa, &sl, pif, &tgt->oaddr, tgt->oport);
+	pif_sockaddr(c, &sa, pif, &tgt->oaddr, tgt->oport);
 
 	switch (pif) {
 	case PIF_HOST:
@@ -222,13 +219,12 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 		else if (sa.sa_family == AF_INET6)
 			ifname = c->ip6.ifname_out;
 
-		return sock_l4_sa(c, type, &sa, sl, ifname,
+		return sock_l4_sa(c, type, &sa, ifname,
 				  sa.sa_family == AF_INET6);
 
 	case PIF_SPLICE: {
 		struct flowside_sock_args args = {
-			.c = c, .type = type,
-			.sa = &sa.sa, .sl = sl,
+			.c = c, .type = type, .sa = &sa,
 		};
 		NS_CALL(flowside_sock_splice, &args);
 		errno = args.err;
@@ -257,10 +253,9 @@ int flowside_connect(const struct ctx *c, int s,
 		     uint8_t pif, const struct flowside *tgt)
 {
 	union sockaddr_inany sa;
-	socklen_t sl;
 
-	pif_sockaddr(c, &sa, &sl, pif, &tgt->eaddr, tgt->eport);
-	return connect(s, &sa.sa, sl);
+	pif_sockaddr(c, &sa, pif, &tgt->eaddr, tgt->eport);
+	return connect(s, &sa.sa, socklen_inany(&sa));
 }
 
 /** flow_log_ - Log flow-related message
diff --git a/icmp.c b/icmp.c
index 35faefb9..9564c496 100644
--- a/icmp.c
+++ b/icmp.c
@@ -312,8 +312,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 	ASSERT(flow_proto[pingf->f.type] == proto);
 	pingf->ts = now->tv_sec;
 
-	pif_sockaddr(c, &sa, &msh.msg_namelen, PIF_HOST, &tgt->eaddr, 0);
+	pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, 0);
 	msh.msg_name = &sa;
+	msh.msg_namelen = socklen_inany(&sa);
 	msh.msg_iov = iov;
 	msh.msg_iovlen = cnt;
 	msh.msg_control = NULL;
diff --git a/inany.h b/inany.h
index 7ca5cbd3..61b36fb4 100644
--- a/inany.h
+++ b/inany.h
@@ -67,6 +67,23 @@ union sockaddr_inany {
 	struct sockaddr_in6	sa6;
 };
 
+/** socklen_inany() - Get relevant address length for sockaddr_inany address
+ * @sa:		sockaddr_inany socket address
+ *
+ * Return: socket address length for bind() or connect(), from IP family in @sa
+ */
+static inline socklen_t socklen_inany(const union sockaddr_inany *sa)
+{
+	switch (sa->sa_family) {
+	case AF_INET:
+		return sizeof(sa->sa4);
+	case AF_INET6:
+		return sizeof(sa->sa6);
+	default:
+		ASSERT(0);
+	}
+}
+
 /** inany_v4 - Extract IPv4 address, if present, from IPv[46] address
  * @addr:	IPv4 or IPv6 address
  *
diff --git a/pif.c b/pif.c
index 331942e7..e6a50806 100644
--- a/pif.c
+++ b/pif.c
@@ -30,12 +30,11 @@ static_assert(ARRAY_SIZE(pif_type_str) == PIF_NUM_TYPES,
 /** pif_sockaddr() - Construct a socket address suitable for an interface
  * @c:		Execution context
  * @sa:		Pointer to sockaddr to fill in
- * @sl:		Updated to relevant length of initialised @sa
  * @pif:	Interface to create the socket address
  * @addr:	IPv[46] address
  * @port:	Port (host byte order)
  */
-void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl,
+void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
 		  uint8_t pif, const union inany_addr *addr, in_port_t port)
 {
 	const struct in_addr *v4 = inany_v4(addr);
@@ -47,7 +46,6 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl,
 		sa->sa4.sin_addr = *v4;
 		sa->sa4.sin_port = htons(port);
 		memset(&sa->sa4.sin_zero, 0, sizeof(sa->sa4.sin_zero));
-		*sl = sizeof(sa->sa4);
 	} else {
 		sa->sa_family = AF_INET6;
 		sa->sa6.sin6_addr = addr->a6;
@@ -57,7 +55,6 @@ void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl,
 		else
 			sa->sa6.sin6_scope_id = 0;
 		sa->sa6.sin6_flowinfo = 0;
-		*sl = sizeof(sa->sa6);
 	}
 }
 
@@ -85,7 +82,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 		.sa6.sin6_port = htons(port),
 	};
 	union epoll_ref ref;
-	socklen_t sl;
 	int ret;
 
 	ASSERT(pif_is_socket(pif));
@@ -97,12 +93,11 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 	}
 
 	if (!addr) {
-		ref.fd = sock_l4_sa(c, type, &sa, sizeof(sa.sa6),
-				    ifname, false);
+		ref.fd = sock_l4_sa(c, type, &sa, ifname, false);
 	} else {
-		pif_sockaddr(c, &sa, &sl, pif, addr, port);
-		ref.fd = sock_l4_sa(c, type, &sa, sl,
-				    ifname, sa.sa_family == AF_INET6);
+		pif_sockaddr(c, &sa, pif, addr, port);
+		ref.fd = sock_l4_sa(c, type, &sa, ifname,
+				    sa.sa_family == AF_INET6);
 	}
 
 	if (ref.fd < 0)
diff --git a/pif.h b/pif.h
index f029282b..0f7f6672 100644
--- a/pif.h
+++ b/pif.h
@@ -57,7 +57,7 @@ static inline bool pif_is_socket(uint8_t pif)
 	return pif == PIF_HOST || pif == PIF_SPLICE;
 }
 
-void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa, socklen_t *sl,
+void pif_sockaddr(const struct ctx *c, union sockaddr_inany *sa,
 		  uint8_t pif, const union inany_addr *addr, in_port_t port);
 int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 		const union inany_addr *addr, const char *ifname,
diff --git a/tcp.c b/tcp.c
index 3202d338..b8a98523 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1457,12 +1457,11 @@ static void tcp_bind_outbound(const struct ctx *c,
 {
 	const struct flowside *tgt = &conn->f.side[TGTSIDE];
 	union sockaddr_inany bind_sa;
-	socklen_t sl;
 
 
-	pif_sockaddr(c, &bind_sa, &sl, PIF_HOST, &tgt->oaddr, tgt->oport);
+	pif_sockaddr(c, &bind_sa, PIF_HOST, &tgt->oaddr, tgt->oport);
 	if (!inany_is_unspecified(&tgt->oaddr) || tgt->oport) {
-		if (bind(s, &bind_sa.sa, sl)) {
+		if (bind(s, &bind_sa.sa, socklen_inany(&bind_sa))) {
 			char sstr[INANY_ADDRSTRLEN];
 
 			flow_dbg_perror(conn,
@@ -1522,7 +1521,6 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	union flow *flow;
 	int s = -1, mss;
 	uint64_t hash;
-	socklen_t sl;
 
 	if (!(flow = flow_alloc()))
 		return;
@@ -1555,7 +1553,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	if ((s = tcp_conn_sock(af)) < 0)
 		goto cancel;
 
-	pif_sockaddr(c, &sa, &sl, PIF_HOST, &tgt->eaddr, tgt->eport);
+	pif_sockaddr(c, &sa, PIF_HOST, &tgt->eaddr, tgt->eport);
 
 	/* Use bind() to check if the target address is local (EADDRINUSE or
 	 * similar) and already bound, and set the LOCAL flag in that case.
@@ -1567,7 +1565,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	 *
 	 * So, if bind() succeeds, close the socket, get a new one, and proceed.
 	 */
-	if (bind(s, &sa.sa, sl)) {
+	if (bind(s, &sa.sa, socklen_inany(&sa))) {
 		if (errno != EADDRNOTAVAIL && errno != EACCES)
 			conn_flag(c, conn, LOCAL);
 	} else {
@@ -1607,7 +1605,7 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 
 	tcp_bind_outbound(c, conn, s);
 
-	if (connect(s, &sa.sa, sl)) {
+	if (connect(s, &sa.sa, socklen_inany(&sa))) {
 		if (errno != EINPROGRESS) {
 			tcp_rst(c, conn);
 			goto cancel;
@@ -1626,7 +1624,8 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af,
 	tcp_epoll_ctl(c, conn);
 
 	if (c->mode == MODE_VU) { /* To rebind to same oport after migration */
-		sl = sizeof(sa);
+		socklen_t sl = sizeof(sa);
+
 		if (getsockname(s, &sa.sa, &sl) ||
 		    inany_from_sockaddr(&tgt->oaddr, &tgt->oport, &sa) < 0)
 			err_perror("Can't get local address for socket %i", s);
@@ -3611,11 +3610,10 @@ static int tcp_flow_repair_bind(const struct ctx *c, struct tcp_tap_conn *conn)
 {
 	const struct flowside *sockside = HOSTFLOW(conn);
 	union sockaddr_inany a;
-	socklen_t sl;
 
-	pif_sockaddr(c, &a, &sl, PIF_HOST, &sockside->oaddr, sockside->oport);
+	pif_sockaddr(c, &a, PIF_HOST, &sockside->oaddr, sockside->oport);
 
-	if (bind(conn->sock, &a.sa, sizeof(a))) {
+	if (bind(conn->sock, &a.sa, socklen_inany(&a))) {
 		int rc = -errno;
 		flow_perror(conn, "Failed to bind socket for migrated flow");
 		return rc;
diff --git a/tcp_splice.c b/tcp_splice.c
index 5efd8597..717766af 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -352,7 +352,6 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn)
 	sa_family_t af = inany_v4(&tgt->eaddr) ? AF_INET : AF_INET6;
 	uint8_t tgtpif = conn->f.pif[TGTSIDE];
 	union sockaddr_inany sa;
-	socklen_t sl;
 	int one = 1;
 
 	if (tgtpif == PIF_HOST)
@@ -380,9 +379,9 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn)
 			   conn->s[1]);
 	}
 
-	pif_sockaddr(c, &sa, &sl, tgtpif, &tgt->eaddr, tgt->eport);
+	pif_sockaddr(c, &sa, tgtpif, &tgt->eaddr, tgt->eport);
 
-	if (connect(conn->s[1], &sa.sa, sl)) {
+	if (connect(conn->s[1], &sa.sa, socklen_inany(&sa))) {
 		if (errno != EINPROGRESS) {
 			flow_trace(conn, "Couldn't connect socket for splice: %s",
 				   strerror_(errno));
diff --git a/udp.c b/udp.c
index 9c009502..d414ca32 100644
--- a/udp.c
+++ b/udp.c
@@ -780,7 +780,6 @@ static void udp_sock_to_sock(const struct ctx *c, int from_s, int n,
 	const struct udp_flow *uflow = udp_at_sidx(tosidx);
 	uint8_t topif = pif_at_sidx(tosidx);
 	int to_s = uflow->s[tosidx.sidei];
-	socklen_t sl;
 	int i;
 
 	if ((n = udp_sock_recv(c, from_s, udp_mh_recv, n)) <= 0)
@@ -791,7 +790,7 @@ static void udp_sock_to_sock(const struct ctx *c, int from_s, int n,
 			= udp_mh_recv[i].msg_len;
 	}
 
-	pif_sockaddr(c, &udp_splice_to, &sl, topif,
+	pif_sockaddr(c, &udp_splice_to, topif,
 		     &toside->eaddr, toside->eport);
 
 	sendmmsg(to_s, udp_mh_splice, n, MSG_NOSIGNAL);
@@ -999,7 +998,6 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 	flow_sidx_t tosidx;
 	in_port_t src, dst;
 	uint8_t topif;
-	socklen_t sl;
 
 	ASSERT(!c->no_udp);
 
@@ -1041,7 +1039,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 	s = uflow->s[tosidx.sidei];
 	ASSERT(s >= 0);
 
-	pif_sockaddr(c, &to_sa, &sl, topif, &toside->eaddr, toside->eport);
+	pif_sockaddr(c, &to_sa, topif, &toside->eaddr, toside->eport);
 
 	for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) {
 		const struct udphdr *uh_send;
@@ -1054,7 +1052,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 			return p->count - idx;
 
 		mm[i].msg_hdr.msg_name = &to_sa;
-		mm[i].msg_hdr.msg_namelen = sl;
+		mm[i].msg_hdr.msg_namelen = socklen_inany(&to_sa);
 
 		if (data.cnt) {
 			int cnt;
diff --git a/util.c b/util.c
index bad38129..3dd1654f 100644
--- a/util.c
+++ b/util.c
@@ -44,17 +44,15 @@
  * @c:		Execution context
  * @type:	epoll type
  * @sa:		Socket address to bind to
- * @sl:		Length of @sa
  * @ifname:	Interface for binding, NULL for any
  * @v6only:	Set IPV6_V6ONLY socket option
  *
  * Return: newly created socket, negative error code on failure
  */
 int sock_l4_sa(const struct ctx *c, enum epoll_type type,
-	       const void *sa, socklen_t sl,
-	       const char *ifname, bool v6only)
+	       const union sockaddr_inany *sa, const char *ifname, bool v6only)
 {
-	sa_family_t af = ((const struct sockaddr *)sa)->sa_family;
+	sa_family_t af = sa->sa_family;
 	bool freebind = false;
 	int fd, y = 1, ret;
 	uint8_t proto;
@@ -147,7 +145,7 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 		}
 	}
 
-	if (bind(fd, sa, sl) < 0) {
+	if (bind(fd, &sa->sa, socklen_inany(sa)) < 0) {
 		/* We'll fail to bind to low ports if we don't have enough
 		 * capabilities, and we'll fail to bind on already bound ports,
 		 * this is fine. This might also fail for ICMP because of a
diff --git a/util.h b/util.h
index 200b6408..fb82ad30 100644
--- a/util.h
+++ b/util.h
@@ -206,10 +206,10 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 #include "packet.h"
 
 struct ctx;
+union sockaddr_inany;
 
 int sock_l4_sa(const struct ctx *c, enum epoll_type type,
-	       const void *sa, socklen_t sl,
-	       const char *ifname, bool v6only);
+	       const union sockaddr_inany *sa, const char *ifname, bool v6only);
 int sock_unix(char *sock_path);
 void sock_probe_features(struct ctx *c);
 long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
-- 
2.52.0

[PATCH v5 06/15] util, flow, pif: Simplify sock_l4_sa() interface

[David Gibson]

sock_l4_sa() has a somewhat confusing 'v6only' option controlling whether
to set the IPV6_V6ONLY socket option.  Usually it's set when the given
address is IPv6, but not when we want to create a dual stack listening
socket.  The latter only makes sense when the address is :: however.

Clarify this by only keeping the v6only option in an internal helper
sock_l4_().  External users will call either sock_l4() which always creates
a socket bound to a specific IP version, or sock_l4_dualstack() which
creates a dual stack socket, but takes only a port not an address.

We drop the '_sa' suffix while we're at it - it exists because this used
to be an internal version with a sock_l4() wrapper.  The wrapper no longer
exists so the '_sa' is no longer useful.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 flow.c |  6 ++----
 pif.c  | 12 ++++--------
 util.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 util.h |  6 ++++--
 4 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/flow.c b/flow.c
index 11e3f283..8d72965c 100644
--- a/flow.c
+++ b/flow.c
@@ -183,8 +183,7 @@ static int flowside_sock_splice(void *arg)
 
 	ns_enter(a->c);
 
-	a->fd = sock_l4_sa(a->c, a->type, a->sa, NULL,
-			   a->sa->sa_family == AF_INET6);
+	a->fd = sock_l4(a->c, a->type, a->sa, NULL);
 	a->err = errno;
 
 	return 0;
@@ -219,8 +218,7 @@ int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 		else if (sa.sa_family == AF_INET6)
 			ifname = c->ip6.ifname_out;
 
-		return sock_l4_sa(c, type, &sa, ifname,
-				  sa.sa_family == AF_INET6);
+		return sock_l4(c, type, &sa, ifname);
 
 	case PIF_SPLICE: {
 		struct flowside_sock_args args = {
diff --git a/pif.c b/pif.c
index e6a50806..85904f35 100644
--- a/pif.c
+++ b/pif.c
@@ -76,11 +76,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 		const union inany_addr *addr, const char *ifname,
 		in_port_t port, uint32_t data)
 {
-	union sockaddr_inany sa = {
-		.sa6.sin6_family = AF_INET6,
-		.sa6.sin6_addr = in6addr_any,
-		.sa6.sin6_port = htons(port),
-	};
 	union epoll_ref ref;
 	int ret;
 
@@ -93,11 +88,12 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 	}
 
 	if (!addr) {
-		ref.fd = sock_l4_sa(c, type, &sa, ifname, false);
+		ref.fd = sock_l4_dualstack(c, type, port, ifname);
 	} else {
+		union sockaddr_inany sa;
+
 		pif_sockaddr(c, &sa, pif, addr, port);
-		ref.fd = sock_l4_sa(c, type, &sa, ifname,
-				    sa.sa_family == AF_INET6);
+		ref.fd = sock_l4(c, type, &sa, ifname);
 	}
 
 	if (ref.fd < 0)
diff --git a/util.c b/util.c
index 3dd1654f..ee369614 100644
--- a/util.c
+++ b/util.c
@@ -40,7 +40,7 @@
 #endif
 
 /**
- * sock_l4_sa() - Create and bind socket to socket address, add to epoll list
+ * sock_l4_() - Create and bind socket to socket address
  * @c:		Execution context
  * @type:	epoll type
  * @sa:		Socket address to bind to
@@ -49,8 +49,9 @@
  *
  * Return: newly created socket, negative error code on failure
  */
-int sock_l4_sa(const struct ctx *c, enum epoll_type type,
-	       const union sockaddr_inany *sa, const char *ifname, bool v6only)
+static int sock_l4_(const struct ctx *c, enum epoll_type type,
+		    const union sockaddr_inany *sa, const char *ifname,
+		    bool v6only)
 {
 	sa_family_t af = sa->sa_family;
 	bool freebind = false;
@@ -168,6 +169,44 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type type,
 	return fd;
 }
 
+/**
+ * sock_l4() - Create and bind socket to given address
+ * @c:		Execution context
+ * @type:	epoll type
+ * @sa:		Socket address to bind to
+ * @ifname:	Interface for binding, NULL for any
+ *
+ * Return: newly created socket, negative error code on failure
+ */
+int sock_l4(const struct ctx *c, enum epoll_type type,
+	    const union sockaddr_inany *sa, const char *ifname)
+{
+	return sock_l4_(c, type, sa, ifname, sa->sa_family == AF_INET6);
+}
+
+/**
+ * sock_l4_dualstack() - Create a dual stack socket bound with wildcard address
+ * @c:		Execution context
+ * @type:	epoll type
+ * @port	Port to bind to (:: and 0.0.0.0)
+ * @ifname:	Interface for binding, NULL for any
+ *
+ * Return: newly created socket, negative error code on failure
+ *
+ * A dual stack socket is effectively bound to both :: and 0.0.0.0.
+ */
+int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
+		      in_port_t port, const char *ifname)
+{
+	union sockaddr_inany sa = {
+		.sa6.sin6_family = AF_INET6,
+		.sa6.sin6_addr = in6addr_any,
+		.sa6.sin6_port = htons(port),
+	};
+
+	return sock_l4_(c, type, &sa, ifname, 0);
+}
+
 /**
  * sock_unix() - Create and bind AF_UNIX socket
  * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
diff --git a/util.h b/util.h
index fb82ad30..ebc14138 100644
--- a/util.h
+++ b/util.h
@@ -208,8 +208,10 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 struct ctx;
 union sockaddr_inany;
 
-int sock_l4_sa(const struct ctx *c, enum epoll_type type,
-	       const union sockaddr_inany *sa, const char *ifname, bool v6only);
+int sock_l4(const struct ctx *c, enum epoll_type type,
+	    const union sockaddr_inany *sa, const char *ifname);
+int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
+		      in_port_t port, const char *ifname);
 int sock_unix(char *sock_path);
 void sock_probe_features(struct ctx *c);
 long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
-- 
2.52.0

[PATCH v5 07/15] tcp: Merge tcp_ns_sock_init[46]() into tcp_sock_init_one()

[David Gibson]

Surprisingly little logic is shared between the path for creating a
listen()ing socket in the guest namespace versus in the host namespace.
Improve this, by extending tcp_sock_init_one() to take a pif parameter
indicating where it should open the socket.  This allows
tcp_ns_sock_init[46]() to be removed entirely.

We generalise tcp_sock_init() in the same way, although we don't fully use
it yet, due to some subtle differences in how we bind for -t versus -T.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c |   2 +-
 tcp.c  | 100 +++++++++++++++++++--------------------------------------
 tcp.h  |   5 +--
 3 files changed, 37 insertions(+), 70 deletions(-)

diff --git a/conf.c b/conf.c
index 43bcaf31..04fbc67b 100644
--- a/conf.c
+++ b/conf.c
@@ -175,7 +175,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		fwd->delta[i] = to - first;
 
 		if (optname == 't')
-			ret = tcp_sock_init(c, addr, ifname, i);
+			ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i);
 		else if (optname == 'u')
 			ret = udp_sock_init(c, 0, addr, ifname, i);
 		else
diff --git a/tcp.c b/tcp.c
index b8a98523..428bac7b 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2530,29 +2530,42 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
 /**
  * tcp_sock_init_one() - Initialise listening socket for address and port
  * @c:		Execution context
+ * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
  * @addr:	Pointer to address for binding, NULL for dual stack any
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
  * Return: fd for the new listening socket, negative error code on failure
+ *
+ * If pif == PIF_SPLICE, the caller must have already entered the guest ns.
  */
-static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr,
-			     const char *ifname, in_port_t port)
+static int tcp_sock_init_one(const struct ctx *c, uint8_t pif,
+			     const union inany_addr *addr, const char *ifname,
+			     in_port_t port)
 {
 	union tcp_listen_epoll_ref tref = {
 		.port = port,
-		.pif = PIF_HOST,
+		.pif = pif,
 	};
+	const struct fwd_ports *fwd;
 	int s;
 
-	s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_HOST, addr,
-				ifname, port, tref.u32);
+	if (pif == PIF_HOST)
+		fwd = &c->tcp.fwd_in;
+	else
+		fwd = &c->tcp.fwd_out;
+
+	s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, pif, addr, ifname,
+			port, tref.u32);
+
+	if (fwd->mode == FWD_AUTO) {
+		int (*socks)[IP_VERSIONS] = pif == PIF_SPLICE ?
+			tcp_sock_ns : tcp_sock_init_ext;
 
-	if (c->tcp.fwd_in.mode == FWD_AUTO) {
 		if (!addr || inany_v4(addr))
-			tcp_sock_init_ext[port][V4] = s < 0 ? -1 : s;
+			socks[port][V4] = s < 0 ? -1 : s;
 		if (!addr || !inany_v4(addr))
-			tcp_sock_init_ext[port][V6] = s < 0 ? -1 : s;
+			socks[port][V6] = s < 0 ? -1 : s;
 	}
 
 	if (s < 0)
@@ -2564,14 +2577,16 @@ static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr,
 /**
  * tcp_sock_init() - Create listening sockets for a given host ("inbound") port
  * @c:		Execution context
+ * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
  * @addr:	Pointer to address for binding, NULL if not configured
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
  * Return: 0 on (partial) success, negative error code on (complete) failure
  */
-int tcp_sock_init(const struct ctx *c, const union inany_addr *addr,
-		  const char *ifname, in_port_t port)
+int tcp_sock_init(const struct ctx *c, uint8_t pif,
+		  const union inany_addr *addr, const char *ifname,
+		  in_port_t port)
 {
 	int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
 
@@ -2579,72 +2594,23 @@ int tcp_sock_init(const struct ctx *c, const union inany_addr *addr,
 
 	if (!addr && c->ifi4 && c->ifi6)
 		/* Attempt to get a dual stack socket */
-		if (tcp_sock_init_one(c, NULL, ifname, port) >= 0)
+		if (tcp_sock_init_one(c, pif, NULL, ifname, port) >= 0)
 			return 0;
 
 	/* Otherwise create a socket per IP version */
 	if ((!addr || inany_v4(addr)) && c->ifi4)
-		r4 = tcp_sock_init_one(c, addr ? addr : &inany_any4,
-				       ifname, port);
+		r4 = tcp_sock_init_one(c, pif,
+				       addr ? addr : &inany_any4, ifname, port);
 
 	if ((!addr || !inany_v4(addr)) && c->ifi6)
-		r6 = tcp_sock_init_one(c, addr ? addr : &inany_any6,
-				       ifname, port);
+		r6 = tcp_sock_init_one(c, pif,
+				       addr ? addr : &inany_any6, ifname, port);
 
 	if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6))
 		return 0;
 
 	return r4 < 0 ? r4 : r6;
 }
-
-/**
- * tcp_ns_sock_init4() - Init socket to listen for outbound IPv4 connections
- * @c:		Execution context
- * @port:	Port, host order
- */
-static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
-{
-	union tcp_listen_epoll_ref tref = {
-		.port = port,
-		.pif = PIF_SPLICE,
-	};
-	int s;
-
-	ASSERT(c->mode == MODE_PASTA);
-
-	s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_SPLICE, &inany_loopback4,
-			NULL, port, tref.u32);
-	if (s < 0)
-		s = -1;
-
-	if (c->tcp.fwd_out.mode == FWD_AUTO)
-		tcp_sock_ns[port][V4] = s;
-}
-
-/**
- * tcp_ns_sock_init6() - Init socket to listen for outbound IPv6 connections
- * @c:		Execution context
- * @port:	Port, host order
- */
-static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port)
-{
-	union tcp_listen_epoll_ref tref = {
-		.port = port,
-		.pif = PIF_SPLICE,
-	};
-	int s;
-
-	ASSERT(c->mode == MODE_PASTA);
-
-	s = pif_sock_l4(c, EPOLL_TYPE_TCP_LISTEN, PIF_SPLICE, &inany_loopback6,
-			NULL, port, tref.u32);
-	if (s < 0)
-		s = -1;
-
-	if (c->tcp.fwd_out.mode == FWD_AUTO)
-		tcp_sock_ns[port][V6] = s;
-}
-
 /**
  * tcp_ns_sock_init() - Init socket to listen for spliced outbound connections
  * @c:		Execution context
@@ -2655,9 +2621,9 @@ static void tcp_ns_sock_init(const struct ctx *c, in_port_t port)
 	ASSERT(!c->no_tcp);
 
 	if (c->ifi4)
-		tcp_ns_sock_init4(c, port);
+		tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port);
 	if (c->ifi6)
-		tcp_ns_sock_init6(c, port);
+		tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback6, NULL, port);
 }
 
 /**
@@ -2855,7 +2821,7 @@ static void tcp_port_rebind(struct ctx *c, bool outbound)
 			if (outbound)
 				tcp_ns_sock_init(c, port);
 			else
-				tcp_sock_init(c, NULL, NULL, port);
+				tcp_sock_init(c, PIF_HOST, NULL, NULL, port);
 		}
 	}
 }
diff --git a/tcp.h b/tcp.h
index 00823867..de6b9f93 100644
--- a/tcp.h
+++ b/tcp.h
@@ -18,8 +18,9 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
 int tcp_tap_handler(const struct ctx *c, uint8_t pif, sa_family_t af,
 		    const void *saddr, const void *daddr, uint32_t flow_lbl,
 		    const struct pool *p, int idx, const struct timespec *now);
-int tcp_sock_init(const struct ctx *c, const union inany_addr *addr,
-		  const char *ifname, in_port_t port);
+int tcp_sock_init(const struct ctx *c, uint8_t pif,
+		  const union inany_addr *addr, const char *ifname,
+		  in_port_t port);
 int tcp_init(struct ctx *c);
 void tcp_port_rebind_all(struct ctx *c);
 void tcp_timer(const struct ctx *c, const struct timespec *now);
-- 
2.52.0

[PATCH v5 08/15] udp: Unify some more inbound/outbound parts of udp_sock_init()

[David Gibson]

udp_sock_init() takes an 'ns' parameter determining if it creates a socket
in the guest namespace or host namespace.  Alter it to take a pif
parameter instead, like tcp_sock_init(), and use that change to slightly
reduce code duplication between the HOST and SPLICE cases.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c |  2 +-
 udp.c  | 63 ++++++++++++++++++++++++++++++----------------------------
 udp.h  |  5 +++--
 3 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/conf.c b/conf.c
index 04fbc67b..6bd9717b 100644
--- a/conf.c
+++ b/conf.c
@@ -177,7 +177,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		if (optname == 't')
 			ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i);
 		else if (optname == 'u')
-			ret = udp_sock_init(c, 0, addr, ifname, i);
+			ret = udp_sock_init(c, PIF_HOST, addr, ifname, i);
 		else
 			/* No way to check in advance for -T and -U */
 			ret = 0;
diff --git a/udp.c b/udp.c
index d414ca32..3b43f136 100644
--- a/udp.c
+++ b/udp.c
@@ -1104,64 +1104,66 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 /**
  * udp_sock_init() - Initialise listening sockets for a given port
  * @c:		Execution context
- * @ns:		In pasta mode, if set, bind with loopback address in namespace
+ * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
  * @addr:	Pointer to address for binding, NULL if not configured
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
  * Return: 0 on (partial) success, negative error code on (complete) failure
  */
-int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr,
-		  const char *ifname, in_port_t port)
+int udp_sock_init(const struct ctx *c, uint8_t pif,
+		  const union inany_addr *addr, const char *ifname,
+		  in_port_t port)
 {
 	union udp_listen_epoll_ref uref = {
-		.pif = ns ? PIF_SPLICE : PIF_HOST,
+		.pif = pif,
 		.port = port,
 	};
 	int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
+	int (*socks)[NUM_PORTS];
 
 	ASSERT(!c->no_udp);
+	ASSERT(pif_is_socket(pif));
 
-	if (!addr && c->ifi4 && c->ifi6 && !ns) {
+	if (pif == PIF_HOST)
+		socks = udp_splice_init;
+	else
+		socks = udp_splice_ns;
+
+	if (!addr && c->ifi4 && c->ifi6 && pif == PIF_HOST) {
 		int s;
 
 		/* Attempt to get a dual stack socket */
 		s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
 				NULL, ifname, port, uref.u32);
-		udp_splice_init[V4][port] = s < 0 ? -1 : s;
-		udp_splice_init[V6][port] = s < 0 ? -1 : s;
+		socks[V4][port] = s < 0 ? -1 : s;
+		socks[V6][port] = s < 0 ? -1 : s;
 		if (IN_INTERVAL(0, FD_REF_MAX, s))
 			return 0;
 	}
 
 	if ((!addr || inany_v4(addr)) && c->ifi4) {
-		if (!ns) {
-			r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
-					 addr ? addr : &inany_any4, ifname,
-					 port, uref.u32);
+		const union inany_addr *a = addr ? addr : &inany_any4;
 
-			udp_splice_init[V4][port] = r4 < 0 ? -1 : r4;
-		} else {
-			r4  = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE,
-					  &inany_loopback4, ifname,
-					  port, uref.u32);
-			udp_splice_ns[V4][port] = r4 < 0 ? -1 : r4;
-		}
+		if (pif == PIF_SPLICE)
+			a = &inany_loopback4;
+
+		r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
+				 port, uref.u32);
+
+		socks[V4][port] = r4 < 0 ? -1 : r4;
 	}
 
 	if ((!addr || !inany_v4(addr)) && c->ifi6) {
-		if (!ns) {
-			r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
-					 addr ? addr : &inany_any6, ifname,
-					 port, uref.u32);
+		const union inany_addr *a = addr ? addr : &inany_any6;
 
-			udp_splice_init[V6][port] = r6 < 0 ? -1 : r6;
-		} else {
-			r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_SPLICE,
-					 &inany_loopback6, ifname,
-					 port, uref.u32);
-			udp_splice_ns[V6][port] = r6 < 0 ? -1 : r6;
-		}
+		if (pif == PIF_SPLICE)
+			a = &inany_loopback6;
+
+		r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
+				 port, uref.u32);
+
+		socks[V6][port] = r6 < 0 ? -1 : r6;
 	}
 
 	if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6))
@@ -1221,7 +1223,8 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
 
 		if ((c->ifi4 && socks[V4][port] == -1) ||
 		    (c->ifi6 && socks[V6][port] == -1))
-			udp_sock_init(c, outbound, NULL, NULL, port);
+			udp_sock_init(c, outbound ? PIF_SPLICE : PIF_HOST,
+				      NULL, NULL, port);
 	}
 }
 
diff --git a/udp.h b/udp.h
index f1d83f38..03e8dc54 100644
--- a/udp.h
+++ b/udp.h
@@ -15,8 +15,9 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 		    sa_family_t af, const void *saddr, const void *daddr,
 		    uint8_t ttl, const struct pool *p, int idx,
 		    const struct timespec *now);
-int udp_sock_init(const struct ctx *c, int ns, const union inany_addr *addr,
-		  const char *ifname, in_port_t port);
+int udp_sock_init(const struct ctx *c, uint8_t pif,
+		  const union inany_addr *addr, const char *ifname,
+		  in_port_t port);
 int udp_init(struct ctx *c);
 void udp_port_rebind_all(struct ctx *c);
 void udp_update_l2_buf(const unsigned char *eth_d);
-- 
2.52.0

[PATCH v5 09/15] udp: Move udp_sock_init() special case to its caller

[David Gibson]

Inbound sockets are bound to the unspecified address by default, whereas
outbound sockets are bound to the loopback address.  This is currently
handled in udp_sock_init() which ignores its addr argument in the outbound
case.

Move the handling of this special case to the caller, udp_port_rebind().
This makes the semantics of the 'addr' argument more consistent, and will
make future changes easier.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/udp.c b/udp.c
index 3b43f136..b3ce9c7f 100644
--- a/udp.c
+++ b/udp.c
@@ -1130,7 +1130,7 @@ int udp_sock_init(const struct ctx *c, uint8_t pif,
 	else
 		socks = udp_splice_ns;
 
-	if (!addr && c->ifi4 && c->ifi6 && pif == PIF_HOST) {
+	if (!addr && c->ifi4 && c->ifi6) {
 		int s;
 
 		/* Attempt to get a dual stack socket */
@@ -1145,9 +1145,6 @@ int udp_sock_init(const struct ctx *c, uint8_t pif,
 	if ((!addr || inany_v4(addr)) && c->ifi4) {
 		const union inany_addr *a = addr ? addr : &inany_any4;
 
-		if (pif == PIF_SPLICE)
-			a = &inany_loopback4;
-
 		r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
 				 port, uref.u32);
 
@@ -1157,9 +1154,6 @@ int udp_sock_init(const struct ctx *c, uint8_t pif,
 	if ((!addr || !inany_v4(addr)) && c->ifi6) {
 		const union inany_addr *a = addr ? addr : &inany_any6;
 
-		if (pif == PIF_SPLICE)
-			a = &inany_loopback6;
-
 		r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
 				 port, uref.u32);
 
@@ -1222,9 +1216,16 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
 		}
 
 		if ((c->ifi4 && socks[V4][port] == -1) ||
-		    (c->ifi6 && socks[V6][port] == -1))
-			udp_sock_init(c, outbound ? PIF_SPLICE : PIF_HOST,
-				      NULL, NULL, port);
+		    (c->ifi6 && socks[V6][port] == -1)) {
+			if (outbound) {
+				udp_sock_init(c, PIF_SPLICE,
+					      &inany_loopback4, NULL, port);
+				udp_sock_init(c, PIF_SPLICE,
+					      &inany_loopback6, NULL, port);
+			} else {
+				udp_sock_init(c, PIF_HOST, NULL, NULL, port);
+			}
+		}
 	}
 }
 
-- 
2.52.0

[PATCH v5 10/15] util: Fix setting of IPV6_V6ONLY socket option

[David Gibson]

Currently we only call setsockopt() on IPV6_V6ONLY when we want to set it
to 1, which we typically do on all IPv6 sockets except those explicitly for
dual stack listening.  That's not quite right in two ways:

 * Although IPV6_V6ONLY==0 is normally the default on Linux, that can be
   changed with the net.ipv6.bindv6only sysctl.  It may also have different
   defaults on other OSes if we ever support them.  We know we need it off
   for dual stack sockets, so explicitly set it to 0 in that case.

 * At the same time setting IPV6_V6ONLY to 1 for IPv6 sockets bound to a
   specific address is harmless but pointless.  Don't set the option at all
   in this case, saving a syscall.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 util.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/util.c b/util.c
index ee369614..853c35a3 100644
--- a/util.c
+++ b/util.c
@@ -45,13 +45,13 @@
  * @type:	epoll type
  * @sa:		Socket address to bind to
  * @ifname:	Interface for binding, NULL for any
- * @v6only:	Set IPV6_V6ONLY socket option
+ * @v6only:	If >= 0, set IPV6_V6ONLY socket option to this value
  *
  * Return: newly created socket, negative error code on failure
  */
 static int sock_l4_(const struct ctx *c, enum epoll_type type,
 		    const union sockaddr_inany *sa, const char *ifname,
-		    bool v6only)
+		    int v6only)
 {
 	sa_family_t af = sa->sa_family;
 	bool freebind = false;
@@ -95,9 +95,13 @@ static int sock_l4_(const struct ctx *c, enum epoll_type type,
 		return -EBADF;
 	}
 
-	if (v6only)
-		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &y, sizeof(y)))
-			debug("Failed to set IPV6_V6ONLY on socket %i", fd);
+	if (v6only >= 0) {
+		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY,
+			       &v6only, sizeof(v6only))) {
+			debug("Failed to set IPV6_V6ONLY to %d on socket %i",
+			      v6only, fd);
+		}
+	}
 
 	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
 		debug("Failed to set SO_REUSEADDR on socket %i", fd);
@@ -181,7 +185,16 @@ static int sock_l4_(const struct ctx *c, enum epoll_type type,
 int sock_l4(const struct ctx *c, enum epoll_type type,
 	    const union sockaddr_inany *sa, const char *ifname)
 {
-	return sock_l4_(c, type, sa, ifname, sa->sa_family == AF_INET6);
+	int v6only = -1;
+
+	/* The option doesn't exist for IPv4 sockets, and we don't care about it
+	 * for IPv6 sockets with a non-wildcard address.
+	 */
+	if (sa->sa_family == AF_INET6 &&
+	    IN6_IS_ADDR_UNSPECIFIED(&sa->sa6.sin6_addr))
+		v6only = 1;
+
+	return sock_l4_(c, type, sa, ifname, v6only);
 }
 
 /**
@@ -204,6 +217,10 @@ int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
 		.sa6.sin6_port = htons(port),
 	};
 
+	/* Dual stack sockets require IPV6_V6ONLY == 0.  Usually that's the
+	 * default, but sysctl net.ipv6.bindv6only can change that, so set the
+	 * sockopt explicitly.
+	 */
 	return sock_l4_(c, type, &sa, ifname, 0);
 }
 
-- 
2.52.0

[PATCH v5 11/15] tcp, udp: Remove fallback if creating dual stack socket fails

[David Gibson]

To save kernel memory we try to use "dual stack" sockets which can listen
for both IPv4 and IPv6 connections where possible.  To support kernels
which don't allow dual stack sockets, we fall back to creating individual
sockets for IPv4 and IPv6.

This fallback causes some mild ugliness now, and will cause more difficulty
with upcoming improvements to the forwarding logic.  I don't think we need
the fallback on the following grounds:

1) The fallback was broken since inception:

The fallback was triggered if pif_sock_l4() failed attempting to create the
dual stack socket.  But even if the kernel didn't support them,
pif_sock_l4() would not report a failure.
  - Dual stack sockets are distinguished by having the IPV6_V6ONLY sockopt
    set to 0.  However, until the last patch, we only called setsockopt()
    if we wanted to set this to 1, so there was no kernel operation which
    could fail for dual stack sockets - we'd silently create a IPv6 only
    socket instead.
  - Even if we did call the setsockopt(), we only printed a debug() message
    for failures, we didn't report it to the caller

2) Dual stack sockets are not just a Linux extension

The dual stack socket interface is described in RFC3493, specifically
section 3.7 and section 5.3.  It is supported on BSD:
  https://man.freebsd.org/cgi/man.cgi?query=ip6
and on Windows:
  https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ipv6-socket-options

3) Linux has supported dual stack sockets for over 20 years

According to ipv6(7) the IPV6_V6ONLY socket option was introduced in
Linux 2.6 and Linux 2.4.21 both from 2003.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 43 +++++++++++++++++++++++++------------------
 udp.c | 56 ++++++++++++++++++++++++++------------------------------
 2 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/tcp.c b/tcp.c
index 428bac7b..2abb8be4 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2575,42 +2575,49 @@ static int tcp_sock_init_one(const struct ctx *c, uint8_t pif,
 }
 
 /**
- * tcp_sock_init() - Create listening sockets for a given host ("inbound") port
+ * tcp_sock_init() - Create listening socket for a given host ("inbound") port
  * @c:		Execution context
  * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
  * @addr:	Pointer to address for binding, NULL if not configured
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
- * Return: 0 on (partial) success, negative error code on (complete) failure
+ * Return: 0 on success, negative error code on failure
  */
 int tcp_sock_init(const struct ctx *c, uint8_t pif,
 		  const union inany_addr *addr, const char *ifname,
 		  in_port_t port)
 {
-	int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
+	int s;
 
 	ASSERT(!c->no_tcp);
 
-	if (!addr && c->ifi4 && c->ifi6)
-		/* Attempt to get a dual stack socket */
-		if (tcp_sock_init_one(c, pif, NULL, ifname, port) >= 0)
+	if (!c->ifi4) {
+		if (!addr)
+			/* Restrict to v6 only */
+			addr = &inany_any6;
+		else if (inany_v4(addr))
+			/* Nothing to do */
 			return 0;
+	}
+	if (!c->ifi6) {
+		if (!addr)
+			/* Restrict to v4 only */
+			addr = &inany_any4;
+		else if (!inany_v4(addr))
+			/* Nothing to do */
+			return 0;
+	}
 
-	/* Otherwise create a socket per IP version */
-	if ((!addr || inany_v4(addr)) && c->ifi4)
-		r4 = tcp_sock_init_one(c, pif,
-				       addr ? addr : &inany_any4, ifname, port);
-
-	if ((!addr || !inany_v4(addr)) && c->ifi6)
-		r6 = tcp_sock_init_one(c, pif,
-				       addr ? addr : &inany_any6, ifname, port);
-
-	if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6))
-		return 0;
+	s = tcp_sock_init_one(c, pif, addr, ifname, port);
+	if (s < 0)
+		return s;
+	if (s > FD_REF_MAX)
+		return -EIO;
 
-	return r4 < 0 ? r4 : r6;
+	return 0;
 }
+
 /**
  * tcp_ns_sock_init() - Init socket to listen for spliced outbound connections
  * @c:		Execution context
diff --git a/udp.c b/udp.c
index b3ce9c7f..3d097fbb 100644
--- a/udp.c
+++ b/udp.c
@@ -1102,14 +1102,14 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
 }
 
 /**
- * udp_sock_init() - Initialise listening sockets for a given port
+ * udp_sock_init() - Initialise listening socket for a given port
  * @c:		Execution context
  * @pif:	Interface to open the socket for (PIF_HOST or PIF_SPLICE)
  * @addr:	Pointer to address for binding, NULL if not configured
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
- * Return: 0 on (partial) success, negative error code on (complete) failure
+ * Return: 0 on success, negative error code on failure
  */
 int udp_sock_init(const struct ctx *c, uint8_t pif,
 		  const union inany_addr *addr, const char *ifname,
@@ -1119,8 +1119,8 @@ int udp_sock_init(const struct ctx *c, uint8_t pif,
 		.pif = pif,
 		.port = port,
 	};
-	int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1;
 	int (*socks)[NUM_PORTS];
+	int s;
 
 	ASSERT(!c->no_udp);
 	ASSERT(pif_is_socket(pif));
@@ -1130,40 +1130,36 @@ int udp_sock_init(const struct ctx *c, uint8_t pif,
 	else
 		socks = udp_splice_ns;
 
-	if (!addr && c->ifi4 && c->ifi6) {
-		int s;
-
-		/* Attempt to get a dual stack socket */
-		s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, PIF_HOST,
-				NULL, ifname, port, uref.u32);
-		socks[V4][port] = s < 0 ? -1 : s;
-		socks[V6][port] = s < 0 ? -1 : s;
-		if (IN_INTERVAL(0, FD_REF_MAX, s))
+	if (!c->ifi4) {
+		if (!addr)
+			/* Restrict to v6 only */
+			addr = &inany_any6;
+		else if (inany_v4(addr))
+			/* Nothing to do */
 			return 0;
 	}
-
-	if ((!addr || inany_v4(addr)) && c->ifi4) {
-		const union inany_addr *a = addr ? addr : &inany_any4;
-
-		r4 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
-				 port, uref.u32);
-
-		socks[V4][port] = r4 < 0 ? -1 : r4;
+	if (!c->ifi6) {
+		if (!addr)
+			/* Restrict to v4 only */
+			addr = &inany_any4;
+		else if (!inany_v4(addr))
+			/* Nothing to do */
+			return 0;
 	}
 
-	if ((!addr || !inany_v4(addr)) && c->ifi6) {
-		const union inany_addr *a = addr ? addr : &inany_any6;
-
-		r6 = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif, a, ifname,
-				 port, uref.u32);
-
-		socks[V6][port] = r6 < 0 ? -1 : r6;
+	s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif,
+			addr, ifname, port, uref.u32);
+	if (s > FD_REF_MAX) {
+		close(s);
+		s = -EIO;
 	}
 
-	if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6))
-		return 0;
+	if (!addr || inany_v4(addr))
+		socks[V4][port] = s < 0 ? -1 : s;
+	if (!addr || !inany_v4(addr))
+		socks[V6][port] = s < 0 ? -1 : s;
 
-	return r4 < 0 ? r4 : r6;
+	return s < 0 ? s : 0;
 }
 
 /**
-- 
2.52.0

[PATCH v5 12/15] tcp, udp: Bind outbound listening sockets by interface instead of address

[David Gibson]

Currently, outbound forwards (-T, -U) are handled by sockets bound to the
loopback address.  Typically we create two sockets, one for 127.0.0.1 and
one for ::1.

This has some disadvantages:
 * The guest can't connect via 127.0.0.0/8 addresses other than 127.0.0.1
 * We can't use dual-stack sockets, we have to have separate sockets for
   IPv4 and IPv6.

The restriction exists for a reason though.  If the guest has any
interfaces other than pasta (e.g. a VPN tunnel) external hosts could reach
the host via the forwards.  Especially combined with -T auto / -U auto this
would make it very easy to make a mistake with nasty security implications.

We can achieve this a different way, however.  Don't bind to a specific
address, but _do_ use SO_BINDTODEVICE to restrict the sockets to the "lo"
interface.  We fall back to the old behaviour for older kernels where
SO_BINDTODEVICE is not available unprivileged.

Note that although traffic to a local but non-loopback address is passed
over the 'lo' interface (as seen by netfilter and dumpcap), it doesn't
count as attached to that interface for the purposes of SO_BINDTODEVICE
(information from the routing table overrides the "physical" interface).
So, this change doesn't help for bug 100.

It's also not a complete fix for bug 113, it does however:
 * Get us a step closer to fixing bug 113
 * Slightly simplify the code
 * Make things a bit easier to allow more flexible binding on the guest in
   in future

Link: https://bugs.passt.top/show_bug.cgi?id=113

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c |  6 ++++++
 pif.c  |  6 ------
 tcp.c  |  5 +++++
 udp.c  | 30 +++++++++++++++++++++++-------
 4 files changed, 34 insertions(+), 13 deletions(-)

diff --git a/conf.c b/conf.c
index 6bd9717b..02a4b65a 100644
--- a/conf.c
+++ b/conf.c
@@ -235,6 +235,12 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		if (c->mode != MODE_PASTA)
 			die("'auto' port forwarding is only allowed for pasta");
 
+		if ((optname == 'T' || optname == 'U') && c->no_bindtodevice) {
+			warn(
+"'-%c auto' enabled without unprivileged SO_BINDTODEVICE", optname);
+			warn(
+"Forwarding from addresses other than 127.0.0.1 will not work");
+		}
 		fwd->mode = FWD_AUTO;
 		return;
 	}
diff --git a/pif.c b/pif.c
index 85904f35..db447b4f 100644
--- a/pif.c
+++ b/pif.c
@@ -81,12 +81,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 
 	ASSERT(pif_is_socket(pif));
 
-	if (pif == PIF_SPLICE) {
-		/* Sanity checks */
-		ASSERT(!ifname);
-		ASSERT(addr && inany_is_loopback(addr));
-	}
-
 	if (!addr) {
 		ref.fd = sock_l4_dualstack(c, type, port, ifname);
 	} else {
diff --git a/tcp.c b/tcp.c
index 2abb8be4..aacc5b20 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2627,6 +2627,11 @@ static void tcp_ns_sock_init(const struct ctx *c, in_port_t port)
 {
 	ASSERT(!c->no_tcp);
 
+	if (!c->no_bindtodevice) {
+		tcp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
+		return;
+	}
+
 	if (c->ifi4)
 		tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port);
 	if (c->ifi6)
diff --git a/udp.c b/udp.c
index 3d097fbb..4b625b78 100644
--- a/udp.c
+++ b/udp.c
@@ -1182,6 +1182,26 @@ static void udp_splice_iov_init(void)
 	}
 }
 
+/**
+ * udp_ns_sock_init() - Init socket to listen for spliced outbound connections
+ * @c:		Execution context
+ * @port:	Port, host order
+ */
+static void udp_ns_sock_init(const struct ctx *c, in_port_t port)
+{
+	ASSERT(!c->no_udp);
+
+	if (!c->no_bindtodevice) {
+		udp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
+		return;
+	}
+
+	if (c->ifi4)
+		udp_sock_init(c, PIF_SPLICE, &inany_loopback4, NULL, port);
+	if (c->ifi6)
+		udp_sock_init(c, PIF_SPLICE, &inany_loopback6, NULL, port);
+}
+
 /**
  * udp_port_rebind() - Rebind ports to match forward maps
  * @c:		Execution context
@@ -1213,14 +1233,10 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
 
 		if ((c->ifi4 && socks[V4][port] == -1) ||
 		    (c->ifi6 && socks[V6][port] == -1)) {
-			if (outbound) {
-				udp_sock_init(c, PIF_SPLICE,
-					      &inany_loopback4, NULL, port);
-				udp_sock_init(c, PIF_SPLICE,
-					      &inany_loopback6, NULL, port);
-			} else {
+			if (outbound)
+				udp_ns_sock_init(c, port);
+			else
 				udp_sock_init(c, PIF_HOST, NULL, NULL, port);
-			}
 		}
 	}
 }
-- 
2.52.0

[PATCH v5 13/15] util: Rename sock_l4_dualstack() to sock_l4_dualstack_any()

[David Gibson]

Stefano correctly noted that the fact a socket is dual-stack doesn't
necessarily imply that it is bound to a wildcard address.  While that's the
only case we use for dual-stack sockets, there may be others.  Therefore
rename this function to make it clearer that it always uses a wildcard
bind.

Suggested-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 pif.c  | 2 +-
 util.c | 6 +++---
 util.h | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/pif.c b/pif.c
index db447b4f..3d7a90e5 100644
--- a/pif.c
+++ b/pif.c
@@ -82,7 +82,7 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
 	ASSERT(pif_is_socket(pif));
 
 	if (!addr) {
-		ref.fd = sock_l4_dualstack(c, type, port, ifname);
+		ref.fd = sock_l4_dualstack_any(c, type, port, ifname);
 	} else {
 		union sockaddr_inany sa;
 
diff --git a/util.c b/util.c
index 853c35a3..fb1945c1 100644
--- a/util.c
+++ b/util.c
@@ -198,7 +198,7 @@ int sock_l4(const struct ctx *c, enum epoll_type type,
 }
 
 /**
- * sock_l4_dualstack() - Create a dual stack socket bound with wildcard address
+ * sock_l4_dualstack_any() - Create dualstack socket bound to :: and 0.0.0.0
  * @c:		Execution context
  * @type:	epoll type
  * @port	Port to bind to (:: and 0.0.0.0)
@@ -208,8 +208,8 @@ int sock_l4(const struct ctx *c, enum epoll_type type,
  *
  * A dual stack socket is effectively bound to both :: and 0.0.0.0.
  */
-int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
-		      in_port_t port, const char *ifname)
+int sock_l4_dualstack_any(const struct ctx *c, enum epoll_type type,
+			  in_port_t port, const char *ifname)
 {
 	union sockaddr_inany sa = {
 		.sa6.sin6_family = AF_INET6,
diff --git a/util.h b/util.h
index ebc14138..21acc05f 100644
--- a/util.h
+++ b/util.h
@@ -210,8 +210,8 @@ union sockaddr_inany;
 
 int sock_l4(const struct ctx *c, enum epoll_type type,
 	    const union sockaddr_inany *sa, const char *ifname);
-int sock_l4_dualstack(const struct ctx *c, enum epoll_type type,
-		      in_port_t port, const char *ifname);
+int sock_l4_dualstack_any(const struct ctx *c, enum epoll_type type,
+			  in_port_t port, const char *ifname);
 int sock_unix(char *sock_path);
 void sock_probe_features(struct ctx *c);
 long timespec_diff_ms(const struct timespec *a, const struct timespec *b);
-- 
2.52.0

[PATCH v5 14/15] tcp: Always populate oaddr field for socket initiated flows

[David Gibson]

When we receive a TCP connection, we get the peer address from the accept()
call.  In the case of a listening socket with an unspecified address (:: or
0.0.0.0) the local address of the accept()ed socket could vary.  We don't
get that from the accept() - we must explicitly call getsockname() to get
it.

Currently we avoid the latency of that extra syscall, and therefore don't
populate the initiating 'oaddr' field of a flow created by an incoming TCP
socket connection.  This more or less works, because we rarely need that
local address, but it does cause some oddities:

 * For migration we need the local address to recreate the socket on the
   destination, so we *do* call getsockname() in vhost-user mode
 * It limits our options in terms of forwarding connections flexibly based
   on the address to which they're received
 * It differs from UDP, where we explicitly use the IP_PKTINFO cmsg to
   populate oaddr.
 * It means (some) flow debug messages will contain wildcards instead of
   real local addresses

In theory we can elide this call when accept()ing from a socket bound to
a specific address instead of a wildcard.  However to do that will need
revisions to the data structures we use to keep track of listening sockets.

The lack of this information is making it hard to implement some fixes we
want.  So, pay the price of the extra syscall to get this information, with
the hope that we can later optimise it away for some cases.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tcp.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tcp.c b/tcp.c
index aacc5b20..535d6cfa 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2354,11 +2354,9 @@ void tcp_listen_handler(const struct ctx *c, union epoll_ref ref,
 	ini = flow_initiate_sa(flow, ref.tcp_listen.pif, &sa,
 			       NULL, ref.tcp_listen.port);
 
-	if (c->mode == MODE_VU) { /* Rebind to same address after migration */
-		if (getsockname(s, &sa.sa, &sl) ||
-		    inany_from_sockaddr(&ini->oaddr, &ini->oport, &sa) < 0)
-			err_perror("Can't get local address for socket %i", s);
-	}
+	if (getsockname(s, &sa.sa, &sl) ||
+	    inany_from_sockaddr(&ini->oaddr, &ini->oport, &sa) < 0)
+		err_perror("Can't get local address for socket %i", s);
 
 	if (!inany_is_unicast(&ini->eaddr) || ini->eport == 0) {
 		char sastr[SOCKADDR_STRLEN];
-- 
2.52.0

[PATCH v5 15/15] fwd: Preserve non-standard loopback address when splice forwarding

[David Gibson]

When forwarding "spliced" connections outwards (-T or -U) we listen on the
guest's loopback and always forward to 127.0.0.1 (or ::1) on the host.
However, it's also possible for clients on the guest to attempt connecting
to other addresses in 127.0.0.0/8 (systemd-resolved uses 127.0.0.53 in
practice).  If the host side server is only listening on that specific
non-standard loopback address, the forward won't work.  Fix this by
preserving the specific (loopback) address when forwarding such
connections.

Link: https://bugs.passt.top/show_bug.cgi?id=113

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

diff --git a/fwd.c b/fwd.c
index c417e0f5..44a0e109 100644
--- a/fwd.c
+++ b/fwd.c
@@ -660,7 +660,9 @@ uint8_t fwd_nat_from_splice(const struct ctx *c, uint8_t proto,
 		return PIF_NONE;
 	}
 
-	if (inany_v4(&ini->eaddr))
+	if (!inany_is_unspecified(&ini->oaddr))
+		tgt->eaddr = ini->oaddr;
+	else if (inany_v4(&ini->oaddr))
 		tgt->eaddr = inany_loopback4;
 	else
 		tgt->eaddr = inany_loopback6;
-- 
2.52.0

Re: [PATCH v5 12/15] tcp, udp: Bind outbound listening sockets by interface instead of address

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5168 bytes --]

Sorry, I realised the warning I add here isn't correct in a couple of
ways.  I'll respin shortly, but patches 1..11 should still be fine to
review.

On Tue, Dec 02, 2025 at 03:02:12PM +1100, David Gibson wrote:
> Currently, outbound forwards (-T, -U) are handled by sockets bound to the
> loopback address.  Typically we create two sockets, one for 127.0.0.1 and
> one for ::1.
> 
> This has some disadvantages:
>  * The guest can't connect via 127.0.0.0/8 addresses other than 127.0.0.1
>  * We can't use dual-stack sockets, we have to have separate sockets for
>    IPv4 and IPv6.
> 
> The restriction exists for a reason though.  If the guest has any
> interfaces other than pasta (e.g. a VPN tunnel) external hosts could reach
> the host via the forwards.  Especially combined with -T auto / -U auto this
> would make it very easy to make a mistake with nasty security implications.
> 
> We can achieve this a different way, however.  Don't bind to a specific
> address, but _do_ use SO_BINDTODEVICE to restrict the sockets to the "lo"
> interface.  We fall back to the old behaviour for older kernels where
> SO_BINDTODEVICE is not available unprivileged.
> 
> Note that although traffic to a local but non-loopback address is passed
> over the 'lo' interface (as seen by netfilter and dumpcap), it doesn't
> count as attached to that interface for the purposes of SO_BINDTODEVICE
> (information from the routing table overrides the "physical" interface).
> So, this change doesn't help for bug 100.
> 
> It's also not a complete fix for bug 113, it does however:
>  * Get us a step closer to fixing bug 113
>  * Slightly simplify the code
>  * Make things a bit easier to allow more flexible binding on the guest in
>    in future
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=113
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c |  6 ++++++
>  pif.c  |  6 ------
>  tcp.c  |  5 +++++
>  udp.c  | 30 +++++++++++++++++++++++-------
>  4 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6bd9717b..02a4b65a 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -235,6 +235,12 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		if (c->mode != MODE_PASTA)
>  			die("'auto' port forwarding is only allowed for pasta");
>  
> +		if ((optname == 'T' || optname == 'U') && c->no_bindtodevice) {
> +			warn(
> +"'-%c auto' enabled without unprivileged SO_BINDTODEVICE", optname);
> +			warn(
> +"Forwarding from addresses other than 127.0.0.1 will not work");
> +		}
>  		fwd->mode = FWD_AUTO;
>  		return;
>  	}
> diff --git a/pif.c b/pif.c
> index 85904f35..db447b4f 100644
> --- a/pif.c
> +++ b/pif.c
> @@ -81,12 +81,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
>  
>  	ASSERT(pif_is_socket(pif));
>  
> -	if (pif == PIF_SPLICE) {
> -		/* Sanity checks */
> -		ASSERT(!ifname);
> -		ASSERT(addr && inany_is_loopback(addr));
> -	}
> -
>  	if (!addr) {
>  		ref.fd = sock_l4_dualstack(c, type, port, ifname);
>  	} else {
> diff --git a/tcp.c b/tcp.c
> index 2abb8be4..aacc5b20 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2627,6 +2627,11 @@ static void tcp_ns_sock_init(const struct ctx *c, in_port_t port)
>  {
>  	ASSERT(!c->no_tcp);
>  
> +	if (!c->no_bindtodevice) {
> +		tcp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
> +		return;
> +	}
> +
>  	if (c->ifi4)
>  		tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port);
>  	if (c->ifi6)
> diff --git a/udp.c b/udp.c
> index 3d097fbb..4b625b78 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1182,6 +1182,26 @@ static void udp_splice_iov_init(void)
>  	}
>  }
>  
> +/**
> + * udp_ns_sock_init() - Init socket to listen for spliced outbound connections
> + * @c:		Execution context
> + * @port:	Port, host order
> + */
> +static void udp_ns_sock_init(const struct ctx *c, in_port_t port)
> +{
> +	ASSERT(!c->no_udp);
> +
> +	if (!c->no_bindtodevice) {
> +		udp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
> +		return;
> +	}
> +
> +	if (c->ifi4)
> +		udp_sock_init(c, PIF_SPLICE, &inany_loopback4, NULL, port);
> +	if (c->ifi6)
> +		udp_sock_init(c, PIF_SPLICE, &inany_loopback6, NULL, port);
> +}
> +
>  /**
>   * udp_port_rebind() - Rebind ports to match forward maps
>   * @c:		Execution context
> @@ -1213,14 +1233,10 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
>  
>  		if ((c->ifi4 && socks[V4][port] == -1) ||
>  		    (c->ifi6 && socks[V6][port] == -1)) {
> -			if (outbound) {
> -				udp_sock_init(c, PIF_SPLICE,
> -					      &inany_loopback4, NULL, port);
> -				udp_sock_init(c, PIF_SPLICE,
> -					      &inany_loopback6, NULL, port);
> -			} else {
> +			if (outbound)
> +				udp_ns_sock_init(c, port);
> +			else
>  				udp_sock_init(c, PIF_HOST, NULL, NULL, port);
> -			}
>  		}
>  	}
>  }
> -- 
> 2.52.0
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/15] util: Extend sock_probe_mem() to sock_probe_features()

[Stefano Brivio]

On Tue,  2 Dec 2025 15:02:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> sock_probe_mem() currently checks whether we're able to allocate large
> socket buffers.  Extend it to also check whether the SO_BINDTODEVICE
> socket option is available.  Rename to sock_probe_features() to reflect the
> new functionality.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  passt.c |  2 +-
>  passt.h |  2 ++
>  util.c  | 19 +++++++++++++++++--
>  util.h  |  2 +-
>  4 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index 4964427d..0b84ac6c 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -381,7 +381,7 @@ int main(int argc, char **argv)
>  	if (setrlimit(RLIMIT_NOFILE, &limit))
>  		die_perror("Failed to set current limit for open files");
>  
> -	sock_probe_mem(&c);
> +	sock_probe_features(&c);
>  
>  	conf(&c, argc, argv);
>  	trace_init(c.trace);
> diff --git a/passt.h b/passt.h
> index 15801b44..79d01ddb 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -204,6 +204,7 @@ struct ip6_ctx {
>   * @freebind:		Allow binding of non-local addresses for forwarding
>   * @low_wmem:		Low probed net.core.wmem_max
>   * @low_rmem:		Low probed net.core.rmem_max
> + * @no_bindtodevice:	Unprivileged SO_BINDTODEVICE not available
>   * @vdev:		vhost-user device
>   * @device_state_fd:	Device state migration channel
>   * @device_state_result: Device state migration result
> @@ -281,6 +282,7 @@ struct ctx {
>  
>  	int low_wmem;
>  	int low_rmem;
> +	int no_bindtodevice;
>  
>  	struct vu_dev *vdev;
>  
> diff --git a/util.c b/util.c
> index 347f34f5..bad38129 100644
> --- a/util.c
> +++ b/util.c
> @@ -233,12 +233,13 @@ int sock_unix(char *sock_path)
>  }
>  
>  /**
> - * sock_probe_mem() - Check if setting high SO_SNDBUF and SO_RCVBUF is allowed
> + * sock_probe_features() - Probe for socket features we might use
>   * @c:		Execution context
>   */
> -void sock_probe_mem(struct ctx *c)
> +void sock_probe_features(struct ctx *c)
>  {
>  	int v = INT_MAX / 2, s;
> +	const char lo[] = "lo";
>  	socklen_t sl;
>  
>  	s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
> @@ -247,6 +248,7 @@ void sock_probe_mem(struct ctx *c)
>  		return;
>  	}
>  
> +	/* Check if setting high SO_SNDBUF and SO_RCVBUF is allowed */
>  	sl = sizeof(v);
>  	if (setsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v))	||
>  	    getsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, &sl) ||
> @@ -259,6 +261,19 @@ void sock_probe_mem(struct ctx *c)
>  	    (size_t)v < RCVBUF_BIG)
>  		c->low_rmem = 1;
>  
> +	/* Check if SO_BINDTODEVICE is available
> +	 *
> +	 * Supported since kernel version 5.7, commit c427bfec18f2 ("net: core:
> +	 * enable SO_BINDTODEVICE for non-root users").  Some distro kernels may
> +	 * have backports, of course.  Record whether we can use it so that we
> +	 * can give more useful diagnostics.
> +	 */
> +	if (setsockopt(s, SOL_SOCKET, SO_BINDTODEVICE, lo, sizeof(lo)-1)) {

Nit: our coding style uses spaces around arithmetic operators... I took
the liberty of fixing this up on merge, though.

-- 
Stefano

Re: [PATCH v5 00/15] Reduce differences between inbound and outbound socket binding

[Stefano Brivio]

On Tue,  2 Dec 2025 15:02:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> This series is based on my series fixing bug 176 (regression in auto
> forwarding).
> 
> The fact that outbound forwarding sockets are bound to the loopback
> address, whereas inbound forwarding sockets are (by default) bound to
> the unspecified address leads to some unexpected differences between
> the paths setting up each of them.
> 
> An idea for tackling bug 100 suggested a different approach which will
> also reduce some of those differences and allow more code to be shared
> between the two paths.  I've since discovered that this approach
> doesn't help for bug 100, but I think it's still worthwhile for other reasons.
> 
> v5:
>  - Combine with SO_BINDTODEVICE and bug 113 patch series
>  - Add fallback handling for kernels without SO_BINDTODEVICE
>  - Add missing struct field documentation for no_bindtodevice

Applied (with nit from 2/15 fixed).

-- 
Stefano

Re: [PATCH v5 12/15] tcp, udp: Bind outbound listening sockets by interface instead of address

[Stefano Brivio]

On Wed, 3 Dec 2025 15:41:36 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Sorry, I realised the warning I add here isn't correct in a couple of
> ways.  I'll respin shortly, but patches 1..11 should still be fine to
> review.

...read just in time, I won't push the series (not even up to 11/15 I
guess?).

-- 
Stefano

Re: [PATCH v5 12/15] tcp, udp: Bind outbound listening sockets by interface instead of address

[Stefano Brivio]

On Wed, 3 Dec 2025 07:38:55 +0100
Stefano Brivio <sbrivio@redhat.com> wrote:

> On Wed, 3 Dec 2025 15:41:36 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Sorry, I realised the warning I add here isn't correct in a couple of
> > ways.  I'll respin shortly, but patches 1..11 should still be fine to
> > review.  
> 
> ...read just in time, I won't push the series (not even up to 11/15 I
> guess?).

Pushed now, as you noted offline that the series doesn't actually make
the situation worse.

-- 
Stefano