Handling of each protocol needs some degree of tracking of the addresses
and ports at the end of each connection or flow.  Sometimes that's explicit
(as in the guest visible addresses for TCP connections), sometimes implicit
(the bound and connected addresses of sockets).

To allow more general and robust handling, and more consistency across
protocols we want to uniformly track the address and port at each end of
the connection.  Furthermore, because we allow port remapping, and we
sometimes need to apply NAT, the addresses and ports can be different as
seen by the guest/namespace and as by the host.

Introduce 'struct flowside' to keep track of common information related to
one side of each flow.  For now that's the addresses, ports and the pif id.
Store two of these in the common fields of a flow to track that information
for both sides.

For now we just introduce the structure and fields themselves, along with
a simple helper.  Later patches will actually use these to store useful
information.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow.h     | 33 +++++++++++++++++++++++++++++++++
 passt.h    |  2 ++
 tcp_conn.h |  1 -
 3 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/flow.h b/flow.h
index 48a0ab4..e090ba0 100644
--- a/flow.h
+++ b/flow.h
@@ -27,11 +27,44 @@ extern const char *flow_type_str[];
 #define FLOW_TYPE(f)							\
         ((f)->type < FLOW_NUM_TYPES ? flow_type_str[(f)->type] : "?")
 
+/**
+ * struct flowside - Common information for one side of a flow
+ * @eaddr:	Endpoint address (remote address from passt's PoV)
+ * @faddr:	Forwarding address (local address from passt's PoV)
+ * @eport:	Endpoint port
+ * @fport:	Forwarding port
+ * @pif:	pif ID on which this side of the flow exists
+ */
+struct flowside {
+	union inany_addr	faddr;
+	union inany_addr	eaddr;
+	in_port_t		fport;
+	in_port_t		eport;
+	uint8_t			pif;
+};
+static_assert(_Alignof(struct flowside) == _Alignof(uint32_t),
+	      "Unexpected alignment for struct flowside");
+
+/** flowside_complete - Check if flowside is fully initialized
+ * @fside:	flowside to check
+ */
+static inline bool flowside_complete(const struct flowside *fside)
+{
+	return fside->pif != PIF_NONE &&
+		!IN6_IS_ADDR_UNSPECIFIED(&fside->faddr) &&
+		!IN6_IS_ADDR_UNSPECIFIED(&fside->eaddr) &&
+		fside->fport != 0 && fside->eport != 0;
+}
+
+#define SIDES			2
+
 /**
  * struct flow_common - Common fields for packet flows
+ * @side[]:	Information for each side of the flow
  * @type:	Type of packet flow
  */
 struct flow_common {
+	struct flowside	side[SIDES];
 	uint8_t		type;
 };
 
diff --git a/passt.h b/passt.h
index a9e8f15..db0cd10 100644
--- a/passt.h
+++ b/passt.h
@@ -37,6 +37,8 @@ union epoll_ref;
 
 #include "pif.h"
 #include "packet.h"
+#include "siphash.h"
+#include "inany.h"
 #include "flow.h"
 #include "icmp.h"
 #include "port_fwd.h"
diff --git a/tcp_conn.h b/tcp_conn.h
index a5f5cfe..1a4dcf2 100644
--- a/tcp_conn.h
+++ b/tcp_conn.h
@@ -106,7 +106,6 @@ struct tcp_tap_conn {
 	uint32_t	seq_init_from_tap;
 };
 
-#define SIDES			2
 /**
  * struct tcp_splice_conn - Descriptor for a spliced TCP connection
  * @f:			Generic flow information
-- 
2.43.0

We now maintain a struct flowside describing each TCP connection as it
appears to the guest.  We don't yet store the same information for the
connections as they appear to the host.  Rather, that information is
implicit in the state of the host side socket.  For future generalisations
of flow/connection tracking, we're going to need to use this information
more heavily, so properly populate the other flowside in each flow table
entry with this information.

This does require an additional getsockname() call for each new connection.
We hope to optimise that away for at least some cases in future.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow.c | 41 +++++++++++++++++++++++++++++++++++++++++
 flow.h |  3 +++
 tcp.c  | 36 ++++++++++++++++++++++++++++++------
 util.h | 18 ++++++++++++++++++
 4 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/flow.c b/flow.c
index 421e6b5..b9c4a18 100644
--- a/flow.c
+++ b/flow.c
@@ -8,6 +8,7 @@
 #include <stdint.h>
 #include <unistd.h>
 #include <string.h>
+#include <errno.h>
 
 #include "util.h"
 #include "passt.h"
@@ -49,6 +50,46 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt,
...)
 	logmsg(pri, "Flow %u (%s): %s", flow_idx(f), FLOW_TYPE(f), msg);
 }
 
+/** flowside_from_sock - Initialize flowside to match an existing socket
+ * @fside:	flowside to initialize
+ * @pif:	pif id of this flowside
+ * @s:		socket
+ * @fsa:	Local addr of @s as sockaddr_in or sockaddr_in6, or NULL
+ * @esa:	Remote addr of @s as sockaddr_in or sockaddr_in6, or NULL
+ *
+ * If NULL is passed for either @fsa/@esa, we use getsockname()/getpeername() to
+ * obtain the information from the @s.
+ *
+ * #syscalls getsockname getpeername
+ */
+int flowside_from_sock(struct flowside *fside, uint8_t pif, int s,
+		       const void *fsa, const void *esa)
+{
+	struct sockaddr_storage sa;
+
+	fside->pif = pif;
+
+	if (!fsa) {
+		socklen_t sl = sizeof(sa);
+		if (getsockname(s, (struct sockaddr *)&sa, &sl) < 0)
+			return -errno;
+		fsa = &sa;
+	}
+	inany_from_sockaddr(&fside->faddr, &fside->fport,
+			    (const struct sockaddr *)fsa);
+
+	if (!esa) {
+		socklen_t sl = sizeof(sa);
+		if (getpeername(s, (struct sockaddr *)&sa, &sl) < 0)
+			return -errno;
+		esa = &sa;
+	}
+	inany_from_sockaddr(&fside->eaddr, &fside->eport,
+			    (const struct sockaddr *)esa);
+
+	return 0;
+}
+
 /**
  * DOC: Theory of Operation - allocation and freeing of flow entries
  *
diff --git a/flow.h b/flow.h
index e7126e4..37885b2 100644
--- a/flow.h
+++ b/flow.h
@@ -65,6 +65,9 @@ static inline void flowside_from_af(struct flowside *fside, uint8_t pif,
int af,
 	fside->eport = eport;
 }
 
+int flowside_from_sock(struct flowside *fside, uint8_t pif, int s,
+		       const void *fsa, const void *esa);
+
 /** flowside_complete - Check if flowside is fully initialized
  * @fside:	flowside to check
  */
diff --git a/tcp.c b/tcp.c
index 7ef20b1..18ab3ac 100644
--- a/tcp.c
+++ b/tcp.c
@@ -395,6 +395,7 @@ struct tcp6_l2_head {	/* For MSS6 macro: keep in sync with
tcp6_l2_buf_t */
 #define OPT_TS		8
 
 #define TAPFSIDE(conn)		(&(conn)->f.side[TAPSIDE])
+#define SOCKFSIDE(conn)		(&(conn)->f.side[SOCKSIDE])
 
 #define CONN_V4(conn)		(!!inany_v4(&TAPFSIDE(conn)->faddr))
 #define CONN_V6(conn)		(!CONN_V4(conn))
@@ -2014,6 +2015,14 @@ static void tcp_conn_from_tap(struct ctx *c,
 		conn_event(c, conn, TAP_SYN_ACK_SENT);
 	}
 
+	if (flowside_from_sock(SOCKFSIDE(conn), PIF_HOST, s, NULL, sa) < 0) {
+		err("tcp: Failed to get local name for outgoing connection");
+		tcp_rst(c, conn);
+		return;
+	}
+
+	ASSERT(flowside_complete(SOCKFSIDE(conn)));
+
 	tcp_epoll_ctl(c, conn);
 	return;
 
@@ -2653,8 +2662,10 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr
*addr)
  * @s:		Accepted socket
  * @sa:		Peer socket address (from accept())
  * @now:	Current timestamp
+ *
+ * Return: true if able to create a tap connection, false otherwise
  */
