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.133.124]) by passt.top (Postfix) with ESMTP id 2D9355A026D for ; Sat, 6 Jan 2024 17:00:51 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704556850; 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=w56JNN9g0e/SpuFOjHkt2QmT++itF1ERFs8vfE+u4ng=; b=JPD8F3Wv7g1FJiyfk5PX3deq2Cdnm4J4+Azzz3vZWykcfegB3nn0HlmHhxqTwZ7H0FpTTn TtLFKGHVRWJe8so4/yNH8CzJS0RTqoUaW8ZK7lSNPtgqqe6xPIquwXHm/We5aeR1hw3Vb8 UIopT34mhvit/n8YmnV6Nt+DdqkO718= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-594-cICDwa5iOd2mcuLcpWzMMA-1; Sat, 06 Jan 2024 11:00:48 -0500 X-MC-Unique: cICDwa5iOd2mcuLcpWzMMA-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 48C09185A781; Sat, 6 Jan 2024 16:00:48 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 619A72166B33; Sat, 6 Jan 2024 16:00:47 +0000 (UTC) Date: Sat, 6 Jan 2024 17:00:44 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 09/12] icmp: Consolidate icmp_sock_handler() with icmpv6_sock_handler() Message-ID: <20240106170044.0aa57f62@elisabeth> In-Reply-To: <20231221065327.1307827-10-david@gibson.dropbear.id.au> References: <20231221065327.1307827-1-david@gibson.dropbear.id.au> <20231221065327.1307827-10-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: IZW3YQOCKWYNVJNSCXPQBD73UNFAZ5S4 X-Message-ID-Hash: IZW3YQOCKWYNVJNSCXPQBD73UNFAZ5S4 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, 21 Dec 2023 17:53:24 +1100 David Gibson 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 > --- > 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