[PATCH 0/3] Allow listen functions to return fds

[PATCH 0/3] Allow listen functions to return fds

[David Gibson]

For future forwarding changes, it makes more sense for the tcp and udp
listen functions to return socket fds, rather than just success /
failure.  Make the change, fixing bug 186 and adding a debugging
option along the way.

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

David Gibson (3):
  conf: Introduce --no-bindtodevice option for testing
  tcp, udp, conf: Don't silently ignore listens on unsupported IP
    versions
  tcp, udp: Make {tcp,udp}_listen() return socket fds

 conf.c  | 26 +++++++++++++++++++-------
 passt.1 |  6 ++++++
 tcp.c   | 13 ++++---------
 udp.c   | 10 ++++------
 4 files changed, 33 insertions(+), 22 deletions(-)

-- 
2.52.0

[PATCH 1/3] conf: Introduce --no-bindtodevice option for testing

[David Gibson]

We need to support (as best we can) older kernels which don't allow
unprivilieged processes to use the SO_BINDTODEVICE socket option.
Fallcaks for that case are controlled by the c->no_bindtodevice variable.

Currently testing behaviour of those fallbacks requires setting up a test
system with a kernel that doesn't support the option, which is pretty
awkward.  We can test it almost as well and much more easily by adding a
command line option to explicitly disable use of SO_BINDTODEVICE.

Like --no-splice this is envisaged as something for developers' and
testers' convenience, not a supported option for end users.  The man page
text reflects that.

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

diff --git a/conf.c b/conf.c
index ceb9aa55..70ea168c 100644
--- a/conf.c
+++ b/conf.c
@@ -962,6 +962,7 @@ static void usage(const char *name, FILE *f, int status)
 		"  --no-ndp		Disable NDP responses\n"
 		"  --no-dhcpv6		Disable DHCPv6 server\n"
 		"  --no-ra		Disable router advertisements\n"
+		"  --no-bindtodevice	Disable SO_BINDTODEVICE\n"
 		"  --freebind		Bind to any address for forwarding\n"
 		"  --no-map-gw		Don't map gateway address to host\n"
 		"  -4, --ipv4-only	Enable IPv4 operation only\n"
@@ -1454,6 +1455,7 @@ void conf(struct ctx *c, int argc, char **argv)
 		{"no-dhcpv6",	no_argument,		&c->no_dhcpv6,	1 },
 		{"no-ndp",	no_argument,		&c->no_ndp,	1 },
 		{"no-ra",	no_argument,		&c->no_ra,	1 },
+		{"no-bindtodevice", no_argument,	&c->no_bindtodevice, 1},
 		{"no-splice",	no_argument,		&c->no_splice,	1 },
 		{"freebind",	no_argument,		&c->freebind,	1 },
 		{"no-map-gw",	no_argument,		&no_map_gw,	1 },
diff --git a/passt.1 b/passt.1
index db0d6620..4859d9e5 100644
--- a/passt.1
+++ b/passt.1
@@ -348,6 +348,12 @@ namespace will be silently dropped.
 Disable Router Advertisements. Router Solicitations coming from guest or target
 namespace will be ignored.
 
+.TP
+.BR \-\-no-bindtodevice
+Development/testing option, do not use.  Disables use of
+SO_BINDTODEVICE socket option.  Implicitly enabled on older kernels
+which don't permit unprivileged use of SO_BINDTODEVICE.
+
 .TP
 .BR \-\-freebind
 Allow any binding address to be specified for \fB-t\fR and \fB-u\fR
-- 
2.52.0

[PATCH 2/3] tcp, udp, conf: Don't silently ignore listens on unsupported IP versions

[David Gibson]

Currently, it's possible to explicitly ask for forwarding from an IPv4
address, while disabling IPv4:
    $ pasta -t 192.0.2.1/12345 -6
or vice versa:
    $ pasta -t 2001:db8::1/12345 -4

Currently, the impossible to implement forwarding option will be silently
ignored.  That's potentially confusing since in a complex setup, it might
not be obvious why the requested forward isn't taking effect.

Specifically, it's ignored at a fairly low level: tcp_listen() and
udp_listen() ignore it and return 0.  Those run kind of late to give a
good error message.  Change the low-level functions to return -EACCES
(chosen because that's what the kernel will return if you request IPv6
when it's disabled by sysctl).  Most callers of {tcp,udp}_listen() ignore
the return code, so this is a no-op for them.  In the remaining caller,
conf_ports_range_except() check for the case explicitly, and provide a
meaningful error message.

Of itself, this bug is insignificant, but this is a roadblock to having
{tcp,udp}_listen() return socket fds, which in turn is a roadblock to my
flexible forwarding work.  So, might as well fix it.

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

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 10 ++++++++++
 tcp.c  |  6 ++----
 udp.c  |  6 ++----
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/conf.c b/conf.c
index 70ea168c..cc3c20a9 100644
--- a/conf.c
+++ b/conf.c
@@ -162,6 +162,16 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		    optname, optarg);
 	}
 