-static void tcp_tap_conn_from_sock(struct ctx *c,
+static bool tcp_tap_conn_from_sock(struct ctx *c,
 				   union tcp_listen_epoll_ref ref,
 				   struct tcp_tap_conn *conn, int s,
 				   const struct sockaddr *sa,
@@ -2666,8 +2677,16 @@ static void tcp_tap_conn_from_sock(struct ctx *c,
 	conn->ws_to_tap = conn->ws_from_tap = 0;
 	conn_event(c, conn, SOCK_ACCEPTED);
 
-	TAPFSIDE(conn)->pif = PIF_HOST;
-	inany_from_sockaddr(&TAPFSIDE(conn)->faddr, &TAPFSIDE(conn)->fport, sa);
+	if (flowside_from_sock(SOCKFSIDE(conn), PIF_HOST, s, NULL, sa) < 0) {
+		err("tcp: Failed to get local name, connection dropped");
+		return false;
+	}
+
+	ASSERT(flowside_complete(SOCKFSIDE(conn)));
+
+	TAPFSIDE(conn)->pif = PIF_TAP;
+	TAPFSIDE(conn)->faddr = SOCKFSIDE(conn)->eaddr;
+	TAPFSIDE(conn)->fport = SOCKFSIDE(conn)->eport;
 	tcp_snat_inbound(c, &TAPFSIDE(conn)->faddr);
 
 	if (CONN_V4(conn)) {
@@ -2693,6 +2712,8 @@ static void tcp_tap_conn_from_sock(struct ctx *c,
 	conn_flag(c, conn, ACK_FROM_TAP_DUE);
 
 	tcp_get_sndbuf(conn);
+
+	return true;
 }
 
 /**
@@ -2721,11 +2742,14 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
 				      s, (struct sockaddr *)&sa))
 		return;
 
-	tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s,
-			       (struct sockaddr *)&sa, now);
-	return;
+	if (tcp_tap_conn_from_sock(c, ref.tcp_listen, &flow->tcp, s,
+				   (struct sockaddr *)&sa, now))
+		return;
 
 cancel:
+	/* Failed to create the connection */
+	if (s >= 0)
+		close(s);
 	flow_alloc_cancel(flow);
 }
 
diff --git a/util.h b/util.h
index d2320f8..13d7353 100644
--- a/util.h
+++ b/util.h
@@ -298,4 +298,22 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
 #define accept4(s, addr, addrlen, flags) \
 	wrap_accept4((s), (addr), (addrlen), (flags))
 
+static inline int wrap_getsockname(int sockfd, struct sockaddr *addr,
+				   socklen_t *addrlen)
+{
+	sa_init(addr, addrlen);
+	return getsockname(sockfd, addr, addrlen);
+}
+#define getsockname(s, addr, addrlen)			\
+	wrap_getsockname((s), (addr), (addrlen))
+
+static inline int wrap_getpeername(int sockfd, struct sockaddr *addr,
+				   socklen_t *addrlen)
+{
+	sa_init(addr, addrlen);
+	return getpeername(sockfd, addr, addrlen);
+}
+#define getpeername(s, addr, addrlen)			\
+	wrap_getpeername((s), (addr), (addrlen))
+
 #endif /* UTIL_H */
-- 
2.43.0

When debugging passt/pasta, and the flow table in particular, one of the
most obvious things to know is when a new flow is initiated, along with the
details of its interface, addresses and ports.  Once we've determined to
what interface the flow should be forwarded, it's useful to know the
details of how it will appear on that other interface.

To help present that information uniformly, introduce FLOW_NEW_DBG() and
FLOW_FWD_DBG() helpers and use them for TCP connections, both "tap" and
spliced.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow.c       | 40 ++++++++++++++++++++++++++++++++++++++++
 flow.h       | 16 ++++++++++++++++
 tcp.c        | 11 +++++++++--
 tcp_splice.c |  3 ++-
 4 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/flow.c b/flow.c
index b9c4a18..bc8cfc6 100644
--- a/flow.c
+++ b/flow.c
@@ -9,6 +9,7 @@
 #include <unistd.h>
 #include <string.h>
 #include <errno.h>
+#include <arpa/inet.h>
 
 #include "util.h"
 #include "passt.h"
@@ -50,6 +51,45 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt,
...)
 	logmsg(pri, "Flow %u (%s): %s", flow_idx(f), FLOW_TYPE(f), msg);
 }
 
+/**
+ * flow_new_dbg() - Print debug message for new flow
+ * @f:		Common flow structure
+ * @side:	Which side initiated the new flow
+ */
+void flow_new_dbg(const struct flow_common *f, unsigned side)
+{
+	char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN];
+	const struct flowside *fside = &f->side[side];
+
+	flow_log_(f, LOG_DEBUG, "New %s from %s/%u: [%s]:%hu <-> [%s]:%hu",
+		  flow_type_str[f->type], pif_name(fside->pif), side,
+		  inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf)),
+		  fside->fport,
+		  inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf)),
+		  fside->eport);
+}
+
+/**
+ * flow_fwd_dbg() - Print debug message for newly forwarded flow
+ * @f:		Common flow structure
+ * @side:	Which side was the flow forwarded to
+ */
+void flow_fwd_dbg(const struct flow_common *f, unsigned side)
+{
+	char ebuf[INET6_ADDRSTRLEN], fbuf[INET6_ADDRSTRLEN];
+	const struct flowside *fside = &f->side[side];
+
+	inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf));
+	inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf));
+
+	flow_log_(f, LOG_DEBUG, "Forwarded to %s/%u: [%s]:%hu <-> [%s]:%hu",
+		  pif_name(fside->pif), side,
+		  inet_ntop(AF_INET6, &fside->faddr, fbuf, sizeof(fbuf)),
+		  fside->fport,
+		  inet_ntop(AF_INET6, &fside->eaddr, ebuf, sizeof(ebuf)),
+		  fside->eport);
+}
+
 /** flowside_from_sock - Initialize flowside to match an existing socket
  * @fside:	flowside to initialize
  * @pif:	pif id of this flowside
diff --git a/flow.h b/flow.h
index 37885b2..e7c4484 100644
--- a/flow.h
+++ b/flow.h
@@ -97,6 +97,22 @@ struct flow_common {
 #define FLOW_TABLE_PRESSURE		30	/* % of FLOW_MAX */
 #define FLOW_FILE_PRESSURE		30	/* % of c->nofile */
 
