[PATCH v2 08/13] epoll: Split handling of ICMP and ICMPv6 sockets

[David Gibson <david@gibson.dropbear.id.au>]

We have different epoll type values for ICMP and ICMPv6 sockets, but they
both call the same handler function, icmp_sock_handler().  However that
function does essentially nothing in common for the two cases.  So, split
it into icmp_sock_handler() and icmpv6_sock_handler() and dispatch them
separately from the top level.

While we're there remove some parameters that the function was never using
anyway.  Also move the test for c->no_icmp into the functions, so that all
the logic specific to ICMP is within the handler, rather than in the top
level dispatch code.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 icmp.c  | 112 ++++++++++++++++++++++++++++++++------------------------
 icmp.h  |   9 ++---
 passt.c |   5 ++-
 3 files changed, 70 insertions(+), 56 deletions(-)

diff --git a/icmp.c b/icmp.c
index a4b6a47..b676a1a 100644
--- a/icmp.c
+++ b/icmp.c
@@ -60,78 +60,94 @@ static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
 
 /**
- * icmp_sock_handler() - Handle new data from socket
+ * icmp_sock_handler() - Handle new data from IPv4 ICMP socket
  * @c:		Execution context
  * @ref:	epoll reference
- * @events:	epoll events bitmap
- * @now:	Current timestamp, unused
  */
