On Thu, 21 Dec 2023 17:53:19 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  Linux ICMP "ping" sockets are very specific
in what they do.  They let
 userspace send ping requests (ICMP_ECHO or ICMP6_ECHO_REQUEST), and receive
 matching replies (ICMP_ECHOREPLY or ICMP6_ECHO_REPLY).  They don't let you
 intercept or handle incoming ping requests. 
Right... I don't know exactly what I was trying to do with this, back
then. By the way this is the main reason why it took me a while to
review this series... did I really write all those checks without a
purpose? :) Well, it looks like it.

Anyway, if you look at ping_err() in net/ipv4/ping.c, you'll see that
among the messages which can be sent back as error (they're received on
the socket causing the error -- same as ICMP messages you get on a UDP
socket for port unreachable), ICMP_ECHO is allowed (see
ping_supported()).

I think I just used that ping_supported() function to find out which
messages we could get on the socket. But we're not going to get those
anyway.

By the way, ICMP{,V6}_EXT_ECHO{,_REQUEST} support (RFC 8335, PROBE
messages) was added a while ago (kernel commit 08baf54f01f5 "net: add
support for sending RFC 8335 PROBE messages")... we should add that at
some point, it's kind of trivial.

...
  In the case of passt/pasta that means we can process
echo requests from tap
 and forward them to a ping socket, then take the replies from the ping
 socket and forward them to tap.  We can't do the reverse: take echo
 requests from the host and somehow forward them to the guest. There's
 really no way for something outside to initiate a ping to a passt/pasta
 connected guest and if there was we'd need an entirely different mechanism
 to handle it.
 
 However, we have some logic to deal with packets going in that reverse
 direction.  Remove it, since it can't ever be used that way.  While we're
 there use defines for the ICMPv6 types, instead of open coded type values. 
I guess this last sentence only applied to a previous version of your
patch. It doesn't matter so much, but it would be nice to drop if you
re-spin.

...
  Signed-off-by: David Gibson
<david(a)gibson.dropbear.id.au>
 ---
  icmp.c | 16 ++++++----------
  1 file changed, 6 insertions(+), 10 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index c745b7b..1f41440 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -93,8 +93,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
  		icmp_id_map[V4][id].seq = seq;
  	}
  
 -	debug("ICMP: echo %s to tap, ID: %i, seq: %i",
 -	      (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq);
 +	debug("ICMP: echo reply to tap, ID: %i, seq: %i", id, seq);
  
  	tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n);
  }
 @@ -138,8 +137,7 @@ void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref)
  		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);
 +	debug("ICMPv6: echo reply to tap, ID: %i, seq: %i", id, seq);
  
  	tap_icmp6_send(c, &sr.sin6_addr,
  		       tap_ip6_daddr(c, &sr.sin6_addr), buf, n);
 @@ -178,7 +176,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  		if (!ih)
  			return 1;
  
 -		if (ih->type != ICMP_ECHO && ih->type != ICMP_ECHOREPLY)
 +		if (ih->type != ICMP_ECHO)
  			return 1;
  
  		iref.id = id = ntohs(ih->un.echo.id);
 @@ -205,8 +203,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  			   (struct sockaddr *)&sa, sizeof(sa)) < 0) {
  			debug("ICMP: failed to relay request to socket");
  		} else {
 -			debug("ICMP: echo %s to socket, ID: %i, seq: %i",
 -			      (ih->type == ICMP_ECHO) ? "request" : "reply",
 +			debug("ICMP: echo request to socket, ID: %i, seq: %i",
  			      id, ntohs(ih->un.echo.sequence));
  		}
  	} else if (af == AF_INET6) {
 @@ -222,7 +219,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  		if (!ih)
  			return 1;
  
 -		if (ih->icmp6_type != 128 && ih->icmp6_type != 129)
 +		if (ih->icmp6_type != ICMPV6_ECHO_REQUEST)
  			return 1;
  
  		iref.id = id = ntohs(ih->icmp6_identifier);
 @@ -249,8 +246,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  			   (struct sockaddr *)&sa, sizeof(sa)) < 1) {
  			debug("ICMPv6: failed to relay request to socket");
  		} else {
 -			debug("ICMPv6: echo %s to socket, ID: %i, seq: %i",
 -			      (ih->icmp6_type == 128) ? "request" : "reply",
 +			debug("ICMPv6: echo request to socket, ID: %i, seq: %i",
  			      id, ntohs(ih->icmp6_sequence));
  		}
  	} 