+/** flow_complete - Check if common parts of flow are fully initialized
+ * @flow:	flow to check
+ */
+static inline bool flow_complete(const struct flow_common *f)
+{
+	return f->type != FLOW_TYPE_NONE && f->type < FLOW_NUM_TYPES
&&
+		flowside_complete(&f->side[0]) &&
+		flowside_complete(&f->side[1]);
+}
+
+void flow_new_dbg(const struct flow_common *f, unsigned side);
+#define FLOW_NEW_DBG(flow, side)	(flow_new_dbg(&(flow)->f, (side)))
+
+void flow_fwd_dbg(const struct flow_common *f, unsigned side);
+#define FLOW_FWD_DBG(flow, side)	(flow_fwd_dbg(&(flow)->f, (side)))
+
 /**
  * struct flow_sidx - ID for one side of a specific flow
  * @side:	Side referenced (0 or 1)
diff --git a/tcp.c b/tcp.c
index 6d77cf6..954c24f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1949,6 +1949,8 @@ static void tcp_conn_from_tap(struct ctx *c,
 	conn->f.type = FLOW_TCP;
 	flowside_from_af(TAPFSIDE(conn), PIF_TAP, af,
 			 daddr, ntohs(th->dest), saddr, ntohs(th->source));
+
+	FLOW_NEW_DBG(conn, TAPSIDE);
 	ASSERT(flowside_complete(TAPFSIDE(conn)));
 
 	conn->sock = s;
@@ -2021,7 +2023,8 @@ static void tcp_conn_from_tap(struct ctx *c,
 		return;
 	}
 
-	ASSERT(flowside_complete(SOCKFSIDE(conn)));
+	FLOW_FWD_DBG(conn, SOCKSIDE);
+	ASSERT(flow_complete(&conn->f));
 
 	tcp_epoll_ctl(c, conn);
 	return;
@@ -2690,7 +2693,8 @@ static void tcp_tap_conn_from_sock(struct ctx *c,
 	}
 	TAPFSIDE(conn)->eport = ref.port;
 
-	ASSERT(flowside_complete(TAPFSIDE(conn)));
+	FLOW_FWD_DBG(conn, TAPSIDE);
+	ASSERT(flow_complete(&conn->f));
 
 	tcp_seq_init(c, conn, now);
 	tcp_hash_insert(c, conn);
@@ -2734,6 +2738,9 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref,
 		return;
 	}
 
+	FLOW_NEW_DBG(flow, 0);
+	ASSERT(flowside_complete(&flow->f.side[0]));
+
 	if (c->mode == MODE_PASTA &&
 	    tcp_splice_conn_from_sock(c, ref.tcp_listen, &flow->tcp_splice, s))
 		return;
diff --git a/tcp_splice.c b/tcp_splice.c
index 0faeb1b..b59ac90 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -296,7 +296,8 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 	inany_from_sockaddr(&FSIDE1(conn)->faddr, &FSIDE1(conn)->fport,
 			    (struct sockaddr *)&sa);
 
-	ASSERT(flowside_complete(FSIDE1(conn)));
+	FLOW_FWD_DBG(conn, 1);
+	ASSERT(flow_complete(&conn->f));
 
 	for (side = 0; side < SIDES; side++) {
 		conn->pipe[side][0] = conn->pipe[side][1] = -1;
-- 
2.43.0

Currently we match TCP packets received on the tap connection to a TCP
connection via a hash table based on the forwarding address and both
ports.  We hope in future to allow for multiple guest side addresses, or
for multiple interfaces which means we may need to distinguish based on
the endpoint address and pif as well.  We also want a unified hash table
to cover multiple protocols, not just TCP.

Replace the TCP specific hash function with one suitable for general flows,
or rather for one side of a general flow.  This includes all the
information from struct flowside, plus the L4 protocol number.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow.c | 21 +++++++++++++++++++++
 flow.h | 19 +++++++++++++++++++
 tcp.c  | 59 +++++++++++-----------------------------------------------
 3 files changed, 51 insertions(+), 48 deletions(-)

diff --git a/flow.c b/flow.c
index bc8cfc6..263633e 100644
--- a/flow.c
+++ b/flow.c
@@ -229,6 +229,27 @@ void flow_alloc_cancel(union flow *flow)
 	flow_first_free = FLOW_IDX(flow);
 }
 
+/**
+ * flow_hash() - Calculate hash value for one side of a flow
+ * @c:		Execution context
+ * @proto:	Protocol of this flow (IP L4 protocol number)
+ * @fside:	Flowside
+ *
+ * Return: hash value
+ */
+uint64_t flow_hash(const struct ctx *c, uint8_t proto,
+		   const struct flowside *fside)
+{
+	struct siphash_state state = SIPHASH_INIT(c->hash_secret);
+
+	ASSERT(flowside_complete(fside));
+	inany_siphash_feed(&state, &fside->faddr);
+	inany_siphash_feed(&state, &fside->eaddr);
+	return siphash_final(&state, 38, (uint64_t)proto << 40 |
+			     (uint64_t)fside->pif << 32 |
+			     fside->fport << 16 | fside->eport);
+}
+
 /**
  * flow_defer_handler() - Handler for per-flow deferred and timed tasks
  * @c:		Execution context
diff --git a/flow.h b/flow.h
index e7c4484..72ded54 100644
--- a/flow.h
+++ b/flow.h
@@ -81,6 +81,22 @@ static inline bool flowside_complete(const struct flowside *fside)
 
 #define SIDES			2
 
+/**
+ * flowside_eq() - Check if two flowsides are equal
+ * @left, @right:	Flowsides to compare
+ *
+ * Return: true if equal, false otherwise
+ */
+static inline bool flowside_eq(const struct flowside *left,
+			       const struct flowside *right)
+{
+	return left->pif == right->pif &&
+		inany_equals(&left->eaddr, &right->eaddr) &&
+		left->eport == right->eport &&
+		inany_equals(&left->faddr, &right->faddr) &&
+		left->fport == right->fport;
+}
+
 /**
  * struct flow_common - Common fields for packet flows
  * @side[]:	Information for each side of the flow
@@ -113,6 +129,9 @@ void flow_new_dbg(const struct flow_common *f, unsigned side);
 void flow_fwd_dbg(const struct flow_common *f, unsigned side);
 #define FLOW_FWD_DBG(flow, side)	(flow_fwd_dbg(&(flow)->f, (side)))
 
+uint64_t flow_hash(const struct ctx *c, uint8_t proto,
+		   const struct flowside *fside);
+
 /**
  * struct flow_sidx - ID for one side of a specific flow
  * @side:	Side referenced (0 or 1)
diff --git a/tcp.c b/tcp.c
index 954c24f..a7907f2 100644
--- a/tcp.c
+++ b/tcp.c
@@ -575,7 +575,7 @@ static unsigned int tcp6_l2_flags_buf_used;
 
 #define CONN(idx)		(&(FLOW(idx)->tcp))
 
-/* Table for lookup from remote address, local port, remote port */
+/* Table for lookup from flowside information */
 static flow_sidx_t tc_hash[TCP_HASH_TABLE_SIZE];
 
 static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX,
@@ -1134,44 +1134,6 @@ static int tcp_opt_get(const char *opts, size_t len, uint8_t
type_find,
 	return -1;
 }
 
-/**
- * tcp_hash_match() - Check if a connection entry matches address and ports
- * @conn:	Connection entry to match against
- * @faddr:	Guest side forwarding address
- * @eport:	Guest side endpoint port
- * @fport:	Guest side forwarding port
- *
- * Return: 1 on match, 0 otherwise
- */
-static int tcp_hash_match(const struct tcp_tap_conn *conn,
-			  const union inany_addr *faddr,
-			  in_port_t eport, in_port_t fport)
-{
-	if (inany_equals(&TAPFSIDE(conn)->faddr, faddr) &&
-	    TAPFSIDE(conn)->eport == eport && TAPFSIDE(conn)->fport == fport)
-		return 1;
-
-	return 0;
-}
-
-/**
- * tcp_hash() - Calculate hash value for connection given address and ports
- * @c:		Execution context
- * @faddr:	Guest side forwarding address
- * @eport:	Guest side endpoint port
- * @fport:	Guest side forwarding port
- *
- * Return: hash value, needs to be adjusted for table size
- */
-static uint64_t tcp_hash(const struct ctx *c, const union inany_addr *faddr,
-			 in_port_t eport, in_port_t fport)
-{
-	struct siphash_state state = SIPHASH_INIT(c->hash_secret);
-
-	inany_siphash_feed(&state, faddr);
-	return siphash_final(&state, 20, (uint64_t)eport << 16 | fport);
-}
-
 /**
  * tcp_conn_hash() - Calculate hash bucket of an existing connection
  * @c:		Execution context
@@ -1182,8 +1144,7 @@ static uint64_t tcp_hash(const struct ctx *c, const union inany_addr
*faddr,
 static uint64_t tcp_conn_hash(const struct ctx *c,
 			      const struct tcp_tap_conn *conn)
 {
-	return tcp_hash(c, &TAPFSIDE(conn)->faddr,
-			TAPFSIDE(conn)->eport, TAPFSIDE(conn)->fport);
+	return flow_hash(c, IPPROTO_TCP, TAPFSIDE(conn));
 }
 
 /**
@@ -1258,25 +1219,26 @@ static void tcp_hash_remove(const struct ctx *c,
  * tcp_hash_lookup() - Look up connection given remote address and ports
  * @c:		Execution context
  * @af:		Address family, AF_INET or AF_INET6
+ * @eaddr:	Guest side endpoint address (guest local address)
  * @faddr:	Guest side forwarding address (guest remote address)
  * @eport:	Guest side endpoint port (guest local port)
  * @fport:	Guest side forwarding port (guest remote port)
  *
  * Return: connection pointer, if found, -ENOENT otherwise
  */
