[PATCH v2 0/4] Clean up reset of tap connections

[PATCH v2 0/4] Clean up reset of tap connections

[David Gibson]

I ran into some mildly confusing stuff in the tap reset path while
working on the epoll rework.  Here are some fixes for it.

Changes since v1:
 * Two extra patches that further clean up the reset path

David Gibson (4):
  tap: Clean up tap reset path
  tap: More sensible behaviour for error on listening qemu socket
  tap: Fold reset handling into tap_handler_pasta()
  tap: Fold reset handling into tap_handler_passt()

 tap.c | 94 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

-- 
2.41.0

[PATCH v2 1/4] tap: Clean up tap reset path

[David Gibson]

In tap_handler() if we get an error on the tap device or socket, we use
tap_sock_init() to re-initialise it.  However, what we actually need for
this reset case has remarkably little in common with the case where we're
initialising for the first time:
    * Re-initialising the packet pools is unnecessary
    * The case of a passed in fd (--fd) isn't relevant
    * We don't even call this for pasta mode
    * We will never re-call tap_sock_unix_init() because we never clear
      fd_tap_listen

In fact the only thing we do in tap_sock_init() relevant to the reset case
is to remove the fd from the epoll and close it... which isn't used in the
first initialisation case.

So make a new tap_sock_reset() function just for this case, and simplify
tap_sock_init() slightly as being used only for the first time case.

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

diff --git a/tap.c b/tap.c
index e034f94..b4967d0 100644
--- a/tap.c
+++ b/tap.c
@@ -1236,19 +1236,14 @@ void tap_sock_init(struct ctx *c)
 		tap6_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz);
 	}
 
-	if (c->fd_tap != -1) {
-		if (c->one_off) {	/* Passed as --fd */
-			struct epoll_event ev = { 0 };
-
-			ev.data.fd = c->fd_tap;
-			ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
-			epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
-			return;
-		}
+	if (c->fd_tap != -1) { /* Passed as --fd */
+		struct epoll_event ev = { 0 };
+		ASSERT(c->one_off);
 
-		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-		close(c->fd_tap);
-		c->fd_tap = -1;
+		ev.data.fd = c->fd_tap;
+		ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
+		epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
+		return;
 	}
 
 	if (c->mode == MODE_PASST) {
@@ -1259,6 +1254,26 @@ void tap_sock_init(struct ctx *c)
 	}
 }
 
+/**
+ * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_reset(struct ctx *c)
+{
+	if (c->one_off) {
+		info("Client closed connection, exiting");
+		exit(EXIT_SUCCESS);
+	}
+
+	if (c->mode == MODE_PASTA)
+		die("Error on tap device, exiting");
+
+	/* Close the connected socket, wait for a new connection */
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
+	close(c->fd_tap);
+	c->fd_tap = -1;
+}
+
 /**
  * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor
  * @c:		Execution context
@@ -1276,15 +1291,6 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 
 	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
 	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
-	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) {
-		if (c->one_off) {
-			info("Client closed connection, exiting");
-			exit(EXIT_SUCCESS);
-		}
-
-		if (c->mode == MODE_PASTA)
-			die("Error on tap device, exiting");
-
-		tap_sock_init(c);
-	}
+	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
+		tap_sock_reset(c);
 }
-- 
@@ -1236,19 +1236,14 @@ void tap_sock_init(struct ctx *c)
 		tap6_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz);
 	}
 
-	if (c->fd_tap != -1) {
-		if (c->one_off) {	/* Passed as --fd */
-			struct epoll_event ev = { 0 };
-
-			ev.data.fd = c->fd_tap;
-			ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
-			epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
-			return;
-		}
+	if (c->fd_tap != -1) { /* Passed as --fd */
+		struct epoll_event ev = { 0 };
+		ASSERT(c->one_off);
 
-		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-		close(c->fd_tap);
-		c->fd_tap = -1;
+		ev.data.fd = c->fd_tap;
+		ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
+		epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev);
+		return;
 	}
 
 	if (c->mode == MODE_PASST) {
@@ -1259,6 +1254,26 @@ void tap_sock_init(struct ctx *c)
 	}
 }
 
