Re: [PATCH 4/4] icmp: Don't discard first reply sequence for a given echo ID

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

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

On Wed, Oct 26, 2022 at 06:25:31PM +0200, Stefano Brivio wrote:
> In pasta mode, ICMP and ICMPv6 echo sockets relay back to us any
> reply we send: we're on the same host as the target, after all. We
> discard them by comparing the last sequence we sent with the sequence
> we receive.
> 
> However, on the first reply for a given identifier, the sequence
> might be zero, depending on the implementation of ping(8): we need
> another value to indicate we haven't sent any sequence number, yet.
> 
> Use -1 as initialiser in the echo identifier map.
> 
> This is visible with Busybox's ping, and was reported by Paul on the
> integration at https://github.com/containers/podman/pull/16141, with:
> 
>   $ podman run --net=pasta alpine ping -c 2 192.168.188.1
> 
> ...where only the second reply would be routed back.
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Fixes: 33482d5bf293 ("passt: Add PASTA mode, major rework")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  icmp.c  | 16 ++++++++++++++--
>  icmp.h  |  1 +
>  passt.c |  3 +++
>  3 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 9caa7e6..4ee847f 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -44,12 +44,12 @@
>  /**
>   * 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
> + * @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;
> -	uint16_t seq;
> +	int seq;
>  	time_t ts;
>  };
>  
> @@ -273,6 +273,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->seq = -1;
>  }
>  
>  /**
> @@ -301,3 +302,14 @@ v6:
>  		goto v6;
>  	}
>  }
> +
> +/**
> + * icmp_init() - Initialise sequences in ID map to -1 (no sequence sent yet)
> + */
> +void icmp_init(void)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < ICMP_NUM_IDS; i++)
> +		icmp_id_map[V4][i].seq = icmp_id_map[V6][i].seq = -1;
> +}
> diff --git a/icmp.h b/icmp.h
> index 458ce31..275486d 100644
> --- a/icmp.h
> +++ b/icmp.h
> @@ -15,6 +15,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref,
>  int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
>  		     const struct pool *p, const struct timespec *now);
>  void icmp_timer(const struct ctx *c, const struct timespec *ts);
> +void icmp_init(void);
>  
>  /**
>   * union icmp_epoll_ref - epoll reference portion for ICMP tracking
> diff --git a/passt.c b/passt.c
> index ff4ee5d..34cd832 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -256,6 +256,9 @@ 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, &c.ip4.addr);
>  
>  	if (c.ifi4 && !c.no_dhcp)

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