-static struct tcp_tap_conn *tcp_hash_lookup(const struct ctx *c,
-					    int af, const void *faddr,
+static struct tcp_tap_conn *tcp_hash_lookup(const struct ctx *c, int af,
+					    const void *eaddr, const void *faddr,
 					    in_port_t eport, in_port_t fport)
 {
-	union inany_addr aany;
+	struct flowside fside;
 	union flow *flow;
 	unsigned b;
 
-	inany_from_af(&aany, af, faddr);
+	flowside_from_af(&fside, PIF_TAP, af, faddr, fport, eaddr, eport);
 
-	b = tcp_hash(c, &aany, eport, fport) % TCP_HASH_TABLE_SIZE;
+	b = flow_hash(c, IPPROTO_TCP, &fside) % TCP_HASH_TABLE_SIZE;
 	while ((flow = flow_at_sidx(tc_hash[b])) &&
-	       !tcp_hash_match(&flow->tcp, &aany, eport, fport))
+	       !flowside_eq(&flow->f.side[TAPSIDE], &fside))
 		b = mod_sub(b, 1, TCP_HASH_TABLE_SIZE);
 
 	return &flow->tcp;
@@ -2506,7 +2468,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	optlen = MIN(optlen, ((1UL << 4) /* from doff width */ - 6) * 4UL);
 	opts = packet_get(p, idx, sizeof(*th), optlen, NULL);
 
-	conn = tcp_hash_lookup(c, af, daddr, ntohs(th->source), ntohs(th->dest));
+	conn = tcp_hash_lookup(c, af, saddr, daddr,
+			       ntohs(th->source), ntohs(th->dest));
 
 	/* New connection from tap */
 	if (!conn) {
-- 
2.43.0

Move the data structures and helper functions for the TCP hash table to
flow.c, making it a general hash table indexing sides of flows.  This is
largely code motion and straightforward renames.  There are two semantic
changes:

 * flow_hash_lookup() now needs to verify that the entry has a matching
   protocol as well as matching addresses, ports and interface
 * When moving entries in the flow table tcp_tap_conn_update() could
   previously assume that only the TAP side of the TCP connection was
   hashed and might need an update.  For the general flow hash table either
   side of a flow could be hashed, so when we move an entry in the flow
   table we need to allow for updating both sides in the hash table.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 flow.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++-
 flow.h |  11 +++--
 tcp.c  | 146 +++++----------------------------------------------------
 3 files changed, 147 insertions(+), 138 deletions(-)

diff --git a/flow.c b/flow.c
index 70036fc..3d016c3 100644
--- a/flow.c
+++ b/flow.c
@@ -40,6 +40,16 @@ union flow flowtab[FLOW_MAX];
 /* Last time the flow timers ran */
 static struct timespec flow_timer_run;
 
+/* Hash table to index it */
+#define FLOW_HASH_LOAD		70		/* % */
+#define FLOW_HASH_SIZE		((2 * FLOW_MAX * 100 / FLOW_HASH_LOAD))
+
+/* Table for lookup from flowside information */
+static flow_sidx_t flow_hashtab[FLOW_HASH_SIZE];
+
+static_assert(ARRAY_SIZE(flow_hashtab) >= 2 * FLOW_MAX,
+"Safe linear probing requires hash table with more entries than the number of sides
in the flow table");
+
 /** flow_log_ - Log flow-related message
  * @f:		flow the message is related to
  * @pri:	Log priority
@@ -244,8 +254,8 @@ void flow_alloc_cancel(union flow *flow)
  *
  * Return: hash value
  */
-uint64_t flow_hash(const struct ctx *c, uint8_t proto,
-		   const struct flowside *fside)
+static uint64_t flow_hash(const struct ctx *c, uint8_t proto,
+			  const struct flowside *fside)
 {
 	struct siphash_state state = SIPHASH_INIT(c->hash_secret);
 
@@ -257,6 +267,115 @@ uint64_t flow_hash(const struct ctx *c, uint8_t proto,
 			     fside->fport << 16 | fside->eport);
 }
 
+/**
+ * flow_sidx_hash() - Calculate hash value for given side of a given flow
+ * @c:		Execution context
+ * @sidx:	Flow & side index to get hash for
+ *
+ * Return: hash value, of the flow & side represented by @sidx
+ */
+static uint64_t flow_sidx_hash(const struct ctx *c, flow_sidx_t sidx)
+{
+	const struct flow_common *f = &flow_at_sidx(sidx)->f;
+	return flow_hash(c, FLOW_PROTO(f), &f->side[sidx.side]);
+}
+
+/**
+ * flow_hash_probe() - Find hash bucket for a flow
+ * @c:		Execution context
+ * @sidx:	Flow and side to find bucket for
+ *
+ * Return: If @sidx is in the hash table, its current bucket, otherwise a
+ *         suitable free bucket for it.
+ */
+static inline unsigned flow_hash_probe(const struct ctx *c, flow_sidx_t sidx)
+{
+	unsigned b = flow_sidx_hash(c, sidx) % FLOW_HASH_SIZE;
+
+	/* Linear probing */
+	while (!flow_sidx_eq(flow_hashtab[b], FLOW_SIDX_NONE) &&
+	       !flow_sidx_eq(flow_hashtab[b], sidx))
+		b = mod_sub(b, 1, FLOW_HASH_SIZE);
+
+	return b;
+}
+
+/**
+ * flow_hash_insert() - Insert side of a flow into into hash table
+ * @c:		Execution context
+ * @sidx:	Flow & side index
+ */
+void flow_hash_insert(const struct ctx *c, flow_sidx_t sidx)
+{
+	unsigned b = flow_hash_probe(c, sidx);
+
+	flow_hashtab[b] = sidx;
+	flow_dbg(flow_at_sidx(sidx), "hash table insert: bucket: %u", b);
+}
+
+/**
+ * flow_hash_remove() - Drop side of a flow from the hash table
+ * @c:		Execution context
+ * @sidx:	Side of flow to remove
+ */
+void flow_hash_remove(const struct ctx *c, flow_sidx_t sidx)
+{
+	unsigned b = flow_hash_probe(c, sidx), s;
+
+	if (flow_sidx_eq(flow_hashtab[b], FLOW_SIDX_NONE))
+		return; /* Redundant remove */
+
+	flow_dbg(flow_at_sidx(sidx), "hash table remove: bucket: %u", b);
+
+	/* Scan the remainder of the cluster */
+	for (s = mod_sub(b, 1, FLOW_HASH_SIZE);
+	     !flow_sidx_eq(flow_hashtab[s], FLOW_SIDX_NONE);
+	     s = mod_sub(s, 1, FLOW_HASH_SIZE)) {
+		unsigned h = flow_sidx_hash(c, flow_hashtab[s]) % FLOW_HASH_SIZE;
+
+		if (!mod_between(h, s, b, FLOW_HASH_SIZE)) {
+			/* flow_hashtab[s] can live in flow_hashtab[b]'s slot */
+			debug("hash table remove: shuffle %u -> %u", s, b);
+			flow_hashtab[b] = flow_hashtab[s];
+			b = s;
+		}
+	}
+
+	flow_hashtab[b] = FLOW_SIDX_NONE;
+}
+
+/**
+ * flow_hash_lookup() - Look up a flow given addressing information
+ * @c:		Execution context
+ * @proto:	Protocol of the flow (IP L4 protocol number)
+ * @pif:	Interface of the flow
+ * @af:		Address family, AF_INET or AF_INET6
+ * @eaddr:	Guest side endpoint address (guest local address)
+ * @faddr:	Guest side forwarding address (guest remote address)
+ * @eport:	Guest side endpoint port (guest local port)
+ * @fport:	Guest side forwarding port (guest remote port)
+ *
+ * Return: sidx of the matching flow & side, FLOW_SIDX_NONE if not found
+ */
+flow_sidx_t flow_hash_lookup(const struct ctx *c, uint8_t proto, uint8_t pif,
+			     int af, const void *eaddr, const void *faddr,
+			     in_port_t eport, in_port_t fport)
+{
+	struct flowside fside;
+	union flow *flow;
+	int b;
+
+	flowside_from_af(&fside, pif, af, faddr, fport, eaddr, eport);
+
+	b = flow_hash(c, proto, &fside) % FLOW_HASH_SIZE;
+	while ((flow = flow_at_sidx(flow_hashtab[b])) &&
+	       FLOW_PROTO(&flow->f) == proto &&
+	       !flowside_eq(&flow->f.side[flow_hashtab[b].side], &fside))
+		b = (b + 1) % FLOW_HASH_SIZE;
+
+	return flow_hashtab[b];
+}
+
 /**
  * flow_defer_handler() - Handler for per-flow deferred and timed tasks
  * @c:		Execution context
@@ -334,7 +453,12 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
  */
 void flow_init(void)
 {
+	unsigned b;
+
 	/* Initial state is a single free block containing the whole table */
 	flowtab[0].free.n = FLOW_MAX;
 	flowtab[0].free.next = FLOW_MAX;
+
+	for (b = 0; b < FLOW_HASH_SIZE; b++)
+		flow_hashtab[b] = FLOW_SIDX_NONE;
 }
diff --git a/flow.h b/flow.h
index ab831d1..adb5d44 100644
--- a/flow.h
+++ b/flow.h
@@ -133,9 +133,6 @@ void flow_new_dbg(const struct flow_common *f, unsigned side);
 void flow_fwd_dbg(const struct flow_common *f, unsigned side);
 #define FLOW_FWD_DBG(flow, side)	(flow_fwd_dbg(&(flow)->f, (side)))
 
-uint64_t flow_hash(const struct ctx *c, uint8_t proto,
-		   const struct flowside *fside);
-
 /**
  * struct flow_sidx - ID for one side of a specific flow
  * @side:	Side referenced (0 or 1)
@@ -161,6 +158,12 @@ static inline bool flow_sidx_eq(flow_sidx_t a, flow_sidx_t b)
 	return (a.flow == b.flow) && (a.side == b.side);
 }
 
+void flow_hash_insert(const struct ctx *c, flow_sidx_t sidx);
+void flow_hash_remove(const struct ctx *c, flow_sidx_t sidx);
+flow_sidx_t flow_hash_lookup(const struct ctx *c, uint8_t proto, uint8_t pif,
+			     int af, const void *eaddr, const void *faddr,
+			     in_port_t eport, in_port_t fport);
+
 union flow;
 
 void flow_init(void);
@@ -180,4 +183,6 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt,
...)
 			flow_dbg((f), __VA_ARGS__);			\
 	} while (0)
 
+void flow_init(void);
+
 #endif /* FLOW_H */
diff --git a/tcp.c b/tcp.c
index a7907f2..b6d046f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -308,9 +308,6 @@
 #define TCP_FRAMES							\
 	(c->mode == MODE_PASST ? TCP_FRAMES_MEM : 1)
 
-#define TCP_HASH_TABLE_LOAD		70		/* % */
-#define TCP_HASH_TABLE_SIZE		(FLOW_MAX * 100 / TCP_HASH_TABLE_LOAD)
-
 #define MAX_WS				8
 #define MAX_WINDOW			(1 << (16 + (MAX_WS)))
 
@@ -575,12 +572,6 @@ static unsigned int tcp6_l2_flags_buf_used;
 
 #define CONN(idx)		(&(FLOW(idx)->tcp))
 
-/* Table for lookup from flowside information */
-static flow_sidx_t tc_hash[TCP_HASH_TABLE_SIZE];
-
-static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX,
-	"Safe linear probing requires hash table larger than connection table");
-
 /* Pools for pre-opened sockets (in init) */
 int init_sock_pool4		[TCP_SOCK_POOL_SIZE];
 int init_sock_pool6		[TCP_SOCK_POOL_SIZE];
@@ -773,9 +764,6 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn
*conn,
 		tcp_timer_ctl(c, conn);
 }
 
-static void tcp_hash_remove(const struct ctx *c,
-			    const struct tcp_tap_conn *conn);
-
 /**
  * conn_event_do() - Set and log connection events, update epoll state
  * @c:		Execution context
@@ -821,7 +809,7 @@ static void conn_event_do(const struct ctx *c, struct tcp_tap_conn
*conn,
 			 num == -1 	       ? "CLOSED" : tcp_event_str[num]);
 
 	if (event == CLOSED)
-		tcp_hash_remove(c, conn);
+		flow_hash_remove(c, FLOW_SIDX(conn, TAPSIDE));
 	else if ((event == TAP_FIN_RCVD) && !(conn->events & SOCK_FIN_RCVD))
 		conn_flag(c, conn, ACTIVE_CLOSE);
 	else
@@ -1134,116 +1122,6 @@ static int tcp_opt_get(const char *opts, size_t len, uint8_t
type_find,
 	return -1;
 }
 
-/**
- * tcp_conn_hash() - Calculate hash bucket of an existing connection
- * @c:		Execution context
- * @conn:	Connection
- *
- * Return: hash value, needs to be adjusted for table size
- */
-static uint64_t tcp_conn_hash(const struct ctx *c,
-			      const struct tcp_tap_conn *conn)
-{
-	return flow_hash(c, IPPROTO_TCP, TAPFSIDE(conn));
-}
-
-/**
- * tcp_hash_probe() - Find hash bucket for a connection
- * @c:		Execution context
- * @conn:	Connection to find bucket for
- *
- * Return: If @conn is in the table, its current bucket, otherwise a suitable
- *         free bucket for it.
- */
-static inline unsigned tcp_hash_probe(const struct ctx *c,
-				      const struct tcp_tap_conn *conn)
-{
-	flow_sidx_t sidx = FLOW_SIDX(conn, TAPSIDE);
-	unsigned b = tcp_conn_hash(c, conn) % TCP_HASH_TABLE_SIZE;
-
-	/* Linear probing */
-	while (!flow_sidx_eq(tc_hash[b], FLOW_SIDX_NONE) &&
-	       !flow_sidx_eq(tc_hash[b], sidx))
-		b = mod_sub(b, 1, TCP_HASH_TABLE_SIZE);
-
-	return b;
-}
-
-/**
- * tcp_hash_insert() - Insert connection into hash table, chain link
- * @c:		Execution context
- * @conn:	Connection pointer
- */
-static void tcp_hash_insert(const struct ctx *c, struct tcp_tap_conn *conn)
-{
-	unsigned b = tcp_hash_probe(c, conn);
-
-	tc_hash[b] = FLOW_SIDX(conn, TAPSIDE);
-	flow_dbg(conn, "hash table insert: sock %i, bucket: %u", conn->sock, b);
-}
-
-/**
- * tcp_hash_remove() - Drop connection from hash table, chain unlink
- * @c:		Execution context
- * @conn:	Connection pointer
- */
-static void tcp_hash_remove(const struct ctx *c,
-			    const struct tcp_tap_conn *conn)
-{
-	unsigned b = tcp_hash_probe(c, conn), s;
-	union flow *flow = flow_at_sidx(tc_hash[b]);
-
-	if (!flow)
-		return; /* Redundant remove */
-
-	flow_dbg(conn, "hash table remove: sock %i, bucket: %u", conn->sock, b);
-
-	/* Scan the remainder of the cluster */
-	for (s = mod_sub(b, 1, TCP_HASH_TABLE_SIZE);
-	     (flow = flow_at_sidx(tc_hash[s]));
-	     s = mod_sub(s, 1, TCP_HASH_TABLE_SIZE)) {
-		unsigned h = tcp_conn_hash(c, &flow->tcp) % TCP_HASH_TABLE_SIZE;
-
-		if (!mod_between(h, s, b, TCP_HASH_TABLE_SIZE)) {
-			/* tc_hash[s] can live in tc_hash[b]'s slot */
-			debug("hash table remove: shuffle %u -> %u", s, b);
-			tc_hash[b] = tc_hash[s];
-			b = s;
-		}
-	}
-
-	tc_hash[b] = FLOW_SIDX_NONE;
-}
-
-/**
- * tcp_hash_lookup() - Look up connection given remote address and ports
- * @c:		Execution context
- * @af:		Address family, AF_INET or AF_INET6
- * @eaddr:	Guest side endpoint address (guest local address)
- * @faddr:	Guest side forwarding address (guest remote address)
- * @eport:	Guest side endpoint port (guest local port)
- * @fport:	Guest side forwarding port (guest remote port)
- *
- * Return: connection pointer, if found, -ENOENT otherwise
- */
-static struct tcp_tap_conn *tcp_hash_lookup(const struct ctx *c, int af,
-					    const void *eaddr, const void *faddr,
-					    in_port_t eport, in_port_t fport)
-{
-	struct flowside fside;
-	union flow *flow;
-	unsigned b;
-
-	flowside_from_af(&fside, PIF_TAP, af, faddr, fport, eaddr, eport);
-
-	b = flow_hash(c, IPPROTO_TCP, &fside) % TCP_HASH_TABLE_SIZE;
-	while ((flow = flow_at_sidx(tc_hash[b])) &&
-	       !flowside_eq(&flow->f.side[TAPSIDE], &fside))
-		b = mod_sub(b, 1, TCP_HASH_TABLE_SIZE);
-
-	return &flow->tcp;
-}
-
 /**
  * tcp_flow_defer() - Deferred per-flow handling (clean up closed connections)
  * @flow:	Flow table entry for this connection
@@ -1949,7 +1827,7 @@ static void tcp_conn_from_tap(struct ctx *c,
 	tcp_seq_init(c, conn, now);
 	conn->seq_ack_from_tap = conn->seq_to_tap;
 
-	tcp_hash_insert(c, conn);
+	flow_hash_insert(c, FLOW_SIDX(conn, TAPSIDE));
 
 	if (!bind(s, sa, sl)) {
 		tcp_rst(c, conn);	/* Nobody is listening then */
@@ -2452,6 +2330,8 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	struct tcp_tap_conn *conn;
 	size_t optlen, len;
 	struct tcphdr *th;
+	union flow *flow;
+	flow_sidx_t sidx;
 	int ack_due = 0;
 	char *opts;
 	int count;
@@ -2468,17 +2348,22 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, int af,
 	optlen = MIN(optlen, ((1UL << 4) /* from doff width */ - 6) * 4UL);
 	opts = packet_get(p, idx, sizeof(*th), optlen, NULL);
 
-	conn = tcp_hash_lookup(c, af, saddr, daddr,
-			       ntohs(th->source), ntohs(th->dest));
+	sidx = flow_hash_lookup(c, IPPROTO_TCP, PIF_TAP, af, saddr, daddr,
+				ntohs(th->source), ntohs(th->dest));
+	flow = flow_at_sidx(sidx);
 
 	/* New connection from tap */
-	if (!conn) {
+	if (!flow) {
 		if (opts && th->syn && !th->ack)
 			tcp_conn_from_tap(c, af, saddr, daddr, th,
 					  opts, optlen, now);
 		return 1;
 	}
 
+	ASSERT(flow->f.type == FLOW_TCP);
+	ASSERT(sidx.side == TAPSIDE);
+	conn = &flow->tcp;
+
 	flow_trace(conn, "packet length %zu from tap", len);
 
 	if (th->rst) {
@@ -2660,7 +2545,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c,
 	ASSERT(flow_complete(&conn->f));
 
 	tcp_seq_init(c, conn, now);
-	tcp_hash_insert(c, conn);
+	flow_hash_insert(c, FLOW_SIDX(conn, TAPSIDE));
 
 	conn->seq_ack_from_tap = conn->seq_to_tap;
 
@@ -3032,11 +2917,6 @@ static void tcp_sock_refill_init(const struct ctx *c)
  */
 int tcp_init(struct ctx *c)
 {
-	unsigned b;
-
-	for (b = 0; b < TCP_HASH_TABLE_SIZE; b++)
-		tc_hash[b] = FLOW_SIDX_NONE;
-
 	if (c->ifi4)
 		tcp_sock4_iov_init(c);
 
-- 
2.43.0

Currently icmp_id_map[][] stores information about ping sockets in a
bespoke structure.  Move the same information into a new type of flow that
lives in the flow table.  To match that change, replace the existing ICMP
timer with a flow-based timer for expiring ping sockets.  This has the
advantage that we only need to scan the active flows, not all possible ids.

For now we still index the flow table entries with icmp_id_map[][], and we
don't populate all the common flow information.  This is kind of an abuse
of the flow table, but it's a useful interim step towards full flow table
integration for ICMP.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 Makefile     |   6 +--
 flow.c       |   9 ++++
 flow.h       |   4 ++
 flow_table.h |   2 +
 icmp.c       | 127 +++++++++++++++++++++++----------------------------
 icmp.h       |   2 +-
 icmp_flow.h  |  31 +++++++++++++
 passt.c      |   5 --
 8 files changed, 106 insertions(+), 80 deletions(-)
 create mode 100644 icmp_flow.h

diff --git a/Makefile b/Makefile
index af4fa87..610ab96 100644
--- a/Makefile
+++ b/Makefile
@@ -54,9 +54,9 @@ SRCS = $(PASST_SRCS) $(QRAP_SRCS)
 MANPAGES = passt.1 pasta.1 qrap.1
 
 PASST_HEADERS = arch.h arp.h checksum.h conf.h dhcp.h dhcpv6.h flow.h \
-	flow_table.h icmp.h inany.h isolation.h lineread.h log.h ndp.h \
-	netlink.h packet.h passt.h pasta.h pcap.h pif.h port_fwd.h siphash.h \
-	tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h
+	flow_table.h icmp.h icmp_flow.h inany.h isolation.h lineread.h log.h \
+	ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.h port_fwd.h \
+	siphash.h tap.h tcp.h tcp_conn.h tcp_splice.h udp.h util.h
 HEADERS = $(PASST_HEADERS) seccomp.h
 
 C := \#include <linux/tcp.h>\nstruct tcp_info x = { .tcpi_snd_wnd = 0 };
diff --git a/flow.c b/flow.c
index e4ea931..ef146dc 100644
--- a/flow.c
+++ b/flow.c
@@ -22,6 +22,8 @@ const char *flow_type_str[] = {
 	[FLOW_TYPE_NONE]	= "<none>",
 	[FLOW_TCP]		= "TCP connection",
 	[FLOW_TCP_SPLICE]	= "TCP connection (spliced)",
+	[FLOW_PING4]		= "ICMP ping sequence",
+	[FLOW_PING6]		= "ICMPv6 ping sequence",
 };
 static_assert(ARRAY_SIZE(flow_type_str) == FLOW_NUM_TYPES,
 	      "flow_type_str[] doesn't match enum flow_type");
@@ -29,6 +31,8 @@ static_assert(ARRAY_SIZE(flow_type_str) == FLOW_NUM_TYPES,
 const uint8_t flow_proto[] = {
 	[FLOW_TCP]		= IPPROTO_TCP,
 	[FLOW_TCP_SPLICE]	= IPPROTO_TCP,
+	[FLOW_PING4]		= IPPROTO_ICMP,
+	[FLOW_PING6]		= IPPROTO_ICMPV6,
 };
 static_assert(ARRAY_SIZE(flow_proto) == FLOW_NUM_TYPES,
 	      "flow_proto[] doesn't match enum flow_type");
@@ -438,6 +442,11 @@ void flow_defer_handler(const struct ctx *c, const struct timespec
*now)
 			if (!closed && timer)
 				tcp_splice_timer(c, flow);
 			break;
+		case FLOW_PING4:
+		case FLOW_PING6:
+			if (timer)
+				closed = icmp_ping_timer(c, flow, now);
+			break;
 		default:
 			/* Assume other flow types don't need any handling */
 			;
diff --git a/flow.h b/flow.h
index e1aeeed..da2a629 100644
--- a/flow.h
+++ b/flow.h
@@ -19,6 +19,10 @@ enum flow_type {
 	FLOW_TCP,
 	/* A TCP connection between a host socket and ns socket */
 	FLOW_TCP_SPLICE,
+	/* ICMP echo requests from guest to host and matching replies back */
+	FLOW_PING4,
+	/* ICMPv6 echo requests from guest to host and matching replies back */
+	FLOW_PING6,
 
 	FLOW_NUM_TYPES,
 };
diff --git a/flow_table.h b/flow_table.h
index 34fc679..091b430 100644
--- a/flow_table.h
+++ b/flow_table.h
@@ -8,6 +8,7 @@
 #define FLOW_TABLE_H
 
 #include "tcp_conn.h"
+#include "icmp_flow.h"
 
 /**
  * struct flow_free_block - Information about a block of free entries
@@ -33,6 +34,7 @@ union flow {
 	struct flow_free_block free;
 	struct tcp_tap_conn tcp;
 	struct tcp_splice_conn tcp_splice;
+	struct icmp_ping_flow ping;
 };
 
 /* Global Flow Table */
diff --git a/icmp.c b/icmp.c
index d669351..27b150d 100644
--- a/icmp.c
+++ b/icmp.c
@@ -37,24 +37,15 @@
 #include "tap.h"
 #include "log.h"
 #include "icmp.h"
+#include "flow_table.h"
 
 #define ICMP_ECHO_TIMEOUT	60 /* s, timeout for ICMP socket activity */
 #define ICMP_NUM_IDS		(1U << 16)
 
-/**
- * struct icmp_id_sock - Tracking information for single ICMP echo identifier
- * @sock:	Bound socket for identifier
- * @seq:	Last sequence number sent to tap, host order, -1: not sent yet
- * @ts:		Last associated activity from tap, seconds
- */
-struct icmp_id_sock {
-	int sock;
-	int seq;
-	time_t ts;
-};
+#define PINGF(idx)		(&(FLOW(idx)->ping))
 
 /* Indexed by ICMP echo identifier */
-static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
+static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 
 /**
  * icmp_sock_handler() - Handle new data from ICMP or ICMPv6 socket
@@ -64,8 +55,8 @@ static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
  */
 void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref)
 {
-	struct icmp_id_sock *const id_map = af == AF_INET
-		? &icmp_id_map[V4][ref.icmp.id] : &icmp_id_map[V6][ref.icmp.id];
+	struct icmp_ping_flow *pingf = af == AF_INET
+		? icmp_id_map[V4][ref.icmp.id] : icmp_id_map[V6][ref.icmp.id];
 	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	char buf[USHRT_MAX];
 	union {
@@ -80,6 +71,8 @@ void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref
ref)
 	if (c->no_icmp)
 		return;
 
+	ASSERT(pingf);
+
 	n = recvfrom(ref.fd, buf, sizeof(buf), 0, &sr.sa, &sl);
 	if (n < 0) {
 		warn("%s: recvfrom() error on ping socket: %s",
@@ -113,10 +106,10 @@ void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref
ref)
 	}
 
 	if (c->mode == MODE_PASTA) {
-		if (id_map->seq == seq)
+		if (pingf->seq == seq)
 			return;
 
-		id_map->seq = seq;
+		pingf->seq = seq;
 	}
 
 	debug("%s: echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, pname,
@@ -133,16 +126,19 @@ unexpected:
 }
 
 /**
- * icmp_ping_close() - Close out and cleanup a ping sequence
+ * icmp_ping_close() - Close out and cleanup a ping flow
  * @c:		Execution context
- * @id_map:	id map entry of the sequence to close
+ * @pingf:	ping flow entry to close
  */
-static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_map)
+static void icmp_ping_close(const struct ctx *c, struct icmp_ping_flow *pingf)
 {
-	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
-	close(id_map->sock);
-	id_map->sock = -1;
-	id_map->seq = -1;
+	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, pingf->sock, NULL);
+	close(pingf->sock);
+
+	if (pingf->f.type == FLOW_PING4)
+		icmp_id_map[V4][pingf->id] = NULL;
+	else
+		icmp_id_map[V6][pingf->id] = NULL;
 }
 
 /**
@@ -152,18 +148,24 @@ static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock
*id_map)
  * @af:		Address family, AF_INET or AF_INET6
  * @id:		ICMP id for the new sequence
  *
- * Return: Newly opened ping socket fd, or -1 on failure
+ * Return: Newly opened ping flow, or NULL on failure
  */
-static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *id_map,
-			 int af, uint16_t id)
+static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
+					    struct icmp_ping_flow **id_map,
+					    int af, uint16_t id)
 {
-	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
 	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
+	uint8_t flowtype = af == AF_INET ? FLOW_PING4 : FLOW_PING6;
 	union icmp_epoll_ref iref = { .id = id };
+	union flow *flow = flow_alloc();
+	struct icmp_ping_flow *pingf;
 	const void *bind_addr;
 	const char *bind_if;
 	int s;
 
+	if (!flow)
+		return NULL;
+
 	if (af == AF_INET) {
 		bind_addr = &c->ip4.addr_out;
 		bind_if = c->ip4.ifname_out;
@@ -172,7 +174,7 @@ static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock
*id_map,
 		bind_if = c->ip6.ifname_out;
 	}
 
-	s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
+	s = sock_l4(c, af, flow_proto[flowtype], bind_addr, bind_if, 0, iref.u32);
 
 	if (s < 0) {
 		warn("Cannot open \"ping\" socket. You might need to:");
@@ -184,16 +186,23 @@ static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock
*id_map,
 	if (s > FD_REF_MAX)
 		goto cancel;
 
-	id_map->sock = s;
+	pingf = &flow->ping;
+	pingf->f.type = flowtype;
+	pingf->seq = -1;
+	pingf->sock = s;
+	pingf->id = id;
+
+	*id_map = pingf;
 
 	debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
 
-	return s;
+	return pingf;
 
 cancel:
 	if (s >= 0)
 		close(s);
-	return -1;
+	flow_alloc_cancel(flow);
+	return NULL;
 }
 
 /**
@@ -219,11 +228,10 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		struct sockaddr_in6 sa6;
 	} sa = { .sa.sa_family = af };
 	const socklen_t sl = af == AF_INET ? sizeof(sa.sa4) : sizeof(sa.sa6);
-	struct icmp_id_sock *id_map;
+	struct icmp_ping_flow *pingf, **id_map;
 	uint16_t id, seq;
 	size_t plen;
 	void *pkt;
-	int s;
 
 	(void)saddr;
 	(void)pif;
@@ -263,13 +271,13 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		ASSERT(0);
 	}
 
-	if ((s = id_map->sock) < 0)
-		if ((s = icmp_ping_new(c, id_map, af, id)) < 0)
+	if (!(pingf = *id_map))
+		if (!(pingf = icmp_ping_new(c, id_map, af, id)))
 			return 1;
 
-	id_map->ts = now->tv_sec;
+	pingf->ts = now->tv_sec;
 
-	if (sendto(s, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
+	if (sendto(pingf->sock, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
 		debug("%s: failed to relay request to socket: %s",
 		      pname, strerror(errno));
 	} else {
@@ -281,44 +289,21 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 }
 
 /**
- * icmp_timer_one() - Handler for timed events related to a given identifier
- * @c:		Execution context
- * @id_map:	id socket information to check timers for
- * @now:	Current timestamp
- */
-static void icmp_timer_one(const struct ctx *c, struct icmp_id_sock *id_map,
-			   const struct timespec *now)
-{
-	if (id_map->sock < 0 || now->tv_sec - id_map->ts <= ICMP_ECHO_TIMEOUT)
-		return;
-
-	icmp_ping_close(c, id_map);
-}
-
-/**
- * icmp_timer() - Scan activity bitmap for identifiers with timed events
+ * icmp_ping_timer() - Handler for timed events related to a given flow
  * @c:		Execution context
+ * @flow:	flow table entry to check for timeout
  * @now:	Current timestamp
+ *
+ * Return: true if the flow is ready to free, false otherwise
  */
-void icmp_timer(const struct ctx *c, const struct timespec *now)
+bool icmp_ping_timer(const struct ctx *c, union flow *flow,
+		     const struct timespec *now)
 {
-	unsigned int i;
-
-	for (i = 0; i < ICMP_NUM_IDS; i++) {
-		icmp_timer_one(c, &icmp_id_map[V4][i], now);
-		icmp_timer_one(c, &icmp_id_map[V6][i], now);
-	}
-}
+	struct icmp_ping_flow *pingf = &flow->ping;
 
-/**
- * icmp_init() - Initialise sequences in ID map to -1 (no sequence sent yet)
- */
-void icmp_init(void)
-{
-	unsigned i;
+	if (now->tv_sec - pingf->ts <= ICMP_ECHO_TIMEOUT)
+		return false;
 
-	for (i = 0; i < ICMP_NUM_IDS; i++) {
-		icmp_id_map[V4][i].seq = icmp_id_map[V6][i].seq = -1;
-		icmp_id_map[V4][i].sock = icmp_id_map[V6][i].sock = -1;
-	}
+	icmp_ping_close(c, pingf);
+	return true;
 }
diff --git a/icmp.h b/icmp.h
index 0083597..2897f31 100644
--- a/icmp.h
+++ b/icmp.h
@@ -9,12 +9,12 @@
 #define ICMP_TIMER_INTERVAL		10000 /* ms */
 
 struct ctx;
+struct icmp_ping_flow;
 
 void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref);
 int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		     const void *saddr, const void *daddr,
 		     const struct pool *p, const struct timespec *now);