-void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
-		       uint32_t events, const struct timespec *now)
+void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
 {
-	union icmp_epoll_ref *iref = &ref.icmp;
-	struct sockaddr_storage sr;
-	socklen_t sl = sizeof(sr);
 	char buf[USHRT_MAX];
+	struct icmphdr *ih = (struct icmphdr *)buf;
+	struct sockaddr_in sr;
+	socklen_t sl = sizeof(sr);
 	uint16_t seq, id;
 	ssize_t n;
 
-	(void)events;
-	(void)now;
+	if (c->no_icmp)
+		return;
 
+	/* FIXME: Workaround clang-tidy not realizing that recvfrom()
+	 * writes the socket address.  See
+	 * https://github.com/llvm/llvm-project/issues/58992
+	 */
+	memset(&sr, 0, sizeof(sr));
 	n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
 	if (n < 0)
 		return;
 
-	if (iref->v6) {
-		struct sockaddr_in6 *sr6 = (struct sockaddr_in6 *)&sr;
-		struct icmp6hdr *ih = (struct icmp6hdr *)buf;
+	id = ntohs(ih->un.echo.id);
+	seq = ntohs(ih->un.echo.sequence);
 
-		id = ntohs(ih->icmp6_identifier);
-		seq = ntohs(ih->icmp6_sequence);
+	if (id != ref.icmp.id)
+		ih->un.echo.id = htons(ref.icmp.id);
 
-		/* If bind() fails e.g. because of a broken SELinux policy, this
-		 * might happen. Fix up the identifier to match the sent one.
-		 */
-		if (id != iref->id)
-			ih->icmp6_identifier = htons(iref->id);
+	if (c->mode == MODE_PASTA) {
+		if (icmp_id_map[V4][id].seq == seq)
+			return;
 
-		/* In PASTA mode, we'll get any reply we send, discard them. */
-		if (c->mode == MODE_PASTA) {
-			if (icmp_id_map[V6][id].seq == seq)
-				return;
-
-			icmp_id_map[V6][id].seq = seq;
-		}
+		icmp_id_map[V4][id].seq = seq;
+	}
 
-		debug("ICMPv6: echo %s to tap, ID: %i, seq: %i",
-		      (ih->icmp6_type == 128) ? "request" : "reply", id, seq);
+	debug("ICMP: echo %s to tap, ID: %i, seq: %i",
+	      (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq);
 
-		tap_icmp6_send(c, &sr6->sin6_addr,
-			       tap_ip6_daddr(c, &sr6->sin6_addr), buf, n);
-	} else {
-		struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr;
-		struct icmphdr *ih = (struct icmphdr *)buf;
+	tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n);
+}
 
-		id = ntohs(ih->un.echo.id);
-		seq = ntohs(ih->un.echo.sequence);
+/**
+ * icmpv6_sock_handler() - Handle new data from ICMPv6 socket
+ * @c:		Execution context
+ * @ref:	epoll reference
+ */
+void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref)
+{
+	char buf[USHRT_MAX];
+	struct icmp6hdr *ih = (struct icmp6hdr *)buf;
+	struct sockaddr_in6 sr;
+	socklen_t sl = sizeof(sr);
+	uint16_t seq, id;
+	ssize_t n;
 
-		if (id != iref->id)
-			ih->un.echo.id = htons(iref->id);
+	if (c->no_icmp)
+		return;
 
-		if (c->mode == MODE_PASTA) {
+	n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
+	if (n < 0)
+		return;
 
-			if (icmp_id_map[V4][id].seq == seq)
-				return;
+	id = ntohs(ih->icmp6_identifier);
+	seq = ntohs(ih->icmp6_sequence);
 
-			icmp_id_map[V4][id].seq = seq;
-		}
+	/* If bind() fails e.g. because of a broken SELinux policy,
+	 * this might happen. Fix up the identifier to match the sent
+	 * one.
+	 */
+	if (id != ref.icmp.id)
+		ih->icmp6_identifier = htons(ref.icmp.id);
 
-		debug("ICMP: echo %s to tap, ID: %i, seq: %i",
-		      (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq);
+	/* In PASTA mode, we'll get any reply we send, discard them. */
+	if (c->mode == MODE_PASTA) {
+		if (icmp_id_map[V6][id].seq == seq)
+			return;
 
-		tap_icmp4_send(c, sr4->sin_addr, tap_ip4_daddr(c), buf, n);
+		icmp_id_map[V6][id].seq = seq;
 	}
+
+	debug("ICMPv6: echo %s to tap, ID: %i, seq: %i",
+	      (ih->icmp6_type == 128) ? "request" : "reply", id, seq);
+
+	tap_icmp6_send(c, &sr.sin6_addr,
+		       tap_ip6_daddr(c, &sr.sin6_addr), buf, n);
 }
 
 /**
@@ -150,11 +166,11 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 	size_t plen;
 
 	if (af == AF_INET) {
-		union icmp_epoll_ref iref = { .v6 = 0 };
 		struct sockaddr_in sa = {
 			.sin_family = AF_INET,
 			.sin_addr = { .s_addr = htonl(INADDR_ANY) },
 		};
+		union icmp_epoll_ref iref;
 		struct icmphdr *ih;
 		int id, s;
 
@@ -204,12 +220,12 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 			      id, ntohs(ih->un.echo.sequence));
 		}
 	} else if (af == AF_INET6) {
-		union icmp_epoll_ref iref = { .v6 = 1 };
 		struct sockaddr_in6 sa = {
 			.sin6_family = AF_INET6,
 			.sin6_addr = IN6ADDR_ANY_INIT,
 			.sin6_scope_id = c->ifi6,
 		};
+		union icmp_epoll_ref iref;
 		struct icmp6hdr *ih;
 		int id, s;
 
diff --git a/icmp.h b/icmp.h
index 29c7829..32f0c47 100644
--- a/icmp.h
+++ b/icmp.h
@@ -10,8 +10,8 @@
 
 struct ctx;
 
-void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
-		       uint32_t events, const struct timespec *now);
+void icmp_sock_handler(const struct ctx *c, union epoll_ref ref);
+void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref);
 int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 		     const struct pool *p, const struct timespec *now);
 void icmp_timer(const struct ctx *c, const struct timespec *ts);
@@ -24,10 +24,7 @@ void icmp_init(void);
  * @id:			Associated echo identifier, needed if bind() fails
  */
 union icmp_epoll_ref {
-	struct {
-		uint32_t	v6:1,
-				id:16;
-	};
+	uint16_t id;
 	uint32_t u32;
 };
 
diff --git a/passt.c b/passt.c
index 665262b..f7cd376 100644
--- a/passt.c
+++ b/passt.c
@@ -329,9 +329,10 @@ loop:
 				udp_sock_handler(&c, ref, eventmask, &now);
 			break;
 		case EPOLL_TYPE_ICMP:
+			icmp_sock_handler(&c, ref);
+			break;
 		case EPOLL_TYPE_ICMPV6:
-			if (!c.no_icmp)
-				icmp_sock_handler(&c, ref, eventmask, &now);
+			icmpv6_sock_handler(&c, ref);
 			break;
 		default:
 			/* Can't happen */
-- 
@@ -329,9 +329,10 @@ loop:
 				udp_sock_handler(&c, ref, eventmask, &now);
 			break;
 		case EPOLL_TYPE_ICMP:
+			icmp_sock_handler(&c, ref);
+			break;
 		case EPOLL_TYPE_ICMPV6:
-			if (!c.no_icmp)
-				icmp_sock_handler(&c, ref, eventmask, &now);
+			icmpv6_sock_handler(&c, ref);
 			break;
 		default:
 			/* Can't happen */
-- 
2.41.0