From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id CD6EF5A026D for ; Fri, 8 Mar 2024 00:26:44 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1709854003; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Go8BfAisHxujtTt2GmEqhUG/JhTNNd6DM6NJcWIJu1Q=; b=a/HR7wDQJlhHvFb3XFm8baUn5BI7j7h6v7nKQ3e0qbhV0O7xyzoMu1kdhf9UsT7c/qpS3s S4yrCTkB86SWsZ05X9vu7mWcfeBFBEfwbLSBks0hWph4LIsxAQ7ryQvplg6xUYEvjYQNST d/E7U8yt41FLI6ekO/81LNlGTfVsoms= Received: from mail-lf1-f69.google.com (mail-lf1-f69.google.com [209.85.167.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-663-j9L4iXAjPPqm4eCdduDoQQ-1; Thu, 07 Mar 2024 18:26:41 -0500 X-MC-Unique: j9L4iXAjPPqm4eCdduDoQQ-1 Received: by mail-lf1-f69.google.com with SMTP id 2adb3069b0e04-5136f815d28so1788162e87.1 for ; Thu, 07 Mar 2024 15:26:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1709854000; x=1710458800; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=Go8BfAisHxujtTt2GmEqhUG/JhTNNd6DM6NJcWIJu1Q=; b=REaS6I5g5szkQ6oGBqV3Mwbqn7YoMNy3oIKIIrZ/YLMdSpy0HEzGqLbNYBAaV+IBFM Z2ZMip6nFoxuqr6BwwZO6ObHGgJyyzijy9nD9V9epzP5HNChA5/Y7aKbab++IHTRIhpn 3rZ9s4NtIyS92sdDRlXQ3MO8HRsNPNBF7aBiizqjD2NU/acV4kMdW94NK+333Wr1Fzvu YdV2SKSRW98Ev/BUsNy9rRRr8hlQg4aYxdQYS9OByQo84+TxfJskD7UEbhpjvWrEBtkw HeLXd6DvCdJcwlvPlUdqplHsawHM2KTSOB+FzhSWta3aANAAA/gf7NgE8D2+EpuIGjK/ sxmg== X-Gm-Message-State: AOJu0YyzQRFvjix/Si8uH3AXZgj7dXOf6wuTl9tqshGCetKwRRs9Tywo swGO8SX/ZqDBU9hDDJ8FKnFPyn15+OFw/hfcXhII6Ssyk6zBQLjNMHyXfEjhy5AotPbCYeJ75bg uXiAM9Pr9PlJemQIwlOco2KAqOL56HkIm+v0eCcPy5kS0wdSdzCaI9MHDESvI X-Received: by 2002:ac2:5bc6:0:b0:513:5bdb:10f7 with SMTP id u6-20020ac25bc6000000b005135bdb10f7mr2352824lfn.14.1709853999781; Thu, 07 Mar 2024 15:26:39 -0800 (PST) X-Google-Smtp-Source: AGHT+IGJUP8saoDAYwRtg46UJX1ucj0HmwkO22NXVyKiCF2dHBiJ0Wk/cHD7pJoXTmd/Oic7UEqsvw== X-Received: by 2002:ac2:5bc6:0:b0:513:5bdb:10f7 with SMTP id u6-20020ac25bc6000000b005135bdb10f7mr2352803lfn.14.1709853998988; Thu, 07 Mar 2024 15:26:38 -0800 (PST) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id os26-20020a170906af7a00b00a450b817705sm5826845ejb.154.2024.03.07.15.26.38 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 07 Mar 2024 15:26:38 -0800 (PST) Date: Fri, 8 Mar 2024 00:25:44 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 1/3] icmp: Store ping socket information in flow table Message-ID: <20240308002544.59c2c09e@elisabeth> In-Reply-To: References: <20240229041534.2573559-1-david@gibson.dropbear.id.au> <20240229041534.2573559-2-david@gibson.dropbear.id.au> <20240307095315.36eeef59@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 27LUVRAGTRKJHL52D2DGBJ6RWUZBYETV X-Message-ID-Hash: 27LUVRAGTRKJHL52D2DGBJ6RWUZBYETV X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 7 Mar 2024 21:24:28 +1100 David Gibson wrote: > On Thu, Mar 07, 2024 at 09:53:15AM +0100, Stefano Brivio wrote: > > 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 wrote: > > > > > 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 > > > --- > > > 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 \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] = "", > > > [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). > > Oops. I can change it according to your comment below: * @id_sock: Pointer to ping flow entry slot in icmp_id_map[] to update ? > > 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 *)). Ah, right, of course, I thought the caller could/should take care of updating the slot, too, but now I understand the commit message, thanks. -- Stefano