-void icmp_timer(const struct ctx *c, const struct timespec *now);
 void icmp_init(void);
 
 /**
diff --git a/icmp_flow.h b/icmp_flow.h
new file mode 100644
index 0000000..7b7f4ee
--- /dev/null
+++ b/icmp_flow.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later
+ * Copyright Red Hat
+ * Author: David Gibson &lt;david(a)gibson.dropbear.id.au&gt;
+ *
+ * ICMP flow tracking data structures
+ */
+#ifndef ICMP_FLOW_H
+#define ICMP_FLOW_H
+
+/**
+ * struct icmp_ping_flow - Descriptor for a flow of ping requests/replies
+ * @f:		Generic flow information
+ * @seq:	Last sequence number sent to tap, host order, -1: not sent yet
+ * @sock:	"ping" socket
+ * @ts:		Last associated activity from tap, seconds
+ * @id:		Guest side ICMP ping id for this flow
+ */
+struct icmp_ping_flow {
+	/* Must be first element */
+	struct flow_common f;
+
+	int seq;
+	int sock;
+	time_t ts;
+	uint16_t id;
+};
+
+bool icmp_ping_timer(const struct ctx *c, union flow *flow,
+		     const struct timespec *now);
+
+#endif /* ICMP_FLOW_H */
diff --git a/passt.c b/passt.c
index 44d3a0b..9191404 100644
--- a/passt.c
+++ b/passt.c
@@ -105,8 +105,6 @@ static void post_handler(struct ctx *c, const struct timespec *now)
 	CALL_PROTO_HANDLER(c, now, tcp, TCP);
 	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
 	CALL_PROTO_HANDLER(c, now, udp, UDP);