+	if (addr) {
+		if (!c->ifi4 && inany_v4(addr)) {
+			die("IPv4 is disabled, can't use -%c %s",
+			    optname, optarg);
+		} else if (!c->ifi6 && !inany_v4(addr)) {
+			die("IPv6 is disabled, can't use -%c %s",
+			    optname, optarg);
+		}
+	}
+
 	for (i = first; i <= last; i++) {
 		if (bitmap_isset(exclude, i))
 			continue;
diff --git a/tcp.c b/tcp.c
index e7fa85f3..67007c05 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2700,16 +2700,14 @@ int tcp_listen(const struct ctx *c, uint8_t pif,
 			/* Restrict to v6 only */
 			addr = &inany_any6;
 		else if (inany_v4(addr))
-			/* Nothing to do */
-			return 0;
+			return -EACCES;
 	}
 	if (!c->ifi6) {
 		if (!addr)
 			/* Restrict to v4 only */
 			addr = &inany_any4;
 		else if (!inany_v4(addr))
-			/* Nothing to do */
-			return 0;
+			return -EACCES;
 	}
 
 	if (pif == PIF_HOST) {
diff --git a/udp.c b/udp.c
index eda55c39..8cfa1e1f 100644
--- a/udp.c
+++ b/udp.c
@@ -1162,16 +1162,14 @@ int udp_listen(const struct ctx *c, uint8_t pif,
 			/* Restrict to v6 only */
 			addr = &inany_any6;
 		else if (inany_v4(addr))
-			/* Nothing to do */
-			return 0;
+			return -EACCES;
 	}
 	if (!c->ifi6) {
 		if (!addr)
 			/* Restrict to v4 only */
 			addr = &inany_any4;
 		else if (!inany_v4(addr))
-			/* Nothing to do */
-			return 0;
+			return -EACCES;
 	}
 
 	s = pif_sock_l4(c, EPOLL_TYPE_UDP_LISTEN, pif,
-- 
2.52.0

[PATCH 3/3] tcp, udp: Make {tcp,udp}_listen() return socket fds

[David Gibson]

{tcp,udp}_listen() currently return 0 on success, rather than the socket
fd they created.  Historically, that was because these functions could
sometimes create multiple sockets.  We've now refactored things to avoid
that, so it makes more sense for them to return the socket on success.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c | 14 +++++++-------
 tcp.c  |  7 ++-----
 udp.c  |  4 ++--
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/conf.c b/conf.c
index cc3c20a9..5db49653 100644
--- a/conf.c
+++ b/conf.c
@@ -149,7 +149,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 {
 	bool bound_one = false;
 	unsigned i;
-	int ret;
+	int fd;
 
 	if (first == 0) {
 		die("Can't forward port 0 for option '-%c %s'",
@@ -185,23 +185,23 @@ static void conf_ports_range_except(const struct ctx *c, char optname,
 		fwd->delta[i] = to - first;
 
 		if (optname == 't')
-			ret = tcp_listen(c, PIF_HOST, addr, ifname, i);
+			fd = tcp_listen(c, PIF_HOST, addr, ifname, i);
 		else if (optname == 'u')
-			ret = udp_listen(c, PIF_HOST, addr, ifname, i);
+			fd = udp_listen(c, PIF_HOST, addr, ifname, i);
 		else
 			/* No way to check in advance for -T and -U */
-			ret = 0;
+			fd = 0;
 
-		if (ret == -ENFILE || ret == -EMFILE) {
+		if (fd == -ENFILE || fd == -EMFILE) {
 			die("Can't open enough sockets for port specifier: %s",
 			    optarg);
 		}
 
-		if (!ret) {
+		if (fd >= 0) {
 			bound_one = true;
 		} else if (!weak) {
 			die("Failed to bind port %u (%s) for option '-%c %s'",
-			    i, strerror_(-ret), optname, optarg);
+			    i, strerror_(-fd), optname, optarg);
 		}
 	}
 
diff --git a/tcp.c b/tcp.c
index 67007c05..80b47f3c 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2680,7 +2680,7 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref,
  * @ifname:	Name of interface to bind to, NULL for any
  * @port:	Port, host order
  *
- * Return: 0 on success, negative error code on failure
+ * Return: Socket fd on success, negative error code on failure
  */
 int tcp_listen(const struct ctx *c, uint8_t pif,
 	       const union inany_addr *addr, const char *ifname, in_port_t port)
@@ -2729,10 +2729,7 @@ int tcp_listen(const struct ctx *c, uint8_t pif,
 			socks[port][V6] = s < 0 ? -1 : s;
 	}
 
-	if (s < 0)
-		return s;
-
-	return 0;
+	return s;
 }
 
 /**
diff --git a/udp.c b/udp.c
index 8cfa1e1f..63dc7c25 100644
--- a/udp.c
+++ b/udp.c
@@ -1136,7 +1136,7 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
- * Return: 0 on success, negative error code on failure
+ * Return: Socket fd on success, negative error code on failure
  */
 int udp_listen(const struct ctx *c, uint8_t pif,
 	       const union inany_addr *addr, const char *ifname, in_port_t port)
@@ -1180,7 +1180,7 @@ int udp_listen(const struct ctx *c, uint8_t pif,
 	if (!addr || !inany_v4(addr))
 		socks[V6][port] = s < 0 ? -1 : s;
 
-	return s < 0 ? s : 0;
+	return s;
 }
 
 /**
-- 
2.52.0