On Fri, Mar 08, 2024 at 12:25:44AM +0100, Stefano Brivio wrote:On Thu, 7 Mar 2024 21:24:28 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Sounds good.On Thu, Mar 07, 2024 at 09:53:15AM +0100, Stefano Brivio wrote:I can change it according to your comment below: * @id_sock: Pointer to ping flow entry slot in icmp_id_map[] to update ?Apologies for the delay, I'm still reviewing 3/3, but I have a question meanwhile: On Thu, 29 Feb 2024 15:15:32 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Oops.Currently icmp_id_map[][] stores information about ping sockets in a bespoke structure. Move the same information into new types of flow 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. We convert icmp_id_map[][] to point to the flow table entries, rather than containing its own information. We do still use that array for locating the right ping flows, rather than using a "flow native" form of lookup for the time being. 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 | 143 +++++++++++++++++++++++---------------------------- icmp.h | 2 +- icmp_flow.h | 31 +++++++++++ passt.c | 5 -- 8 files changed, 115 insertions(+), 87 deletions(-) create mode 100644 icmp_flow.h diff --git a/Makefile b/Makefile index 2d6a5155..47fc5448 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 fwd.h \ - flow_table.h icmp.h inany.h iov.h isolation.h lineread.h log.h ndp.h \ - netlink.h packet.h passt.h pasta.h pcap.h pif.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 iov.h isolation.h lineread.h \ + log.h ndp.h netlink.h packet.h passt.h pasta.h pcap.h pif.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 d7974d59..5835d6c0 100644 --- a/flow.c +++ b/flow.c @@ -21,6 +21,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"); @@ -28,6 +30,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"); @@ -294,6 +298,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 8b66751b..c943c441 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 eecf8844..b7e5529a 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_cluster - Information about a cluster of free entries @@ -33,6 +34,7 @@ union flow { struct flow_free_cluster 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 fb2fcafc..1caf791d 100644 --- a/icmp.c +++ b/icmp.c @@ -39,24 +39,17 @@ #include "siphash.h" #include "inany.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; -}; +/* Sides of a flow as we use them for ping streams */ +#define SOCKSIDE 0 +#define TAPSIDE 1 /* 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 @@ -66,8 +59,8 @@ static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; */ void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref) { - struct icmp_id_sock *const id_sock = 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"; union sockaddr_inany sr; socklen_t sl = sizeof(sr); @@ -78,6 +71,8 @@ void icmp_sock_handler(const struct ctx *c, sa_family_t 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", @@ -112,10 +107,10 @@ void icmp_sock_handler(const struct ctx *c, sa_family_t af, union epoll_ref ref) /* In PASTA mode, we'll get any reply we send, discard them. */ if (c->mode == MODE_PASTA) { - if (id_sock->seq == seq) + if (pingf->seq == seq) return; - id_sock->seq = seq; + pingf->seq = seq; } debug("%s: echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, pname, @@ -132,16 +127,22 @@ unexpected: } /** - * icmp_ping_close() - Close and clean up a ping socket + * icmp_ping_close() - Close and clean up a ping flow * @c: Execution context - * @id_sock: Socket number and other info + * @pingf: ping flow entry to close */ -static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_sock) +static void icmp_ping_close(const struct ctx *c, + const struct icmp_ping_flow *pingf) { - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_sock->sock, NULL); - close(id_sock->sock); - id_sock->sock = -1; - id_sock->seq = -1; + uint16_t id = pingf->id; + + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, pingf->sock, NULL); + close(pingf->sock); + + if (pingf->f.type == FLOW_PING4) + icmp_id_map[V4][id] = NULL; + else + icmp_id_map[V6][id] = NULL; } /** @@ -151,17 +152,27 @@ static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_sock) * @af: Address family, AF_INET or AF_INET6 * @id: ICMP id for the new socket * - * 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_sock, - sa_family_t af, uint16_t id) +static struct icmp_ping_flow *icmp_ping_new(const struct ctx *c, + struct icmp_ping_flow **id_sock,I'm not quite sure why we still need id_sock passed as parameter, and what it's supposed to contain (you haven't updated the function comment).-- 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/~dgibsonAh, right, of course, I thought the caller could/should take care of updating the slot, too, but now I understand the commit message, thanks.Now that all the information is encapsulated in the flow, and you return the new flow, with a trivial change in icmp_tap_handler(), couldn't we just drop id_sock here?No, because this is only the "partial" flow table implementation, lacking the address information in the common part of the flow. Without that, while the information on a single flow is in the flow table, we still need the icmp_id_map[] arrays to *find* the relevant flow. id_sock passed here is the relevant "slot" in those arrays, so we can update them to index the new flow (that's why it's a (ping_flow **) not a (ping_flow *)).