-	/* NOLINTNEXTLINE(bugprone-branch-clone): intervals can be the same */
-	CALL_PROTO_HANDLER(c, now, icmp, ICMP);
 
 	flow_defer_handler(c, now);
 #undef CALL_PROTO_HANDLER
@@ -290,9 +288,6 @@ int main(int argc, char **argv)
 	if ((!c.no_udp && udp_init(&c)) || (!c.no_tcp && tcp_init(&c)))
 		exit(EXIT_FAILURE);
 
-	if (!c.no_icmp)
-		icmp_init();
-
 	proto_update_l2_buf(c.mac_guest, c.mac);
 
 	if (c.ifi4 && !c.no_dhcp)
-- 
2.43.0

Complete filling out the common flow information for "ping" flows by
storing the host side information for the ping socket.  We can only
retrieve this from the socket after we send the echo-request, because
that's when the kernel will assign an ID.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 icmp.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/icmp.c b/icmp.c
index 53ad087..917c810 100644
--- a/icmp.c
+++ b/icmp.c
@@ -310,11 +310,41 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 	if (sendto(pingf->sock, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) {
 		debug("%s: failed to relay request to socket: %s",
 		      pname, strerror(errno));
-	} else {
-		debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16,
-		      pname, id, seq);
+		if (flow)
+			goto cancel;
+	}
+
+	debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16,
+	      pname, id, seq);
+
+	if (!flow)
+		/* Nothing more to do for an existing flow */
+		return 1;
+
+	/* We need to wait until after the sendto() to fill in the SOCKSIDE
+	 * information, so that we can find out the host side id the kernel
+	 * assigned.  If there's no bind address specified, this will still have
+	 * 0.0.0.0 or :: as the host side forwarding address.  There's not
+	 * really anything we can do to fill that in, which means we can never
+	 * insert the SOCKSIDE of a ping flow into the hash table.
+	 */
+	if (flowside_from_sock(SOCKFSIDE(pingf), PIF_HOST, pingf->sock,
+			       NULL, &sa) < 0) {
+		err("%s: Failed to get local name for outgoing ping socket",
+		    pname);
+		goto cancel;
 	}
