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 50E755A026D for ; Sat, 6 Jan 2024 17:00:36 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1704556835; 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=seAF6atIk+E6dLz0yiFsrus5h+nWBy1o3+80pj30Hf8=; b=QHpQQZ6mqI+ezqL2e+gX9oN7JIxF+bKPJuz3znh+fR8ZZgFVhdT4UtXy4sD+xBs2JdALgS IeyWVC3iRzUmXHIoij9vP+cdKWEeRuZpAILngA9T8gz38b+OW2qQqzDjjHq0xOn0PBO/sU 1YWoF1+14StDiqLnD9xw4e1lyBGcCVE= 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-44-b2QYBiVFOP2x_aDMwo-uDw-1; Sat, 06 Jan 2024 11:00:33 -0500 X-MC-Unique: b2QYBiVFOP2x_aDMwo-uDw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 3AEA5101A551; Sat, 6 Jan 2024 16:00:32 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.16]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6A1132026D66; Sat, 6 Jan 2024 16:00:31 +0000 (UTC) Date: Sat, 6 Jan 2024 17:00:21 +0100 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 08/12] icmp: Share more between IPv4 and IPv6 paths in icmp_tap_handler() Message-ID: <20240106170021.1bd09965@elisabeth> In-Reply-To: <20231221065327.1307827-9-david@gibson.dropbear.id.au> References: <20231221065327.1307827-1-david@gibson.dropbear.id.au> <20231221065327.1307827-9-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: BXWECESHARKHERXRBCIHCMWV2X5DWB3Z X-Message-ID-Hash: BXWECESHARKHERXRBCIHCMWV2X5DWB3Z 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:23 +1100 David Gibson 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 > --- > 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. > > if (ih->type != ICMP_ECHO) > return 1; > > - iref.id = id = ntohs(ih->un.echo.id); > + id = ntohs(ih->un.echo.id); > + id_map = &icmp_id_map[V4][id]; > + seq = ntohs(ih->un.echo.sequence); > + sa.sa4.sin_addr = *(struct in_addr *)daddr; > + } else if (af == AF_INET6) { > + struct icmp6hdr *ih = (struct icmp6hdr *)pkt; > > - 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) > - goto fail_sock; > - if (s > FD_REF_MAX) { > - close(s); > - return 1; > - } > + if (plen < sizeof(*ih)) > + return 1; > > - icmp_id_map[V4][id].sock = s; > + if (ih->icmp6_type != ICMPV6_ECHO_REQUEST) > + return 1; > > - debug("ICMP: new socket %i for echo ID %i", s, id); > - } > - icmp_id_map[V4][id].ts = now->tv_sec; > + id = ntohs(ih->icmp6_identifier); > + id_map = &icmp_id_map[V6][id]; > + seq = ntohs(ih->icmp6_sequence); > + sa.sa6.sin6_addr = *(struct in6_addr *)daddr; > + sa.sa6.sin6_scope_id = c->ifi6; > + } else { > + ASSERT(0); > + } > + > + if ((s = id_map->sock) < 0) { > + union icmp_epoll_ref iref = { .id = id }; > + const void *bind_addr; > + const char *bind_if; > > - sa.sin_addr = *(struct in_addr *)daddr; > - if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL, > - (struct sockaddr *)&sa, sizeof(sa)) < 0) { > - debug("ICMP: failed to relay request to socket"); > + if (af == AF_INET) { > + bind_addr = &c->ip4.addr_out; > + bind_if = c->ip4.ifname_out; > } else { > - debug("ICMP: echo request to socket, ID: %i, seq: %i", > - id, ntohs(ih->un.echo.sequence)); > + bind_addr = &c->ip6.addr_out; > + bind_if = c->ip6.ifname_out; > } > - } else if (af == AF_INET6) { > - struct sockaddr_in6 sa = { > - .sin6_family = AF_INET6, > - .sin6_scope_id = c->ifi6, > - }; > - union icmp_epoll_ref iref; > - struct icmp6hdr *ih; > - int id, s; > - > - ih = packet_get(p, 0, 0, sizeof(struct icmp6hdr), &plen); > - if (!ih) > - return 1; > > - if (ih->icmp6_type != ICMPV6_ECHO_REQUEST) > - return 1; > + s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32); > > - iref.id = id = ntohs(ih->icmp6_identifier); > - 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); > - if (s < 0) > - goto fail_sock; > - if (s > FD_REF_MAX) { > - close(s); > - return 1; > - } > - > - icmp_id_map[V6][id].sock = s; > - > - debug("ICMPv6: new socket %i for echo ID %i", s, id); > + 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; > } > - icmp_id_map[V6][id].ts = now->tv_sec; > > - sa.sin6_addr = *(struct in6_addr *)daddr; > - if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL, > - (struct sockaddr *)&sa, sizeof(sa)) < 1) { > - debug("ICMPv6: failed to relay request to socket"); > - } else { > - debug("ICMPv6: echo request to socket, ID: %i, seq: %i", > - id, ntohs(ih->icmp6_sequence)); > + if (s > FD_REF_MAX) { > + close(s); > + return 1; > } > + > + id_map->sock = s; > + > + debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id); > } > > - return 1; > + id_map->ts = now->tv_sec; > + > + if (sendto(s, pkt, plen, MSG_NOSIGNAL, &sa.sa, sl) < 0) { > + debug("%s: failed to relay request to socket: %s", > + pname, strerror(errno)); > + } else { > + debug("%s: echo request to socket, ID: %"PRIu16", seq: %"PRIu16, > + pname, id, seq); > + } > > -fail_sock: > - 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; > } > -- Stefano