+/**
+ * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_reset(struct ctx *c)
+{
+	if (c->one_off) {
+		info("Client closed connection, exiting");
+		exit(EXIT_SUCCESS);
+	}
+
+	if (c->mode == MODE_PASTA)
+		die("Error on tap device, exiting");
+
+	/* Close the connected socket, wait for a new connection */
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
+	close(c->fd_tap);
+	c->fd_tap = -1;
+}
+
 /**
  * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor
  * @c:		Execution context
@@ -1276,15 +1291,6 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 
 	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
 	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
-	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) {
-		if (c->one_off) {
-			info("Client closed connection, exiting");
-			exit(EXIT_SUCCESS);
-		}
-
-		if (c->mode == MODE_PASTA)
-			die("Error on tap device, exiting");
-
-		tap_sock_init(c);
-	}
+	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
+		tap_sock_reset(c);
 }
-- 
2.41.0

[PATCH v2 2/4] tap: More sensible behaviour for error on listening qemu socket

[David Gibson]

We call tap_sock_unix_new() to handle a new connection to the qemu socket
if we get an EPOLLIN event on c->fd_tap_listen.  If we get an error event
on c->fd_tap_listen instead, we'll fall through to the "tap reset" path.

However, this won't do anything useful for an error on the listening
socket, it will just close the already connected socket.  If we wanted to
handle errors on this socket, we'd need to do something different than for
an error on the connected socket.

So, change the logic to explicitly do nothing for any !EPOLLIN events on
the listening socket.

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

diff --git a/tap.c b/tap.c
index b4967d0..7e8b890 100644
--- a/tap.c
+++ b/tap.c
@@ -1284,8 +1284,9 @@ static void tap_sock_reset(struct ctx *c)
 void tap_handler(struct ctx *c, int fd, uint32_t events,
 		 const struct timespec *now)
 {
-	if (fd == c->fd_tap_listen && events == EPOLLIN) {
-		tap_sock_unix_new(c);
+	if (fd == c->fd_tap_listen) {
+		if (events == EPOLLIN)
+			tap_sock_unix_new(c);
 		return;
 	}
 
-- 
@@ -1284,8 +1284,9 @@ static void tap_sock_reset(struct ctx *c)
 void tap_handler(struct ctx *c, int fd, uint32_t events,
 		 const struct timespec *now)
 {
-	if (fd == c->fd_tap_listen && events == EPOLLIN) {
-		tap_sock_unix_new(c);
+	if (fd == c->fd_tap_listen) {
+		if (events == EPOLLIN)
+			tap_sock_unix_new(c);
 		return;
 	}
 
-- 
2.41.0

[PATCH v2 3/4] tap: Fold reset handling into tap_handler_pasta()

[David Gibson]

If tap_handler_pasta() fails, we reset the connection.  But in the case of
pasta the "reset" is just a fatal error.  Fold the die() calls directly
into tap_handler_pasta() for simplicity.

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

diff --git a/tap.c b/tap.c
index 7e8b890..e0a05b2 100644
--- a/tap.c
+++ b/tap.c
@@ -982,15 +982,18 @@ next:
 /**
  * tap_handler_pasta() - Packet handler for tuntap file descriptor
  * @c:		Execution context
+ * @events:	epoll events
  * @now:	Current timestamp
- *
- * Return: -ECONNRESET on receive error, 0 otherwise
  */
-static int tap_handler_pasta(struct ctx *c, const struct timespec *now)
+static void tap_handler_pasta(struct ctx *c, uint32_t events,
+			      const struct timespec *now)
 {
 	ssize_t n, len;
 	int ret;
 
+	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
+		die("Disconnect event on /dev/net/tun device, exiting");
+
 redo:
 	n = 0;
 
@@ -1037,15 +1040,12 @@ restart:
 	tap6_handler(c, pool_tap6, now);
 
 	if (len > 0 || ret == EAGAIN)
-		return 0;
+		return;
 
 	if (n == TAP_BUF_BYTES)
 		goto redo;
 
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-	close(c->fd_tap);
-
-	return -ECONNRESET;
+	die("Error on tap device, exiting");
 }
 
 /**
@@ -1265,9 +1265,6 @@ static void tap_sock_reset(struct ctx *c)
 		exit(EXIT_SUCCESS);
 	}
 
-	if (c->mode == MODE_PASTA)
-		die("Error on tap device, exiting");
-
 	/* Close the connected socket, wait for a new connection */
 	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
 	close(c->fd_tap);
@@ -1290,8 +1287,11 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 		return;
 	}
 