+	/* We want the id as the "port" on both sides */
+	SOCKFSIDE(pingf)->eport = SOCKFSIDE(pingf)->fport;
+
+	FLOW_FWD_DBG(pingf, SOCKSIDE);
+
+	return 1;
 
+cancel:
+	/* Something went wrong, back out creation of the flow */
+	icmp_ping_close(c, pingf);
+	flow_alloc_cancel(flow);
 	return 1;
 }
 
-- 
2.43.0

We have separate epoll reference types for ICMP and ICMPv6 ping sockets.
Now that ping socket references point to a flow table entry, we can easily
determine whether we're dealing with ICMP or ICMPv6 from the flow type,
making the two epoll types redundant.

Merge both types into EPOLL_TYPE_PING.

Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
---
 icmp.c  | 10 ++++------
 icmp.h  |  2 +-
 passt.c | 10 +++-------
 passt.h |  6 ++----
 util.c  |  4 +---
 5 files changed, 11 insertions(+), 21 deletions(-)

diff --git a/icmp.c b/icmp.c
index a073813..272edae 100644
--- a/icmp.c
+++ b/icmp.c
@@ -57,13 +57,13 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
 /**
  * icmp_sock_handler() - Handle new data from ICMP or ICMPv6 socket
  * @c:		Execution context
- * @af:		Address family (AF_INET or AF_INET6)
  * @ref:	epoll reference
  */
