Re: [PATCH 1/3] icmp: Store ping socket information in flow table

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

[-- Attachment #1: Type: text/plain, Size: 11061 bytes --]

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@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au> 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 <david@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).  
> > 
> > Oops.
> 
> I can change it according to your comment below:
> 
>  * @id_sock:	Pointer to ping flow entry slot in icmp_id_map[] to update
> 
> ?

Sounds good.

> > > 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.
> 

-- 
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/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]