-	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
-	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
-	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
-		tap_sock_reset(c);
+	if (c->mode == MODE_PASST) {
+		if (tap_handler_passt(c, now) ||
+		    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
+			tap_sock_reset(c);
+	} else if (c->mode == MODE_PASTA) {
+		tap_handler_pasta(c, events, now);
+	}
 }
-- 
@@ -982,15 +982,18 @@ next:
 /**
  * tap_handler_pasta() - Packet handler for tuntap file descriptor
  * @c:		Execution context
+ * @events:	epoll events
  * @now:	Current timestamp
- *
- * Return: -ECONNRESET on receive error, 0 otherwise
  */
-static int tap_handler_pasta(struct ctx *c, const struct timespec *now)
+static void tap_handler_pasta(struct ctx *c, uint32_t events,
+			      const struct timespec *now)
 {
 	ssize_t n, len;
 	int ret;
 
+	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))
+		die("Disconnect event on /dev/net/tun device, exiting");
+
 redo:
 	n = 0;
 
@@ -1037,15 +1040,12 @@ restart:
 	tap6_handler(c, pool_tap6, now);
 
 	if (len > 0 || ret == EAGAIN)
-		return 0;
+		return;
 
 	if (n == TAP_BUF_BYTES)
 		goto redo;
 
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-	close(c->fd_tap);
-
-	return -ECONNRESET;
+	die("Error on tap device, exiting");
 }
 
 /**
@@ -1265,9 +1265,6 @@ static void tap_sock_reset(struct ctx *c)
 		exit(EXIT_SUCCESS);
 	}
 
-	if (c->mode == MODE_PASTA)
-		die("Error on tap device, exiting");
-
 	/* Close the connected socket, wait for a new connection */
 	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
 	close(c->fd_tap);
@@ -1290,8 +1287,11 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 		return;
 	}
 
-	if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) ||
-	    (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) ||
-	    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
-		tap_sock_reset(c);
+	if (c->mode == MODE_PASST) {
+		if (tap_handler_passt(c, now) ||
+		    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
+			tap_sock_reset(c);
+	} else if (c->mode == MODE_PASTA) {
+		tap_handler_pasta(c, events, now);
+	}
 }
-- 
2.41.0

[PATCH v2 4/4] tap: Fold reset handling into tap_handler_passt()

[David Gibson]

We call tap_sock_reset() if tap_handler_passt() fails, or if we get an
error event on the socket.  Fold that logic into tap_handler() passt itself
which simplifies the caller.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tap.c | 63 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/tap.c b/tap.c
index e0a05b2..c33c6c7 100644
--- a/tap.c
+++ b/tap.c
@@ -891,19 +891,41 @@ append:
 	return in->count;
 }
 
+/**
+ * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_reset(struct ctx *c)
+{
+	if (c->one_off) {
+		info("Client closed connection, exiting");
+		exit(EXIT_SUCCESS);
+	}
+
+	/* Close the connected socket, wait for a new connection */
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
+	close(c->fd_tap);
+	c->fd_tap = -1;
+}
+
 /**
  * tap_handler_passt() - Packet handler for AF_UNIX file descriptor
  * @c:		Execution context
+ * @events:	epoll events
  * @now:	Current timestamp
- *
- * Return: -ECONNRESET on receive error, 0 otherwise
  */
-static int tap_handler_passt(struct ctx *c, const struct timespec *now)
+static void tap_handler_passt(struct ctx *c, uint32_t events,
+			      const struct timespec *now)
 {
 	struct ethhdr *eh;
 	ssize_t n, rem;
 	char *p;
 
+	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) {
+		tap_sock_reset(c);
+		return;
+	}
+
 redo:
 	p = pkt_buf;
 	rem = 0;
@@ -914,12 +936,13 @@ redo:
 	n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT);
 	if (n < 0) {
 		if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK)
-			return 0;
+			return;
 
 		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
 		close(c->fd_tap);
 
