Re: [PATCH v2 09/12] icmp: Consolidate icmp_sock_handler() with icmpv6_sock_handler()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 21 Dec 2023 17:53:24 +1100
David Gibson <david@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@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