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.129.124]) by passt.top (Postfix) with ESMTP id 2D86D5A026E for ; Tue, 18 Oct 2022 10:40:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1666082411; 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=FWhoa8HEzbb6P7VpzUArq2b2MqAniGUuy3f6igEnkyQ=; b=VZ2ciuKgNzh/7h9VA2F7sR16emBKGgW4iWjGVBIVi/6ic2rXv4yy8A6bgt35+Ez+3rWKoP 2VB7cCZli8O66BLldH6+g3ZXEljm4vXh8iiUZS58Sl7n5IbYHzpj5MSsa0NIXqpcFFKkXg amb4m6M4eYAfoEi8n5s0RlQ8mWwErRQ= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-417-BOR2ug41Nqy2bt6_UrPSYw-1; Tue, 18 Oct 2022 04:40:09 -0400 X-MC-Unique: BOR2ug41Nqy2bt6_UrPSYw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 85B6D2999B4D; Tue, 18 Oct 2022 08:40:09 +0000 (UTC) Received: from maya.cloud.tilaa.com (ovpn-208-31.brq.redhat.com [10.40.208.31]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 16060200A7C5; Tue, 18 Oct 2022 08:40:09 +0000 (UTC) Date: Tue, 18 Oct 2022 05:06:34 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 13/14] tap: Split tap_ip4_send() into UDP and ICMP variants Message-ID: <20221018050634.428cf8d6@elisabeth> In-Reply-To: <20221017085807.473470-14-david@gibson.dropbear.id.au> References: <20221017085807.473470-1-david@gibson.dropbear.id.au> <20221017085807.473470-14-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.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: MDF52CVTF3NPYMW7EMRZBWCTUUR23AUU X-Message-ID-Hash: MDF52CVTF3NPYMW7EMRZBWCTUUR23AUU 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.3 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 Mon, 17 Oct 2022 19:58:06 +1100 David Gibson wrote: > tap_ip4_send() has special case logic to compute the checksums for UDP > and ICMP packets, which is a mild layering violation. By using a suitable > helper we can split it into tap_udp4_send() and tap_icmp4_send() functions > without greatly increasing the code size, this removing that layering > violation. > > We make some small changes to the interface while there. In both cases > we make the destination IPv4 address a parameter, which will be useful > later. For the UDP variant we make it take just the UDP payload, and it > will generate the UDP header. For the ICMP variant we pass in the ICMP > header as before. The inconsistency is because that's what seems to be > the more natural way to invoke the function in the callers in each case. > > Signed-off-by: David Gibson > --- > icmp.c | 3 ++- > tap.c | 75 +++++++++++++++++++++++++++++++++++++++++----------------- > tap.h | 7 ++++-- > 3 files changed, 60 insertions(+), 25 deletions(-) > > diff --git a/icmp.c b/icmp.c > index 6493ea9..233acf9 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -124,7 +124,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, > icmp_id_map[V4][id].seq = seq; > } > > - tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n); > + tap_icmp4_send(c, sr4->sin_addr.s_addr, tap_ip4_daddr(c), > + buf, n); > } > } > > diff --git a/tap.c b/tap.c > index 274f4ba..5792880 100644 > --- a/tap.c > +++ b/tap.c > @@ -127,20 +127,10 @@ static void *tap_l2_hdr(const struct ctx *c, void *buf, uint16_t proto) > return eh + 1; > } > > -/** > - * tap_ip4_send() - Send IPv4 packet, with L2 headers, calculating L3/L4 checksums > - * @c: Execution context > - * @src: IPv4 source address > - * @proto: L4 protocol number > - * @in: Payload > - * @len: L4 payload length > - */ > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > - const char *in, size_t len) I understand why you return ip(4)h + 1 here because I've just reviewed 9/14, I wouldn't know otherwise: /** * tap_ip4_hdr() - Build IPv4 header for inbound packet, with checksum * @c: Execution context * @src: IPv4 source address, network order * @dst: IPv4 destination address, network order * @len: L4 payload length * @proto: L4 protocol number * * Return: pointer to write payload to */ > +static void *tap_ip4_hdr(char *buf, in_addr_t src, in_addr_t dst, > + size_t len, uint8_t proto) > { > - char buf[USHRT_MAX]; > - struct iphdr *ip4h = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP); > - char *data = (char *)(ip4h + 1); > + struct iphdr *ip4h = (struct iphdr *)buf; > > ip4h->version = 4; > ip4h->ihl = sizeof(struct iphdr) / 4; > @@ -151,20 +141,61 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > ip4h->ttl = 255; > ip4h->protocol = proto; > ip4h->saddr = src; > - ip4h->daddr = tap_ip4_daddr(c); > + ip4h->daddr = dst; > csum_ip4_header(ip4h); > + return ip4h + 1; > +} > + > +/** > + * tap_udp4_send() - Send UDP over IPv4 packet > + * @c: Execution context > + * @src: IPv4 source address > + * @sport: UDP source port > + * @dst: IPv4 destination address > + * @dport: UDP destination port > + * @in: UDP payload contents (not including UDP header) > + * @len: UDP payload length (not including UDP header) > + */ > +/* cppcheck-suppress unusedFunction */ > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t sport, > + in_addr_t dst, in_port_t dport, > + const void *in, size_t len) > +{ > + size_t udplen = len + sizeof(struct udphdr); > + char buf[USHRT_MAX]; > + void *ip4h = tap_l2_hdr(c, buf, ETH_P_IP); > + void *uhp = tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP); Two observations: - this saves one line and one cast, but it's really a bit unnatural that tap_ip4_hdr() doesn't point to the header it just made, or to nothing. I would rather have to +1 the return value or the original pointer instead or having this trick > + struct udphdr *uh = (struct udphdr *)uhp; > + char *data = (char *)(uh + 1); - it's longer, but in my opinion clearer, if we split a bit more clearly the components of the packet, that is, something like (untested): char buf[USHRT_MAX]; struct udphdr *uh; struct iphdr *iph; char *data; iph = (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_IP) + 1; tap_ip_hdr(iph, src, dst, len + sizeof(uh), IPPROTO_UDP); uh = (struct udphdr *)iph + 1; uh->source = htons(sport); uh->dest = htons(dport); uh->len = htons(len + sizeof(uh)); csum_udp4(uh, src, dst, in, len); data = uh + 1; memcpy(data, in, len); if (tap_send(c, buf, len + (data - buf)) < 0) debug("tap: failed to send %lu bytes (IPv4)", len); > > + uh->source = htons(sport); > + uh->dest = htons(dport); > + uh->len = htons(udplen); > + csum_udp4(uh, src, dst, in, len); > memcpy(data, in, len); > > - if (ip4h->protocol == IPPROTO_UDP) { > - struct udphdr *uh = (struct udphdr *)(ip4h + 1); > + if (tap_send(c, buf, len + (data - buf)) < 0) > + debug("tap: failed to send %lu bytes (IPv4)", len); > +} > > - csum_udp4(uh, ip4h->saddr, ip4h->daddr, > - uh + 1, len - sizeof(*uh)); > - } else if (ip4h->protocol == IPPROTO_ICMP) { > - struct icmphdr *ih = (struct icmphdr *)(ip4h + 1); > - csum_icmp4(ih, ih + 1, len - sizeof(*ih)); > - } > +/** > + * tap_icmp4_send() - Send ICMPv4 packet > + * @c: Execution context > + * @src: IPv4 source address > + * @dst: IPv4 destination address > + * @in: ICMP packet, including ICMP header > + * @len: ICMP packet length, including ICMP header > + */ > +void tap_icmp4_send(const struct ctx *c, in_addr_t src, in_addr_t dst, > + void *in, size_t len) > +{ > + char buf[USHRT_MAX]; > + void *ip4h = tap_l2_hdr(c, buf, ETH_P_IP); > + char *data = tap_ip4_hdr(ip4h, src, dst, len, IPPROTO_ICMP); > + struct icmphdr *icmp4h = (struct icmphdr *)data; ...same here, even though perhaps not so apparent. > + > + memcpy(data, in, len); > + csum_icmp4(icmp4h, icmp4h + 1, len - sizeof(*icmp4h)); > > if (tap_send(c, buf, len + (data - buf)) < 0) > debug("tap: failed to send %lu bytes (IPv4)", len); > diff --git a/tap.h b/tap.h > index d43c7a0..743bc58 100644 > --- a/tap.h > +++ b/tap.h > @@ -7,10 +7,13 @@ > #define TAP_H > > in_addr_t tap_ip4_daddr(const struct ctx *c); > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t sport, > + in_addr_t dst, in_port_t dport, > + const void *in, size_t len); > +void tap_icmp4_send(const struct ctx *c, in_addr_t src, in_addr_t dst, > + void *in, size_t len); > const struct in6_addr *tap_ip6_daddr(const struct ctx *c, > const struct in6_addr *src); > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > - const char *in, size_t len); > void tap_udp6_send(const struct ctx *c, > const struct in6_addr *src, in_port_t sport, > const struct in6_addr *dst, in_port_t dport, -- Stefano