-void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref)
+void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
 {
-	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	struct icmp_ping_flow *pingf = PINGF(ref.flowside.flow);
+	int af = pingf->f.type == FLOW_PING4 ? AF_INET : AF_INET6;
+	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	char buf[USHRT_MAX];
 	union {
 		struct sockaddr sa;
@@ -172,22 +172,20 @@ static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c,
 {
 	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 	uint8_t flowtype = af == AF_INET ? FLOW_PING4 : FLOW_PING6;
+	union epoll_ref ref = { .type = EPOLL_TYPE_PING };
 	union flow *flow = flow_alloc();
 	struct icmp_ping_flow *pingf;
 	const void *bind_addr;
 	const char *bind_if;
-	union epoll_ref ref;
 	int s;
 
 	if (!flow)
 		return NULL;
 
 	if (af == AF_INET) {
-		ref.type = EPOLL_TYPE_ICMP;
 		bind_addr = &c->ip4.addr_out;
 		bind_if = c->ip4.ifname_out;
 	} else {
-		ref.type = EPOLL_TYPE_ICMPV6;
 		bind_addr = &c->ip6.addr_out;
 		bind_if = c->ip6.ifname_out;
 	}
diff --git a/icmp.h b/icmp.h
index 50807c4..5642dda 100644
--- a/icmp.h
+++ b/icmp.h
@@ -11,7 +11,7 @@
 struct ctx;
 struct icmp_ping_flow;
 
-void icmp_sock_handler(const struct ctx *c, int af, union epoll_ref ref);
+void icmp_sock_handler(const struct ctx *c, union epoll_ref ref);
 int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 		     const void *saddr, const void *daddr,
 		     const struct pool *p, const struct timespec *now);
diff --git a/passt.c b/passt.c
index 9191404..eb9f6d8 100644
--- a/passt.c
+++ b/passt.c
@@ -66,8 +66,7 @@ char *epoll_type_str[] = {
 	[EPOLL_TYPE_TCP_LISTEN]	= "listening TCP socket",
 	[EPOLL_TYPE_TCP_TIMER]	= "TCP timer",
 	[EPOLL_TYPE_UDP]	= "UDP socket",
-	[EPOLL_TYPE_ICMP]	= "ICMP socket",
-	[EPOLL_TYPE_ICMPV6]	= "ICMPv6 socket",
+	[EPOLL_TYPE_PING]	= "ICMP/ICMPv6 ping socket",
 	[EPOLL_TYPE_NSQUIT]	= "namespace inotify",
 	[EPOLL_TYPE_TAP_PASTA]	= "/dev/net/tun device",
 	[EPOLL_TYPE_TAP_PASST]	= "connected qemu socket",
@@ -386,11 +385,8 @@ loop:
 		case EPOLL_TYPE_UDP:
 			udp_sock_handler(&c, ref, eventmask, &now);
 			break;
-		case EPOLL_TYPE_ICMP:
-			icmp_sock_handler(&c, AF_INET, ref);
-			break;
-		case EPOLL_TYPE_ICMPV6:
-			icmp_sock_handler(&c, AF_INET6, ref);
+		case EPOLL_TYPE_PING:
+			icmp_sock_handler(&c, ref);
 			break;
 		default:
 			/* Can't happen */
diff --git a/passt.h b/passt.h
index d1ed716..372eb64 100644
--- a/passt.h
+++ b/passt.h
@@ -61,10 +61,8 @@ enum epoll_type {
 	EPOLL_TYPE_TCP_TIMER,
 	/* UDP sockets */
 	EPOLL_TYPE_UDP,
-	/* IPv4 ICMP sockets */
-	EPOLL_TYPE_ICMP,
-	/* ICMPv6 sockets */
-	EPOLL_TYPE_ICMPV6,
+	/* ICMP/ICMPv6 ping sockets */
+	EPOLL_TYPE_PING,
 	/* inotify fd watching for end of netns (pasta) */
 	EPOLL_TYPE_NSQUIT,
 	/* tuntap character device */
diff --git a/util.c b/util.c
index 9c7012c..ab08006 100644
--- a/util.c
+++ b/util.c
@@ -125,10 +125,8 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto,
 		ref.type = EPOLL_TYPE_UDP;
 		break;
 	case IPPROTO_ICMP:
-		ref.type = EPOLL_TYPE_ICMP;
-		break;
 	case IPPROTO_ICMPV6:
-		ref.type = EPOLL_TYPE_ICMPV6;
+		ref.type = EPOLL_TYPE_PING;
 		break;
 	default:
 		return -EPFNOSUPPORT;	/* Not implemented. */
-- 
2.43.0