public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
* [PATCH 0/3] Fail gracefully on too many open files
@ 2023-03-08 12:33 Stefano Brivio
  2023-03-08 12:33 ` [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up Stefano Brivio
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Stefano Brivio @ 2023-03-08 12:33 UTC (permalink / raw)
  To: passt-dev

If we can't bind a few ports out of a port specifier, fine, we don't
want to bother the user with that.

But running out of files on '-t all' or similar is something
different: patch 3/3 fixes this, 1/3 and 2/3 are preparation steps.

Stefano Brivio (3):
  tcp, udp, util: Pass socket creation errors all the way up
  tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init()
  conf: Terminate on EMFILE or ENFILE on sockets for port mapping

 conf.c | 36 +++++++++++++++++++++++++++++-------
 tcp.c  | 29 ++++++++++++++---------------
 udp.c  | 44 +++++++++++++++++++++-----------------------
 util.c | 31 ++++++++++++++++++-------------
 4 files changed, 82 insertions(+), 58 deletions(-)

-- 
2.39.2


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up
  2023-03-08 12:33 [PATCH 0/3] Fail gracefully on too many open files Stefano Brivio
@ 2023-03-08 12:33 ` Stefano Brivio
  2023-03-08 22:16   ` David Gibson
  2023-03-08 12:33 ` [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init() Stefano Brivio
  2023-03-08 12:33 ` [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping Stefano Brivio
  2 siblings, 1 reply; 7+ messages in thread
From: Stefano Brivio @ 2023-03-08 12:33 UTC (permalink / raw)
  To: passt-dev

...starting from sock_l4(), pass negative error (errno) codes instead
of -1. They will only be used in two commits from now, no functional
changes intended here.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c  | 22 ++++++++++++----------
 udp.c  | 18 +++++++++---------
 util.c | 31 ++++++++++++++++++-------------
 3 files changed, 39 insertions(+), 32 deletions(-)

diff --git a/tcp.c b/tcp.c
index 96ca5c7..e209483 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2955,7 +2955,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
  * @addr:	Pointer to address for binding, NULL if not configured
  * @ifname:	Name of interface to bind to, NULL if not configured
  *
- * Return: fd for the new listening socket, or -1 on failure
+ * Return: fd for the new listening socket, negative error code on failure
  */
 static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
 			    const struct in_addr *addr, const char *ifname)
@@ -2968,13 +2968,13 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
 
 	if (c->tcp.fwd_in.mode == FWD_AUTO) {
 		if (af == AF_INET  || af == AF_UNSPEC)
-			tcp_sock_init_ext[port][V4] = s;
+			tcp_sock_init_ext[port][V4] = s < 0 ? -1 : s;
 		if (af == AF_INET6 || af == AF_UNSPEC)
-			tcp_sock_init_ext[port][V6] = s;
+			tcp_sock_init_ext[port][V6] = s < 0 ? -1 : s;
 	}
 
 	if (s < 0)
-		return -1;
+		return s;
 
 	tcp_sock_set_bufsize(c, s);
 	return s;
@@ -2988,12 +2988,12 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
- * Return: 0 on (partial) success, -1 on (complete) failure
+ * Return: 0 on (partial) success, negative error code on (complete) failure
  */
 int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
 		  const char *ifname, in_port_t port)
 {
-	int ret = 0;
+	int ret = 0, af_ret;
 
 	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
 		/* Attempt to get a dual stack socket */
@@ -3002,13 +3002,15 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
 
 	/* Otherwise create a socket per IP version */
 	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
-		if (tcp_sock_init_af(c, AF_INET, port, addr, ifname) < 0)
-			ret = -1;
+		af_ret = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
+		if (af_ret < 0)
+			ret = af_ret;
 	}
 
 	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
-		if (tcp_sock_init_af(c, AF_INET6, port, addr, ifname) < 0)
-			ret = -1;
+		af_ret = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
+		if (af_ret < 0)
+			ret = af_ret;
 	}
 
 	return ret;
diff --git a/udp.c b/udp.c
index 1077cde..41e0afd 100644
--- a/udp.c
+++ b/udp.c
@@ -977,7 +977,7 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
  * @ifname:	Name of interface to bind to, NULL if not configured
  * @port:	Port, host order
  *
- * Return: 0 on (partial) success, -1 on (complete) failure
+ * Return: 0 on (partial) success, negative error code on (complete) failure
  */
 int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		  const void *addr, const char *ifname, in_port_t port)
@@ -1002,19 +1002,19 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
 				    port, uref.u32);
 
-			udp_tap_map[V4][uref.udp.port].sock = s;
-			udp_splice_init[V4][port].sock = s;
+			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
+			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
 		} else {
 			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
 			uref.udp.ns = true;
 
 			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
 				    ifname, port, uref.u32);
-			udp_splice_ns[V4][port].sock = s;
+			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
 		}
 
 		if (s < 0)
-			ret = -1;
+			ret = s;
 	}
 
 	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
@@ -1026,18 +1026,18 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
 				    port, uref.u32);
 
-			udp_tap_map[V6][uref.udp.port].sock = s;
-			udp_splice_init[V6][port].sock = s;
+			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
+			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
 		} else {
 			uref.udp.ns = true;
 
 			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
 				    ifname, port, uref.u32);
-			udp_splice_ns[V6][port].sock = s;
+			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
 		}
 
 		if (s < 0)
-			ret = -1;
+			ret = s;
 	}
 
 	return ret;
diff --git a/util.c b/util.c
index 484889b..fddb5ed 100644
--- a/util.c
+++ b/util.c
@@ -96,7 +96,7 @@ found:
  * @port:	Port, host order
  * @data:	epoll reference portion for protocol handlers
  *
- * Return: newly created socket, -1 on error
+ * Return: newly created socket, negative error code on failure
  */
 int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
@@ -115,16 +115,16 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	};
 	const struct sockaddr *sa;
 	bool dual_stack = false;
+	int fd, sl, y = 1, ret;
 	struct epoll_event ev;
-	int fd, sl, y = 1;
 
 	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP &&
 	    proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6)
-		return -1;	/* Not implemented. */
+		return -EPFNOSUPPORT;	/* Not implemented. */
 
 	if (af == AF_UNSPEC) {
 		if (!DUAL_STACK_SOCKETS || bind_addr)
-			return -1;
+			return -EINVAL;
 		dual_stack = true;
 		af = AF_INET6;
 	}
@@ -134,14 +134,15 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	else
 		fd = socket(af, SOCK_DGRAM | SOCK_NONBLOCK, proto);
 
+	ret = -errno;
 	if (fd < 0) {
-		warn("L4 socket: %s", strerror(errno));
-		return -1;
+		warn("L4 socket: %s", strerror(-ret));
+		return ret;
 	}
 
 	if (fd > SOCKET_MAX) {
 		close(fd);
-		return -1;
+		return -EBADF;
 	}
 
 	ref.r.s = fd;
@@ -186,10 +187,11 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		 */
 		if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE,
 			       ifname, strlen(ifname))) {
+			ret = -errno;
 			warn("Can't bind socket for %s port %u to %s, closing",
 			     ip_proto_str[proto], port, ifname);
 			close(fd);
-			return -1;
+			return ret;
 		}
 	}
 
@@ -200,22 +202,25 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		 * broken SELinux policy, see icmp_tap_handler().
 		 */
 		if (proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6) {
+			ret = -errno;
 			close(fd);
-			return -1;
+			return ret;
 		}
 	}
 
 	if (proto == IPPROTO_TCP && listen(fd, 128) < 0) {
-		warn("TCP socket listen: %s", strerror(errno));
+		ret = -errno;
+		warn("TCP socket listen: %s", strerror(-ret));
 		close(fd);
-		return -1;
+		return ret;
 	}
 
 	ev.events = EPOLLIN;
 	ev.data.u64 = ref.u64;
 	if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, fd, &ev) == -1) {
-		warn("L4 epoll_ctl: %s", strerror(errno));
-		return -1;
+		ret = -errno;
+		warn("L4 epoll_ctl: %s", strerror(-ret));
+		return ret;
 	}
 
 	return fd;
-- 
@@ -96,7 +96,7 @@ found:
  * @port:	Port, host order
  * @data:	epoll reference portion for protocol handlers
  *
- * Return: newly created socket, -1 on error
+ * Return: newly created socket, negative error code on failure
  */
 int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	    const void *bind_addr, const char *ifname, uint16_t port,
@@ -115,16 +115,16 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	};
 	const struct sockaddr *sa;
 	bool dual_stack = false;
+	int fd, sl, y = 1, ret;
 	struct epoll_event ev;
-	int fd, sl, y = 1;
 
 	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP &&
 	    proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6)
-		return -1;	/* Not implemented. */
+		return -EPFNOSUPPORT;	/* Not implemented. */
 
 	if (af == AF_UNSPEC) {
 		if (!DUAL_STACK_SOCKETS || bind_addr)
-			return -1;
+			return -EINVAL;
 		dual_stack = true;
 		af = AF_INET6;
 	}
@@ -134,14 +134,15 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 	else
 		fd = socket(af, SOCK_DGRAM | SOCK_NONBLOCK, proto);
 
+	ret = -errno;
 	if (fd < 0) {
-		warn("L4 socket: %s", strerror(errno));
-		return -1;
+		warn("L4 socket: %s", strerror(-ret));
+		return ret;
 	}
 
 	if (fd > SOCKET_MAX) {
 		close(fd);
-		return -1;
+		return -EBADF;
 	}
 
 	ref.r.s = fd;
@@ -186,10 +187,11 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		 */
 		if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE,
 			       ifname, strlen(ifname))) {
+			ret = -errno;
 			warn("Can't bind socket for %s port %u to %s, closing",
 			     ip_proto_str[proto], port, ifname);
 			close(fd);
-			return -1;
+			return ret;
 		}
 	}
 
@@ -200,22 +202,25 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		 * broken SELinux policy, see icmp_tap_handler().
 		 */
 		if (proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6) {
+			ret = -errno;
 			close(fd);
-			return -1;
+			return ret;
 		}
 	}
 
 	if (proto == IPPROTO_TCP && listen(fd, 128) < 0) {
-		warn("TCP socket listen: %s", strerror(errno));
+		ret = -errno;
+		warn("TCP socket listen: %s", strerror(-ret));
 		close(fd);
-		return -1;
+		return ret;
 	}
 
 	ev.events = EPOLLIN;
 	ev.data.u64 = ref.u64;
 	if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, fd, &ev) == -1) {
-		warn("L4 epoll_ctl: %s", strerror(errno));
-		return -1;
+		ret = -errno;
+		warn("L4 epoll_ctl: %s", strerror(-ret));
+		return ret;
 	}
 
 	return fd;
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init()
  2023-03-08 12:33 [PATCH 0/3] Fail gracefully on too many open files Stefano Brivio
  2023-03-08 12:33 ` [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up Stefano Brivio
@ 2023-03-08 12:33 ` Stefano Brivio
  2023-03-08 22:43   ` David Gibson
  2023-03-08 12:33 ` [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping Stefano Brivio
  2 siblings, 1 reply; 7+ messages in thread
From: Stefano Brivio @ 2023-03-08 12:33 UTC (permalink / raw)
  To: passt-dev

The comments say we should return 0 on partial success, and an error
code on complete failure. Rationale: if the user configures a port
forwarding, and we succeed to bind that port for IPv4 or IPv6 only,
that might actually be what the user intended.

Adjust the two functions to reflect the comments.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tcp.c | 21 +++++++++------------
 udp.c | 30 ++++++++++++++----------------
 2 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/tcp.c b/tcp.c
index e209483..a29e387 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2993,7 +2993,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
 int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
 		  const char *ifname, in_port_t port)
 {
-	int ret = 0, af_ret;
+	int r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
 
 	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
 		/* Attempt to get a dual stack socket */
@@ -3001,19 +3001,16 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
 			return 0;
 
 	/* Otherwise create a socket per IP version */
-	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
-		af_ret = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
-		if (af_ret < 0)
-			ret = af_ret;
-	}
+	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4)
+		r4 = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
 
-	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
-		af_ret = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
-		if (af_ret < 0)
-			ret = af_ret;
-	}
+	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)
+		r6 = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
 
-	return ret;
+	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
+		return 0;
+
+	return r4 < 0 ? r4 : r6;
 }
 
 /**
diff --git a/udp.c b/udp.c
index 41e0afd..a5f7537 100644
--- a/udp.c
+++ b/udp.c
@@ -983,7 +983,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		  const void *addr, const char *ifname, in_port_t port)
 {
 	union udp_epoll_ref uref = { .u32 = 0 };
-	int s, ret = 0;
+	int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
 
 	if (ns) {
 		uref.udp.port = (in_port_t)(port +
@@ -999,8 +999,8 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		uref.udp.orig = true;
 
 		if (!ns) {
-			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
-				    port, uref.u32);
+			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
+					 ifname, port, uref.u32);
 
 			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
@@ -1008,13 +1008,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
 			uref.udp.ns = true;
 
-			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
-				    ifname, port, uref.u32);
+			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
+					 ifname, port, uref.u32);
 			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
 		}
-
-		if (s < 0)
-			ret = s;
 	}
 
 	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
@@ -1023,24 +1020,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		uref.udp.orig = true;
 
 		if (!ns) {
-			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
-				    port, uref.u32);
+			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
+					 ifname, port, uref.u32);
 
 			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
 		} else {
 			uref.udp.ns = true;
 
-			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
-				    ifname, port, uref.u32);
+			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
+					 &in6addr_loopback,
+					 ifname, port, uref.u32);
 			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
 		}
-
-		if (s < 0)
-			ret = s;
 	}
 
-	return ret;
+	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
+		return 0;
+
+	return r4 < 0 ? r4 : r6;
 }
 
 /**
-- 
@@ -983,7 +983,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		  const void *addr, const char *ifname, in_port_t port)
 {
 	union udp_epoll_ref uref = { .u32 = 0 };
-	int s, ret = 0;
+	int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
 
 	if (ns) {
 		uref.udp.port = (in_port_t)(port +
@@ -999,8 +999,8 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		uref.udp.orig = true;
 
 		if (!ns) {
-			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
-				    port, uref.u32);
+			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
+					 ifname, port, uref.u32);
 
 			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
@@ -1008,13 +1008,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
 			uref.udp.ns = true;
 
-			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
-				    ifname, port, uref.u32);
+			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
+					 ifname, port, uref.u32);
 			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
 		}
-
-		if (s < 0)
-			ret = s;
 	}
 
 	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
@@ -1023,24 +1020,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
 		uref.udp.orig = true;
 
 		if (!ns) {
-			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
-				    port, uref.u32);
+			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
+					 ifname, port, uref.u32);
 
 			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
 			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
 		} else {
 			uref.udp.ns = true;
 
-			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
-				    ifname, port, uref.u32);
+			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
+					 &in6addr_loopback,
+					 ifname, port, uref.u32);
 			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
 		}
-
-		if (s < 0)
-			ret = s;
 	}
 
-	return ret;
+	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
+		return 0;
+
+	return r4 < 0 ? r4 : r6;
 }
 
 /**
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping
  2023-03-08 12:33 [PATCH 0/3] Fail gracefully on too many open files Stefano Brivio
  2023-03-08 12:33 ` [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up Stefano Brivio
  2023-03-08 12:33 ` [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init() Stefano Brivio
@ 2023-03-08 12:33 ` Stefano Brivio
  2023-03-08 22:45   ` David Gibson
  2 siblings, 1 reply; 7+ messages in thread
From: Stefano Brivio @ 2023-03-08 12:33 UTC (permalink / raw)
  To: passt-dev

In general, we don't terminate or report failures if we fail to bind
to some ports out of a given port range specifier, to allow users to
conveniently specify big port ranges (or "all") without having to
care about ports that might already be in use.

However, running out of the open file descriptors quota is a
different story: we can't do what the user requested in a very
substantial way.

For example, if the user specifies '-t all' and we can only bind
1024 sockets, the behaviour is rather unexpected.

Fail whenever socket creation returns -ENFILE or -EMFILE.

Link: https://bugs.passt.top/show_bug.cgi?id=27
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/conf.c b/conf.c
index 582c391..ee56501 100644
--- a/conf.c
+++ b/conf.c
@@ -184,6 +184,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	bool exclude_only = true, bound_one = false;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
 	sa_family_t af = AF_UNSPEC;
+	int ret;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -218,11 +219,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (optname == 't') {
-				if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i))
+				ret = tcp_sock_init(c, AF_UNSPEC, NULL, NULL,
+						    i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
-						   i))
+				ret = udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
+						    i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			}
 		}
@@ -303,10 +311,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			bitmap_set(fwd->map, i);
 
 			if (optname == 't') {
-				if (!tcp_sock_init(c, af, addr, ifname, i))
+				ret = tcp_sock_init(c, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, af, addr, ifname, i))
+				ret = udp_sock_init(c, 0, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else {
 				/* No way to check in advance for -T and -U */
@@ -358,10 +372,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			fwd->delta[i] = mapped_range.first - orig_range.first;
 
 			if (optname == 't') {
-				if (!tcp_sock_init(c, af, addr, ifname, i))
+				ret = tcp_sock_init(c, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, af, addr, ifname, i))
+				ret = udp_sock_init(c, 0, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else {
 				/* No way to check in advance for -T and -U */
@@ -374,6 +394,8 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		goto bind_fail;
 
 	return;
+enfile:
+	die("Can't open enough sockets for port specifier: %s", optarg);
 bad:
 	die("Invalid port specifier %s", optarg);
 overlap:
-- 
@@ -184,6 +184,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 	bool exclude_only = true, bound_one = false;
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
 	sa_family_t af = AF_UNSPEC;
+	int ret;
 
 	if (!strcmp(optarg, "none")) {
 		if (fwd->mode)
@@ -218,11 +219,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 
 		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
 			if (optname == 't') {
-				if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i))
+				ret = tcp_sock_init(c, AF_UNSPEC, NULL, NULL,
+						    i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
-						   i))
+				ret = udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
+						    i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			}
 		}
@@ -303,10 +311,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			bitmap_set(fwd->map, i);
 
 			if (optname == 't') {
-				if (!tcp_sock_init(c, af, addr, ifname, i))
+				ret = tcp_sock_init(c, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, af, addr, ifname, i))
+				ret = udp_sock_init(c, 0, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else {
 				/* No way to check in advance for -T and -U */
@@ -358,10 +372,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 			fwd->delta[i] = mapped_range.first - orig_range.first;
 
 			if (optname == 't') {
-				if (!tcp_sock_init(c, af, addr, ifname, i))
+				ret = tcp_sock_init(c, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else if (optname == 'u') {
-				if (!udp_sock_init(c, 0, af, addr, ifname, i))
+				ret = udp_sock_init(c, 0, af, addr, ifname, i);
+				if (ret == -ENFILE || ret == -EMFILE)
+					goto enfile;
+				if (!ret)
 					bound_one = true;
 			} else {
 				/* No way to check in advance for -T and -U */
@@ -374,6 +394,8 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
 		goto bind_fail;
 
 	return;
+enfile:
+	die("Can't open enough sockets for port specifier: %s", optarg);
 bad:
 	die("Invalid port specifier %s", optarg);
 overlap:
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up
  2023-03-08 12:33 ` [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up Stefano Brivio
@ 2023-03-08 22:16   ` David Gibson
  0 siblings, 0 replies; 7+ messages in thread
From: David Gibson @ 2023-03-08 22:16 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

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

On Wed, Mar 08, 2023 at 01:33:46PM +0100, Stefano Brivio wrote:
> ...starting from sock_l4(), pass negative error (errno) codes instead
> of -1. They will only be used in two commits from now, no functional
> changes intended here.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c  | 22 ++++++++++++----------
>  udp.c  | 18 +++++++++---------
>  util.c | 31 ++++++++++++++++++-------------
>  3 files changed, 39 insertions(+), 32 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index 96ca5c7..e209483 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2955,7 +2955,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events,
>   * @addr:	Pointer to address for binding, NULL if not configured
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   *
> - * Return: fd for the new listening socket, or -1 on failure
> + * Return: fd for the new listening socket, negative error code on failure
>   */
>  static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  			    const struct in_addr *addr, const char *ifname)
> @@ -2968,13 +2968,13 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  
>  	if (c->tcp.fwd_in.mode == FWD_AUTO) {
>  		if (af == AF_INET  || af == AF_UNSPEC)
> -			tcp_sock_init_ext[port][V4] = s;
> +			tcp_sock_init_ext[port][V4] = s < 0 ? -1 : s;
>  		if (af == AF_INET6 || af == AF_UNSPEC)
> -			tcp_sock_init_ext[port][V6] = s;
> +			tcp_sock_init_ext[port][V6] = s < 0 ? -1 : s;
>  	}
>  
>  	if (s < 0)
> -		return -1;
> +		return s;
>  
>  	tcp_sock_set_bufsize(c, s);
>  	return s;
> @@ -2988,12 +2988,12 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   * @port:	Port, host order
>   *
> - * Return: 0 on (partial) success, -1 on (complete) failure
> + * Return: 0 on (partial) success, negative error code on (complete) failure
>   */
>  int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  		  const char *ifname, in_port_t port)
>  {
> -	int ret = 0;
> +	int ret = 0, af_ret;
>  
>  	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
>  		/* Attempt to get a dual stack socket */
> @@ -3002,13 +3002,15 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  
>  	/* Otherwise create a socket per IP version */
>  	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
> -		if (tcp_sock_init_af(c, AF_INET, port, addr, ifname) < 0)
> -			ret = -1;
> +		af_ret = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
> +		if (af_ret < 0)
> +			ret = af_ret;
>  	}
>  
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> -		if (tcp_sock_init_af(c, AF_INET6, port, addr, ifname) < 0)
> -			ret = -1;
> +		af_ret = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
> +		if (af_ret < 0)
> +			ret = af_ret;
>  	}
>  
>  	return ret;
> diff --git a/udp.c b/udp.c
> index 1077cde..41e0afd 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -977,7 +977,7 @@ int udp_tap_handler(struct ctx *c, int af, const void *addr,
>   * @ifname:	Name of interface to bind to, NULL if not configured
>   * @port:	Port, host order
>   *
> - * Return: 0 on (partial) success, -1 on (complete) failure
> + * Return: 0 on (partial) success, negative error code on (complete) failure
>   */
>  int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		  const void *addr, const char *ifname, in_port_t port)
> @@ -1002,19 +1002,19 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
>  				    port, uref.u32);
>  
> -			udp_tap_map[V4][uref.udp.port].sock = s;
> -			udp_splice_init[V4][port].sock = s;
> +			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
> +			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
>  		} else {
>  			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
>  			uref.udp.ns = true;
>  
>  			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
>  				    ifname, port, uref.u32);
> -			udp_splice_ns[V4][port].sock = s;
> +			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
>  		}
>  
>  		if (s < 0)
> -			ret = -1;
> +			ret = s;
>  	}
>  
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> @@ -1026,18 +1026,18 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
>  				    port, uref.u32);
>  
> -			udp_tap_map[V6][uref.udp.port].sock = s;
> -			udp_splice_init[V6][port].sock = s;
> +			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
> +			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
>  		} else {
>  			uref.udp.ns = true;
>  
>  			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
>  				    ifname, port, uref.u32);
> -			udp_splice_ns[V6][port].sock = s;
> +			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
>  		}
>  
>  		if (s < 0)
> -			ret = -1;
> +			ret = s;
>  	}
>  
>  	return ret;
> diff --git a/util.c b/util.c
> index 484889b..fddb5ed 100644
> --- a/util.c
> +++ b/util.c
> @@ -96,7 +96,7 @@ found:
>   * @port:	Port, host order
>   * @data:	epoll reference portion for protocol handlers
>   *
> - * Return: newly created socket, -1 on error
> + * Return: newly created socket, negative error code on failure
>   */
>  int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  	    const void *bind_addr, const char *ifname, uint16_t port,
> @@ -115,16 +115,16 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  	};
>  	const struct sockaddr *sa;
>  	bool dual_stack = false;
> +	int fd, sl, y = 1, ret;
>  	struct epoll_event ev;
> -	int fd, sl, y = 1;
>  
>  	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP &&
>  	    proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6)
> -		return -1;	/* Not implemented. */
> +		return -EPFNOSUPPORT;	/* Not implemented. */
>  
>  	if (af == AF_UNSPEC) {
>  		if (!DUAL_STACK_SOCKETS || bind_addr)
> -			return -1;
> +			return -EINVAL;
>  		dual_stack = true;
>  		af = AF_INET6;
>  	}
> @@ -134,14 +134,15 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  	else
>  		fd = socket(af, SOCK_DGRAM | SOCK_NONBLOCK, proto);
>  
> +	ret = -errno;
>  	if (fd < 0) {
> -		warn("L4 socket: %s", strerror(errno));
> -		return -1;
> +		warn("L4 socket: %s", strerror(-ret));
> +		return ret;
>  	}
>  
>  	if (fd > SOCKET_MAX) {
>  		close(fd);
> -		return -1;
> +		return -EBADF;
>  	}
>  
>  	ref.r.s = fd;
> @@ -186,10 +187,11 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  		 */
>  		if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE,
>  			       ifname, strlen(ifname))) {
> +			ret = -errno;
>  			warn("Can't bind socket for %s port %u to %s, closing",
>  			     ip_proto_str[proto], port, ifname);
>  			close(fd);
> -			return -1;
> +			return ret;
>  		}
>  	}
>  
> @@ -200,22 +202,25 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
>  		 * broken SELinux policy, see icmp_tap_handler().
>  		 */
>  		if (proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6) {
> +			ret = -errno;
>  			close(fd);
> -			return -1;
> +			return ret;
>  		}
>  	}
>  
>  	if (proto == IPPROTO_TCP && listen(fd, 128) < 0) {
> -		warn("TCP socket listen: %s", strerror(errno));
> +		ret = -errno;
> +		warn("TCP socket listen: %s", strerror(-ret));
>  		close(fd);
> -		return -1;
> +		return ret;
>  	}
>  
>  	ev.events = EPOLLIN;
>  	ev.data.u64 = ref.u64;
>  	if (epoll_ctl(c->epollfd, EPOLL_CTL_ADD, fd, &ev) == -1) {
> -		warn("L4 epoll_ctl: %s", strerror(errno));
> -		return -1;
> +		ret = -errno;
> +		warn("L4 epoll_ctl: %s", strerror(-ret));
> +		return ret;
>  	}
>  
>  	return fd;

-- 
David Gibson			| 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init()
  2023-03-08 12:33 ` [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init() Stefano Brivio
@ 2023-03-08 22:43   ` David Gibson
  0 siblings, 0 replies; 7+ messages in thread
From: David Gibson @ 2023-03-08 22:43 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

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

On Wed, Mar 08, 2023 at 01:33:47PM +0100, Stefano Brivio wrote:
> The comments say we should return 0 on partial success, and an error
> code on complete failure. Rationale: if the user configures a port
> forwarding, and we succeed to bind that port for IPv4 or IPv6 only,
> that might actually be what the user intended.
> 
> Adjust the two functions to reflect the comments.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tcp.c | 21 +++++++++------------
>  udp.c | 30 ++++++++++++++----------------
>  2 files changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index e209483..a29e387 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2993,7 +2993,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port,
>  int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  		  const char *ifname, in_port_t port)
>  {
> -	int ret = 0, af_ret;
> +	int r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
>  
>  	if (af == AF_UNSPEC && c->ifi4 && c->ifi6)
>  		/* Attempt to get a dual stack socket */
> @@ -3001,19 +3001,16 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr,
>  			return 0;
>  
>  	/* Otherwise create a socket per IP version */
> -	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4) {
> -		af_ret = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
> -		if (af_ret < 0)
> -			ret = af_ret;
> -	}
> +	if ((af == AF_INET  || af == AF_UNSPEC) && c->ifi4)
> +		r4 = tcp_sock_init_af(c, AF_INET, port, addr, ifname);
>  
> -	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> -		af_ret = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
> -		if (af_ret < 0)
> -			ret = af_ret;
> -	}
> +	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6)
> +		r6 = tcp_sock_init_af(c, AF_INET6, port, addr, ifname);
>  
> -	return ret;
> +	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
> +		return 0;
> +
> +	return r4 < 0 ? r4 : r6;
>  }
>  
>  /**
> diff --git a/udp.c b/udp.c
> index 41e0afd..a5f7537 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -983,7 +983,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		  const void *addr, const char *ifname, in_port_t port)
>  {
>  	union udp_epoll_ref uref = { .u32 = 0 };
> -	int s, ret = 0;
> +	int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1;
>  
>  	if (ns) {
>  		uref.udp.port = (in_port_t)(port +
> @@ -999,8 +999,8 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		uref.udp.orig = true;
>  
>  		if (!ns) {
> -			s = sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname,
> -				    port, uref.u32);
> +			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, addr,
> +					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V4][uref.udp.port].sock = s < 0 ? -1 : s;
>  			udp_splice_init[V4][port].sock = s < 0 ? -1 : s;
> @@ -1008,13 +1008,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  			struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
>  			uref.udp.ns = true;
>  
> -			s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
> -				    ifname, port, uref.u32);
> +			r4 = s = sock_l4(c, AF_INET, IPPROTO_UDP, &loopback,
> +					 ifname, port, uref.u32);
>  			udp_splice_ns[V4][port].sock = s < 0 ? -1 : s;
>  		}
> -
> -		if (s < 0)
> -			ret = s;
>  	}
>  
>  	if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) {
> @@ -1023,24 +1020,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af,
>  		uref.udp.orig = true;
>  
>  		if (!ns) {
> -			s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname,
> -				    port, uref.u32);
> +			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP, addr,
> +					 ifname, port, uref.u32);
>  
>  			udp_tap_map[V6][uref.udp.port].sock = s < 0 ? -1 : s;
>  			udp_splice_init[V6][port].sock = s < 0 ? -1 : s;
>  		} else {
>  			uref.udp.ns = true;
>  
> -			s = sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback,
> -				    ifname, port, uref.u32);
> +			r6 = s = sock_l4(c, AF_INET6, IPPROTO_UDP,
> +					 &in6addr_loopback,
> +					 ifname, port, uref.u32);
>  			udp_splice_ns[V6][port].sock = s < 0 ? -1 : s;
>  		}
> -
> -		if (s < 0)
> -			ret = s;
>  	}
>  
> -	return ret;
> +	if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6))
> +		return 0;
> +
> +	return r4 < 0 ? r4 : r6;

Same comment (unless sock_l4() already looks for 

>  }
>  
>  /**

-- 
David Gibson			| 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping
  2023-03-08 12:33 ` [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping Stefano Brivio
@ 2023-03-08 22:45   ` David Gibson
  0 siblings, 0 replies; 7+ messages in thread
From: David Gibson @ 2023-03-08 22:45 UTC (permalink / raw)
  To: Stefano Brivio; +Cc: passt-dev

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

On Wed, Mar 08, 2023 at 01:33:48PM +0100, Stefano Brivio wrote:
> In general, we don't terminate or report failures if we fail to bind
> to some ports out of a given port range specifier, to allow users to
> conveniently specify big port ranges (or "all") without having to
> care about ports that might already be in use.
> 
> However, running out of the open file descriptors quota is a
> different story: we can't do what the user requested in a very
> substantial way.
> 
> For example, if the user specifies '-t all' and we can only bind
> 1024 sockets, the behaviour is rather unexpected.
> 
> Fail whenever socket creation returns -ENFILE or -EMFILE.
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=27
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  conf.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 582c391..ee56501 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -184,6 +184,7 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  	bool exclude_only = true, bound_one = false;
>  	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
>  	sa_family_t af = AF_UNSPEC;
> +	int ret;
>  
>  	if (!strcmp(optarg, "none")) {
>  		if (fwd->mode)
> @@ -218,11 +219,18 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  
>  		for (i = 0; i < PORT_EPHEMERAL_MIN; i++) {
>  			if (optname == 't') {
> -				if (!tcp_sock_init(c, AF_UNSPEC, NULL, NULL, i))
> +				ret = tcp_sock_init(c, AF_UNSPEC, NULL, NULL,
> +						    i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			} else if (optname == 'u') {
> -				if (!udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
> -						   i))
> +				ret = udp_sock_init(c, 0, AF_UNSPEC, NULL, NULL,
> +						    i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			}
>  		}
> @@ -303,10 +311,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  			bitmap_set(fwd->map, i);
>  
>  			if (optname == 't') {
> -				if (!tcp_sock_init(c, af, addr, ifname, i))
> +				ret = tcp_sock_init(c, af, addr, ifname, i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			} else if (optname == 'u') {
> -				if (!udp_sock_init(c, 0, af, addr, ifname, i))
> +				ret = udp_sock_init(c, 0, af, addr, ifname, i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			} else {
>  				/* No way to check in advance for -T and -U */
> @@ -358,10 +372,16 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  			fwd->delta[i] = mapped_range.first - orig_range.first;
>  
>  			if (optname == 't') {
> -				if (!tcp_sock_init(c, af, addr, ifname, i))
> +				ret = tcp_sock_init(c, af, addr, ifname, i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			} else if (optname == 'u') {
> -				if (!udp_sock_init(c, 0, af, addr, ifname, i))
> +				ret = udp_sock_init(c, 0, af, addr, ifname, i);
> +				if (ret == -ENFILE || ret == -EMFILE)
> +					goto enfile;
> +				if (!ret)
>  					bound_one = true;
>  			} else {
>  				/* No way to check in advance for -T and -U */
> @@ -374,6 +394,8 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		goto bind_fail;
>  
>  	return;
> +enfile:
> +	die("Can't open enough sockets for port specifier: %s", optarg);
>  bad:
>  	die("Invalid port specifier %s", optarg);
>  overlap:

-- 
David Gibson			| 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 --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-08 22:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-08 12:33 [PATCH 0/3] Fail gracefully on too many open files Stefano Brivio
2023-03-08 12:33 ` [PATCH 1/3] tcp, udp, util: Pass socket creation errors all the way up Stefano Brivio
2023-03-08 22:16   ` David Gibson
2023-03-08 12:33 ` [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init() Stefano Brivio
2023-03-08 22:43   ` David Gibson
2023-03-08 12:33 ` [PATCH 3/3] conf: Terminate on EMFILE or ENFILE on sockets for port mapping Stefano Brivio
2023-03-08 22:45   ` David Gibson

Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

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