On Sun, 7 Jan 2024 11:37:46 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  On Sat, Jan 06, 2024 at 04:59:11PM +0100, Stefano
Brivio wrote:
  On Thu, 21 Dec 2023 17:53:19 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
   
  Linux ICMP "ping" sockets are very
specific in what they do.  They let
 userspace send ping requests (ICMP_ECHO or ICMP6_ECHO_REQUEST), and receive
 matching replies (ICMP_ECHOREPLY or ICMP6_ECHO_REPLY).  They don't let you
 intercept or handle incoming ping requests.   
 
 Right... I don't know exactly what I was trying to do with this, back
 then. By the way this is the main reason why it took me a while to
 review this series... did I really write all those checks without a
 purpose? :) Well, it looks like it.
 
 Anyway, if you look at ping_err() in net/ipv4/ping.c, you'll see that
 among the messages which can be sent back as error (they're received on
 the socket causing the error -- same as ICMP messages you get on a UDP
 socket for port unreachable), ICMP_ECHO is allowed (see
 ping_supported()).
 
 I think I just used that ping_supported() function to find out which
 messages we could get on the socket. But we're not going to get those
 anyway.   
 
 Right.
 
  By the way, ICMP{,V6}_EXT_ECHO{,_REQUEST} support
(RFC 8335, PROBE
 messages) was added a while ago (kernel commit 08baf54f01f5 "net: add
 support for sending RFC 8335 PROBE messages")... we should add that at
 some point, it's kind of trivial.   
 
 Indeed.  Want to make a BZ for it so we don't forget? 
Done, it's https://bugs.passt.top/show_bug.cgi?id=78.

...
 
 
  In the case of passt/pasta that means we can process
echo requests from tap
 and forward them to a ping socket, then take the replies from the ping
 socket and forward them to tap.  We can't do the reverse: take echo
 requests from the host and somehow forward them to the guest. There's
 really no way for something outside to initiate a ping to a passt/pasta
 connected guest and if there was we'd need an entirely different mechanism
 to handle it.
 
 However, we have some logic to deal with packets going in that reverse
 direction.  Remove it, since it can't ever be used that way.  While we're
 there use defines for the ICMPv6 types, instead of open coded type values.   
 
 I guess this last sentence only applied to a previous version of your
 patch. It doesn't matter so much, but it would be nice to drop if you
 re-spin.   
 
 Uh... no.. that change is right:
 
 [snip]
  > @@ -222,7 +219,7 @@ int
icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
 >  		if (!ih)
 >  			return 1;
 >  
 > -		if (ih->icmp6_type != 128 && ih->icmp6_type != 129)
 > +		if (ih->icmp6_type != ICMPV6_ECHO_REQUEST)   
 
 here. 
Ah, sorry, I missed this one.

-- 
Stefano

On Sat, Jan 06, 2024 at 04:59:32PM +0100, Stefano Brivio wrote:
...
  On Thu, 21 Dec 2023 17:53:21 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  icmp_id_map[] contains, amongst other things, fds
for "ping" sockets
 associated with various ICMP echo ids.  However, we only lazily open()
 those sockets, so many will be missing.  We currently represent that with
 a 0, which isn't great, since that's technically a valid fd.  Use -1
 instead.  This does require initializing the fields in icmp_id_map[] but
 we already have an obvious place to do that.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  icmp.c | 10 ++++++----
  1 file changed, 6 insertions(+), 4 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index 7a505b4..dd98c7f 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -179,7 +179,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  
  		iref.id = id = ntohs(ih->un.echo.id);
  
 -		if ((s = icmp_id_map[V4][id].sock) <= 0) {
 +		if ((s = icmp_id_map[V4][id].sock) < 0) {
  			s = sock_l4(c, AF_INET, IPPROTO_ICMP, &c->ip4.addr_out,
  				    c->ip4.ifname_out, 0, iref.u32);
  			if (s < 0)
 @@ -221,7 +221,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  			return 1;
  
  		iref.id = id = ntohs(ih->icmp6_identifier);
 -		if ((s = icmp_id_map[V6][id].sock) <= 0) {
 +		if ((s = icmp_id_map[V6][id].sock) < 0) {
  			s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6,
  				    &c->ip6.addr_out,
  				    c->ip6.ifname_out, 0, iref.u32);
 @@ -277,7 +277,7 @@ static void icmp_timer_one(const struct ctx *c, int v6, uint16_t id,
  
  	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
  	close(id_map->sock);
 -	id_map->sock = 0;
 +	id_map->sock = -1;
  	id_map->seq = -1;
  }
  
 @@ -315,6 +315,8 @@ void icmp_init(void)
  {
  	unsigned i;
  
 -	for (i = 0; i < ICMP_NUM_IDS; i++)
 +	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;
 +	} 
 
 Another (well, kind of existing) use case for a 0xff-initialising
 helper. 
Hm.. it's still not every field, so only sorta.

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

On Thu, 21 Dec 2023 17:53:22 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  Currently we use icmp_act[] to scan for ICMP ids which
might have an open
 socket which could time out.  However icmp_act[] contains no information
 that's not already in icmp_id_map[] - it's just an "index" which
allows
 scanning for relevant entries with less cache footprint.
 
 We only scan for ICMP socket expiry every 1s, though, so it's not clear
 that cache footprint really matters.  Furthermore, there's no strong reason
 we need to scan even that often - the timeout is fairly arbitrary and
 approximate.
 
 So, eliminate icmp_act[] in favour of directly scanning icmp_id_map[] and
 compensate for the cache impact by reducing the scan frequency to once
 every 10s.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  icmp.c | 34 ++++++----------------------------
  icmp.h |  2 +-
  2 files changed, 7 insertions(+), 29 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index dd98c7f..02739b9 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -56,9 +56,6 @@ struct icmp_id_sock {
  /* Indexed by ICMP echo identifier */
  static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
  
 -/* Bitmaps, activity monitoring needed for identifier */
 -static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)];
 -
  /**
   * icmp_sock_handler() - Handle new data from IPv4 ICMP socket
   * @c:		Execution context
 @@ -194,7 +191,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  			debug("ICMP: new socket %i for echo ID %i", s, id);
  		}
  		icmp_id_map[V4][id].ts = now->tv_sec;
 -		bitmap_set(icmp_act[V4], id);
  
  		sa.sin_addr = *(struct in_addr *)daddr;
  		if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
 @@ -237,7 +233,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  			debug("ICMPv6: new socket %i for echo ID %i", s, id);
  		}
  		icmp_id_map[V6][id].ts = now->tv_sec;
 -		bitmap_set(icmp_act[V6], id);
  
  		sa.sin6_addr = *(struct in6_addr *)daddr;
  		if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
 @@ -261,20 +256,15 @@ fail_sock:
  /**
   * icmp_timer_one() - Handler for timed events related to a given identifier
   * @c:		Execution context
 - * @v6:		Set for IPv6 echo identifier bindings
 - * @id:		Echo identifier, host order
 + * @id_map:	id socket information to check timers for 
It took me a few readings to understand this... what about:

 * @id_sock:	Socket number and activity timestamp

?

-- 
Stefano

On Sat, Jan 06, 2024 at 05:00:21PM +0100, Stefano Brivio wrote:
...
  On Thu, 21 Dec 2023 17:53:23 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  Currently icmp_tap_handler() consists of two
almost disjoint paths for the
 IPv4 and IPv6 cases.  The only thing they share is an error message.
 We can use some intermediate variables to refactor this to share some more
 code between those paths.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  icmp.c | 140 ++++++++++++++++++++++++++++-----------------------------
  1 file changed, 69 insertions(+), 71 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index 02739b9..f6c744a 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -154,102 +154,100 @@ 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)
  {
 +	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
 +	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
 +	union {
 +		struct sockaddr sa;
 +		struct sockaddr_in sa4;
 +		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;
 +	uint16_t id, seq;
  	size_t plen;
 +	void *pkt;
 +	int s;
  
  	(void)saddr;
  	(void)pif;
  
 +	pkt = packet_get(p, 0, 0, 0, &plen);
 +	if (!pkt)
 +		return 1;
 +
  	if (af == AF_INET) {
 -		struct sockaddr_in sa = {
 -			.sin_family = AF_INET,
 -		};
 -		union icmp_epoll_ref iref;
 -		struct icmphdr *ih;
 -		int id, s;
 -
 -		ih = packet_get(p, 0, 0, sizeof(*ih), &plen);
 -		if (!ih)
 +		struct icmphdr *ih = (struct icmphdr *)pkt;
 +
 +		if (plen < sizeof(*ih))
  			return 1; 
 
 This is obviously the same as the existing check. On the other hand,
 I've been trying to use packet_get() to validate any packet read length
 we need.
 
 Here, the first call sets plen, but the only purpose is to get the
 length of the message we need to relay. Given that we need at least
 sizeof(*ih) here, I would prefer keep the explicit packet_get()
 validation.
 
 Same for the IPv6 path below. 
Ok, fair enough.  Updated.

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

On Thu, 21 Dec 2023 17:53:24 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  Currently we have separate handlers for ICMP and
ICMPv6 ping replies.
 Although there are a number of points of difference, with some creative
 refactoring we can combine these together sensibly.  Although it doesn't
 save a vast amount of code, it does make it clearer that we're performing
 basically the same steps for each case.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  icmp.c  | 90 ++++++++++++++++++++++-----------------------------------
  icmp.h  |  3 +-
  passt.c |  4 +--
  3 files changed, 37 insertions(+), 60 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index f6c744a..b26c61f 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -57,85 +57,63 @@ struct icmp_id_sock {
  static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS];
  
  /**
 - * icmp_sock_handler() - Handle new data from IPv4 ICMP socket
 + * 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, union epoll_ref ref)
 +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];
 +	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
  	char buf[USHRT_MAX];
 -	struct icmphdr *ih = (struct icmphdr *)buf;
 -	struct sockaddr_in sr;
 +	union {
 +		struct sockaddr sa;
 +		struct sockaddr_in sa4;
 +		struct sockaddr_in6 sa6;
 +	} sr;
  	socklen_t sl = sizeof(sr);
 -	uint16_t seq, id;
 +	uint16_t seq;
  	ssize_t n;
  
  	if (c->no_icmp)
  		return;
  
 -	n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
 +	n = recvfrom(ref.fd, buf, sizeof(buf), 0, &sr.sa, &sl);
  	if (n < 0)
  		return;
  
 -	seq = ntohs(ih->un.echo.sequence);
 -
 -	/* Adjust the packet to have the ID the guest was using, rather than the
 -	 * host chosen value */
 -	id = ref.icmp.id;
 -	ih->un.echo.id = htons(id);
 +	if (af == AF_INET) {
 +		struct icmphdr *ih4 = (struct icmphdr *)buf;
  
 -	if (c->mode == MODE_PASTA) {
 -		if (icmp_id_map[V4][id].seq == seq)
 -			return;
 +		/* Adjust packet back to guest-side ID */
 +		ih4->un.echo.id = htons(ref.icmp.id);
 +		seq = ntohs(ih4->un.echo.sequence);
 +	} else if (af == AF_INET6) {
 +		struct icmp6hdr *ih6 = (struct icmp6hdr *)buf;
  
 -		icmp_id_map[V4][id].seq = seq;
 +		/* Adjust packet back to guest-side ID */
 +		ih6->icmp6_identifier = htons(ref.icmp.id);
 +		seq = ntohs(ih6->icmp6_sequence);
 +	} else {
 +		ASSERT(0);
  	}
  
 -	debug("ICMP: echo reply to tap, ID: %i, seq: %i", id, seq);
 -
 -	tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n);
 -}
 -
 -/**
 - * 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 (c->no_icmp)
 -		return;
 -
 -	n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl);
 -	if (n < 0)
 -		return;
 -
 -	seq = ntohs(ih->icmp6_sequence);
 -
 -	/* Adjust the packet to have the ID the guest was using, rather than the
 -	 * host chosen value */
 -	id = ref.icmp.id;
 -	ih->icmp6_identifier = htons(id);
 -
 -	/* In PASTA mode, we'll get any reply we send, discard them. */
  	if (c->mode == MODE_PASTA) { 
The comment here should be kept -- the kernel behaviour was rather
surprising to me, and I would have otherwise no idea why we return
early here.

-- 
Stefano

On Sat, Jan 06, 2024 at 05:01:04PM +0100, Stefano Brivio wrote:
...
  On Thu, 21 Dec 2023 17:53:27 +1100
 David Gibson <david(a)gibson.dropbear.id.au> wrote:
 
  ICMP sockets are cleaned up on a timeout
implemented in icmp_timer_one(),
 and the logic to do that cleanup is open coded in that function.  Similarly
 new sockets are opened when we discover we don't have an existing one in
 icmp_tap_handler(), and again the logic is open-coded.
 
 That's not the worst thing, but it's a bit cleaner to have dedicated
 functions for the creation and destruction of ping sockets.  This will also
 make things a bit easier for future changes we have in mind.
 
 Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---
  icmp.c | 102 +++++++++++++++++++++++++++++++++++++--------------------
  1 file changed, 67 insertions(+), 35 deletions(-)
 
 diff --git a/icmp.c b/icmp.c
 index 129a7f1..d669351 100644
 --- a/icmp.c
 +++ b/icmp.c
 @@ -132,6 +132,70 @@ unexpected:
  	warn("%s: Unexpected packet on ping socket", pname);
  }
  
 +/**
 + * icmp_ping_close() - Close out and cleanup a ping sequence 
 
 s/cleanup/clean up/ (verb) 
Done.

...
  The commit title and this comment (referring to a
sequence) are a bit
 misleading... this pretty much closes the socket, and as far as I can
 see setting the sequence to -1 doesn't actually have an effect (despite
 the fact it's a good practice).
 
 If you mean "sequence" not as "sequence number" but rather as a
 "sequence of pings", then I think we should rename one of the two...
 either 'seqnum' for the sequence number, or maybe s/sequence/socket/g
 here. 
Right, I was basically meaning "flow", but avoiding it because this is
not (yet) linked to the flow table.  I've gone with
s/sequence/socket/g, more or less, and I think that works out.

...
  
  + * @c:		Execution context
 + * @id_map:	id map entry of the sequence to close
 + */
 +static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_map)
 +{
 +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
 +	close(id_map->sock);
 +	id_map->sock = -1;
 +	id_map->seq = -1;
 +}
 +
 +/**
 + * icmp_ping_new() - Prepare a new ping socket for a new id
 + * @c:		Execution context
 + * @id_map:	id map entry of the sequence to open
 + * @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
 + */
 +static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *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";
 +	union icmp_epoll_ref iref = { .id = id };
 +	const void *bind_addr;
 +	const char *bind_if;
 +	int s;
 +
 +	if (af == AF_INET) {
 +		bind_addr = &c->ip4.addr_out;
 +		bind_if = c->ip4.ifname_out;
 +	} else {
 +		bind_addr = &c->ip6.addr_out;
 +		bind_if = c->ip6.ifname_out;
 +	}
 +
 +	s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
 +
 +	if (s < 0) {
 +		warn("Cannot open \"ping\" socket. You might need to:");
 +		warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
 +		warn("...echo requests/replies will fail.");
 +		goto cancel;
 +	}
 +
 +	if (s > FD_REF_MAX)
 +		goto cancel;
 +
 +	id_map->sock = s;
 +
 +	debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
 +
 +	return s;
 +
 +cancel:
 +	if (s >= 0)
 +		close(s);
 +	return -1;
 +}
 +
  /**
   * icmp_tap_handler() - Handle packets from tap
   * @c:		Execution context
 @@ -148,7 +212,6 @@ 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)
  {
 -	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
  	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
  	union {
  		struct sockaddr sa;
 @@ -200,37 +263,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
  		ASSERT(0);
  	}
  
 -	if ((s = id_map->sock) < 0) {
 -		union icmp_epoll_ref iref = { .id = id };
 -		const void *bind_addr;
 -		const char *bind_if;
 -
 -		if (af == AF_INET) {
 -			bind_addr = &c->ip4.addr_out;
 -			bind_if = c->ip4.ifname_out;
 -		} else {
 -			bind_addr = &c->ip6.addr_out;
 -			bind_if = c->ip6.ifname_out;
 -		}
 -
 -		s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
 -
 -		if (s < 0) {
 -			warn("Cannot open \"ping\" socket. You might need to:");
 -			warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
 -			warn("...echo requests/replies will fail.");
 -			return 1;
 -		}
 -
 -		if (s > FD_REF_MAX) {
 -			close(s);
 +	if ((s = id_map->sock) < 0)
 +		if ((s = icmp_ping_new(c, id_map, af, id)) < 0)
  			return 1;
 -		}
 -
 -		id_map->sock = s;
 -
 -		debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
 -	}
  
  	id_map->ts = now->tv_sec;
  
 @@ -257,10 +292,7 @@ static void icmp_timer_one(const struct ctx *c, struct icmp_id_sock
*id_map,
  	if (id_map->sock < 0 || now->tv_sec - id_map->ts <= ICMP_ECHO_TIMEOUT)
  		return;
  
 -	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
 -	close(id_map->sock);
 -	id_map->sock = -1;
 -	id_map->seq = -1;
 +	icmp_ping_close(c, id_map);
  }
  
  /** 
  
-- 
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