-		return -ECONNRESET;
+		tap_sock_reset(c);
+		return;
 	}
 
 	while (n > (ssize_t)sizeof(uint32_t)) {
@@ -934,7 +957,7 @@ redo:
 		if (len > n) {
 			rem = recv(c->fd_tap, p + n, len - n, 0);
 			if ((n += rem) != len)
-				return 0;
+				return;
 		}
 
 		/* Complete the partial read above before discarding a malformed
@@ -975,8 +998,6 @@ next:
 	/* We can't use EPOLLET otherwise. */
 	if (rem)
 		goto redo;
-
-	return 0;
 }
 
 /**
@@ -1254,23 +1275,6 @@ void tap_sock_init(struct ctx *c)
 	}
 }
 
-/**
- * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
- * @c:		Execution context
- */
-static void tap_sock_reset(struct ctx *c)
-{
-	if (c->one_off) {
-		info("Client closed connection, exiting");
-		exit(EXIT_SUCCESS);
-	}
-
-	/* Close the connected socket, wait for a new connection */
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-	close(c->fd_tap);
-	c->fd_tap = -1;
-}
-
 /**
  * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor
  * @c:		Execution context
@@ -1287,11 +1291,8 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 		return;
 	}
 
-	if (c->mode == MODE_PASST) {
-		if (tap_handler_passt(c, now) ||
-		    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
-			tap_sock_reset(c);
-	} else if (c->mode == MODE_PASTA) {
+	if (c->mode == MODE_PASST)
+		tap_handler_passt(c, events, now);
+	else if (c->mode == MODE_PASTA)
 		tap_handler_pasta(c, events, now);
-	}
 }
-- 
@@ -891,19 +891,41 @@ append:
 	return in->count;
 }
 
+/**
+ * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_reset(struct ctx *c)
+{
+	if (c->one_off) {
+		info("Client closed connection, exiting");
+		exit(EXIT_SUCCESS);
+	}
+
+	/* Close the connected socket, wait for a new connection */
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
+	close(c->fd_tap);
+	c->fd_tap = -1;
+}
+
 /**
  * tap_handler_passt() - Packet handler for AF_UNIX file descriptor
  * @c:		Execution context
+ * @events:	epoll events
  * @now:	Current timestamp
- *
- * Return: -ECONNRESET on receive error, 0 otherwise
  */
-static int tap_handler_passt(struct ctx *c, const struct timespec *now)
+static void tap_handler_passt(struct ctx *c, uint32_t events,
+			      const struct timespec *now)
 {
 	struct ethhdr *eh;
 	ssize_t n, rem;
 	char *p;
 
+	if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) {
+		tap_sock_reset(c);
+		return;
+	}
+
 redo:
 	p = pkt_buf;
 	rem = 0;
@@ -914,12 +936,13 @@ redo:
 	n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT);
 	if (n < 0) {
 		if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK)
-			return 0;
+			return;
 
 		epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
 		close(c->fd_tap);
 
-		return -ECONNRESET;
+		tap_sock_reset(c);
+		return;
 	}
 
 	while (n > (ssize_t)sizeof(uint32_t)) {
@@ -934,7 +957,7 @@ redo:
 		if (len > n) {
 			rem = recv(c->fd_tap, p + n, len - n, 0);
 			if ((n += rem) != len)
-				return 0;
+				return;
 		}
 
 		/* Complete the partial read above before discarding a malformed
@@ -975,8 +998,6 @@ next:
 	/* We can't use EPOLLET otherwise. */
 	if (rem)
 		goto redo;
-
-	return 0;
 }
 
 /**
@@ -1254,23 +1275,6 @@ void tap_sock_init(struct ctx *c)
 	}
 }
 
-/**
- * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket
- * @c:		Execution context
- */
-static void tap_sock_reset(struct ctx *c)
-{
-	if (c->one_off) {
-		info("Client closed connection, exiting");
-		exit(EXIT_SUCCESS);
-	}
-
-	/* Close the connected socket, wait for a new connection */
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL);
-	close(c->fd_tap);
-	c->fd_tap = -1;
-}
-
 /**
  * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor
  * @c:		Execution context
@@ -1287,11 +1291,8 @@ void tap_handler(struct ctx *c, int fd, uint32_t events,
 		return;
 	}
 
-	if (c->mode == MODE_PASST) {
-		if (tap_handler_passt(c, now) ||
-		    (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)))
-			tap_sock_reset(c);
-	} else if (c->mode == MODE_PASTA) {
+	if (c->mode == MODE_PASST)
+		tap_handler_passt(c, events, now);
+	else if (c->mode == MODE_PASTA)
 		tap_handler_pasta(c, events, now);
-	}
 }
-- 
2.41.0