From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=MHPRn13l; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTPS id A70CE5A0619 for ; Fri, 10 Oct 2025 10:40:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1760085607; 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=t+pgRAstajQTNj/k9NGK/fnmZYN/MTnCl+/jdgiAMKw=; b=MHPRn13lm0BlPvGRPYd/xY1pq4LbcTsVovDylKSpT6c94wNNuvGMSAxkjYkkbPZDzUryt+ RIBHieIM8lJNTWbyX7yoBthgjL/VM6rmITS7kd2bVLZ/yMW/QJVFn3kVrNK/70y7DOsNS3 HxOM3/Yn783g4ug4IxY5wkzH8tOkpjk= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-572-jipvkbonNrmIF1Xpc7_GTw-1; Fri, 10 Oct 2025 04:40:06 -0400 X-MC-Unique: jipvkbonNrmIF1Xpc7_GTw-1 X-Mimecast-MFC-AGG-ID: jipvkbonNrmIF1Xpc7_GTw_1760085605 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-46e47d14dceso8668275e9.2 for ; Fri, 10 Oct 2025 01:40:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1760085604; x=1760690404; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=t+pgRAstajQTNj/k9NGK/fnmZYN/MTnCl+/jdgiAMKw=; b=dVSc3qzx/x60sI/z++oRtmJpu96FYmXhtFifGOjQhovDRUoVo0lJ1saRpy2cFN1uTJ jb7BVZaEtMP+DiS26pfnPmoMf7BAwrDSVD2FiicVhNrrYx/MAYVdcxorzdVdVR2//j11 HsNEW111rW8JNzi32uPk/Iv7UL2GQluiY2ZzDA6of1glAW42mI8fQhlgzOqdLLZPHrvE dH7dmJgoMEP+CHBZesnhJrjiP83ccI0ufOhpxBm9a8tKntAu3Pri8b2iiDkzXN9gtusw cyMPPAnuEbdMJv/6xdc93cF6yHKlcSNS14QYBQVKR75+HpjGBa0LGjNBSbNC9n5XhqE5 vKTA== X-Gm-Message-State: AOJu0YyTUcLuuqzeY40LfnqzUDt78JfN4Qu4rdvEpcVLghOKu4uU5l7T CoJllhqhNcn8FS40DwfSbOxwTaMAgA1322W7F9lKzuHQPMrd5gj3GDw96ycIipvyJKIuqQXxE8L Qc6rdJXB1OdIL/Hw/A5yqOO7hHo3dnj/5oGHX/zxTSlyR06wesxIdTiwevX+Itg== X-Gm-Gg: ASbGncuJ67d8mlwQgZ731dr1t80JC+AFkk0nNBiTn0meS0wEZV9rTrIZ39PDFMJ58YP eKg59661bwSVbhCpnQAOZeAYEyOc05lKPKhb+27FHW5lA/oMNYt1yBcy6MmGp7ukzMb2p9TPlOf jNAzyh+1s1zMcyzeKNzQe9qPiJwe0WPdLEYLTmo+6SJ7n1bEvrU3V0+MAUhOEoU9ca3rMjhtPXw qPraRY6OSSGMb0JnR/Rh25vq4uMLv82TSGuVLU48rnRh8oTNiwL6CwBttKimuzhxzPA01WlaHns PxFD7LzFeq+37lMeBAcJXTh+BH1k0xx3Ue4lu3k7RiUBoEtwe1ToRMDFcWrDYm1v/fIT X-Received: by 2002:a05:600c:609b:b0:456:13b6:4b18 with SMTP id 5b1f17b1804b1-46fa9b106cbmr81901425e9.31.1760085604039; Fri, 10 Oct 2025 01:40:04 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGjhdrDQh4Cb0tz44X+3+k2lHWPUB8cVd77dX/4ZhfeGc+OIjm/Eslc3kaDxJZVAKP4O9S03Q== X-Received: by 2002:a05:600c:609b:b0:456:13b6:4b18 with SMTP id 5b1f17b1804b1-46fa9b106cbmr81900915e9.31.1760085603260; Fri, 10 Oct 2025 01:40:03 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-426ce5e7e44sm3045561f8f.46.2025.10.10.01.40.02 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 10 Oct 2025 01:40:02 -0700 (PDT) Date: Fri, 10 Oct 2025 10:40:00 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH v2 10/14] tap: Split tap_ip6_send() into UDP and ICMP variants Message-ID: <20251010104000.2bcbc351@elisabeth> In-Reply-To: <20221019004357.1454325-11-david@gibson.dropbear.id.au> References: <20221019004357.1454325-1-david@gibson.dropbear.id.au> <20221019004357.1454325-11-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: VjzI5PNJtaNJl2aAIMaqLlPc6t8IKka4qeNXPcbVP1s_1760085605 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: 56TWIGE5BJMJBBFBTV7I7MRDI4KYXDEI X-Message-ID-Hash: 56TWIGE5BJMJBBFBTV7I7MRDI4KYXDEI 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 Wed, 19 Oct 2022 11:43:53 +1100 David Gibson wrote: > tap_ip6_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_udp6_send() and tap_icmp6_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 IPv6 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 > --- > dhcpv6.c | 21 +++------------ > icmp.c | 3 ++- > tap.c | 82 ++++++++++++++++++++++++++++++++++++++++++-------------- > tap.h | 9 +++++-- > 4 files changed, 75 insertions(+), 40 deletions(-) > > diff --git a/dhcpv6.c b/dhcpv6.c > index 7829968..e763aed 100644 > --- a/dhcpv6.c > +++ b/dhcpv6.c > @@ -208,15 +208,8 @@ struct msg_hdr { > uint32_t xid:24; > } __attribute__((__packed__)); > > -#if __BYTE_ORDER == __BIG_ENDIAN > -#define UH_RESP {{{ 547, 546, 0, 0, }}} > -#else > -#define UH_RESP {{{ __bswap_constant_16(547), __bswap_constant_16(546), 0, 0 }}} > -#endif > - > /** > * struct resp_t - Normal advertise and reply message > - * @uh: UDP header > * @hdr: DHCP message header > * @server_id: Server Identifier option > * @ia_na: Non-temporary Address option > @@ -226,7 +219,6 @@ struct msg_hdr { > * @dns_search: Domain Search List, here just for storage size > */ > static struct resp_t { > - struct udphdr uh; > struct msg_hdr hdr; > > struct opt_server_id server_id; > @@ -236,7 +228,6 @@ static struct resp_t { > struct opt_dns_servers dns_servers; > struct opt_dns_search dns_search; > } __attribute__((__packed__)) resp = { > - UH_RESP, > { 0 }, > SERVER_ID, > > @@ -270,13 +261,11 @@ static const struct opt_status_code sc_not_on_link = { > > /** > * struct resp_not_on_link_t - NotOnLink error (mandated by RFC 8415, 18.3.2.) > - * @uh: UDP header > * @hdr: DHCP message header > * @server_id: Server Identifier option > * @var: Payload: IA_NA from client, status code, client ID > */ > static struct resp_not_on_link_t { > - struct udphdr uh; > struct msg_hdr hdr; > > struct opt_server_id server_id; > @@ -284,7 +273,6 @@ static struct resp_not_on_link_t { > uint8_t var[sizeof(struct opt_ia_na) + sizeof(struct opt_status_code) + > sizeof(struct opt_client_id)]; > } __attribute__((__packed__)) resp_not_on_link = { > - UH_RESP, > { TYPE_REPLY, 0 }, > SERVER_ID, > { 0, }, > @@ -527,12 +515,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, > n += sizeof(struct opt_hdr) + ntohs(client_id->l); > > n = offsetof(struct resp_not_on_link_t, var) + n; > - resp_not_on_link.uh.len = htons(n); > > resp_not_on_link.hdr.xid = mh->xid; > > - tap_ip6_send(c, src, IPPROTO_UDP, > - (char *)&resp_not_on_link, n, mh->xid); > + tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546, > + mh->xid, &resp_not_on_link, n); > > return 1; > } > @@ -576,11 +563,11 @@ int dhcpv6(struct ctx *c, const struct pool *p, > n = offsetof(struct resp_t, client_id) + > sizeof(struct opt_hdr) + ntohs(client_id->l); > n = dhcpv6_dns_fill(c, (char *)&resp, n); > - resp.uh.len = htons(n); > > resp.hdr.xid = mh->xid; > > - tap_ip6_send(c, src, IPPROTO_UDP, (char *)&resp, n, mh->xid); > + tap_udp6_send(c, src, 547, tap_ip6_daddr(c, src), 546, > + mh->xid, &resp, n); > c->ip6.addr_seen = c->ip6.addr; > > return 1; > diff --git a/icmp.c b/icmp.c > index 61c2d90..6493ea9 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -105,7 +105,8 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, > icmp_id_map[V6][id].seq = seq; > } > > - tap_ip6_send(c, &sr6->sin6_addr, IPPROTO_ICMPV6, buf, n, 0); > + tap_icmp6_send(c, &sr6->sin6_addr, > + tap_ip6_daddr(c, &sr6->sin6_addr), buf, n); > } else { > struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr; > struct icmphdr *ih = (struct icmphdr *)buf; > diff --git a/tap.c b/tap.c > index 0e8c99b..135d799 100644 > --- a/tap.c > +++ b/tap.c > @@ -175,21 +175,22 @@ void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t proto, > } > > /** > - * tap_ip6_send() - Send IPv6 packet, with L2 headers, calculating L3/L4 checksums > + * tap_push_ip6h() - Build IPv6 header for inbound packet > * @c: Execution context > * @src: IPv6 source address > - * @proto: L4 protocol number > - * @in: Payload > + * @dst: IPv6 destination address > * @len: L4 payload length > - * @flow: Flow label > + * @proto: L4 protocol number > + * @flow: IPv6 flow identifier > + * > + * Return: pointer at which to write the packet's payload > */ > -void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, > - uint8_t proto, const char *in, size_t len, uint32_t flow) > +static void *tap_push_ip6h(char *buf, Sorry, I missed this during review, but this function doesn't take @c anymore, and the comment change didn't cover all the argument changes. It takes a buffer pointer which is not entirely obvious. You changed this later in 1ebe787fe460 ("tap: Simplify some casts in the tap "slow path" functions"), but that commit doesn't update the comment either. Same for tap_push_ip4h(). I just realised as I accidentally passed an "input" header to it. > + const struct in6_addr *src, > + const struct in6_addr *dst, > + size_t len, uint8_t proto, uint32_t flow) > { > - char buf[USHRT_MAX]; > - struct ipv6hdr *ip6h = > - (struct ipv6hdr *)tap_push_l2h(c, buf, ETH_P_IPV6); > - char *data = (char *)(ip6h + 1); > + struct ipv6hdr *ip6h = (struct ipv6hdr *)buf; > > ip6h->payload_len = htons(len); > ip6h->priority = 0; > @@ -197,24 +198,65 @@ void tap_ip6_send(const struct ctx *c, const struct in6_addr *src, > ip6h->nexthdr = proto; > ip6h->hop_limit = 255; > ip6h->saddr = *src; > - ip6h->daddr = *tap_ip6_daddr(c, src); > + ip6h->daddr = *dst; > ip6h->flow_lbl[0] = (flow >> 16) & 0xf; > ip6h->flow_lbl[1] = (flow >> 8) & 0xff; > ip6h->flow_lbl[2] = (flow >> 0) & 0xff; > + return ip6h + 1; > +} > > +/** > + * tap_udp6_send() - Send UDP over IPv6 packet > + * @c: Execution context > + * @src: IPv6 source address > + * @sport: UDP source port > + * @dst: IPv6 destination address > + * @dport: UDP destination port > + * @flow: Flow label > + * @in: UDP payload contents (not including UDP header) > + * @len: UDP payload length (not including UDP header) > + */ > +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, > + uint32_t flow, const void *in, size_t len) > +{ > + size_t udplen = len + sizeof(struct udphdr); > + char buf[USHRT_MAX]; > + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); > + void *uhp = tap_push_ip6h(ip6h, src, dst, udplen, IPPROTO_UDP, flow); > + struct udphdr *uh = (struct udphdr *)uhp; > + char *data = (char *)(uh + 1); > + > + uh->source = htons(sport); > + uh->dest = htons(dport); > + uh->len = htons(udplen); > + csum_udp6(uh, src, dst, in, len); > memcpy(data, in, len); > > - if (proto == IPPROTO_UDP) { > - struct udphdr *uh = (struct udphdr *)(ip6h + 1); > + if (tap_send(c, buf, len + (data - buf)) < 1) > + debug("tap: failed to send %lu bytes (IPv6)", len); > +} > > - csum_udp6(uh, &ip6h->saddr, &ip6h->daddr, > - uh + 1, len - sizeof(*uh)); > - } else if (proto == IPPROTO_ICMPV6) { > - struct icmp6hdr *ih = (struct icmp6hdr *)(ip6h + 1); > +/** > + * tap_icmp6_send() - Send ICMPv6 packet > + * @c: Execution context > + * @src: IPv6 source address > + * @dst: IPv6 destination address > + * @in: ICMP packet, including ICMP header > + * @len: ICMP packet length, including ICMP header > + */ > +void tap_icmp6_send(const struct ctx *c, > + const struct in6_addr *src, const struct in6_addr *dst, > + void *in, size_t len) > +{ > + char buf[USHRT_MAX]; > + void *ip6h = tap_push_l2h(c, buf, ETH_P_IPV6); > + char *data = tap_push_ip6h(ip6h, src, dst, len, IPPROTO_ICMPV6, 0); > + struct icmp6hdr *icmp6h = (struct icmp6hdr *)data; > > - csum_icmp6(ih, &ip6h->saddr, &ip6h->daddr, > - ih + 1, len - sizeof(*ih)); > - } > + memcpy(data, in, len); > + csum_icmp6(icmp6h, src, dst, icmp6h + 1, len - sizeof(*icmp6h)); > > if (tap_send(c, buf, len + (data - buf)) < 1) > debug("tap: failed to send %lu bytes (IPv6)", len); > diff --git a/tap.h b/tap.h > index 011ba8e..d43c7a0 100644 > --- a/tap.h > +++ b/tap.h > @@ -11,8 +11,13 @@ 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_ip6_send(const struct ctx *c, const struct in6_addr *src, > - uint8_t proto, const char *in, size_t len, uint32_t flow); > +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, > + uint32_t flow, const void *in, size_t len); > +void tap_icmp6_send(const struct ctx *c, > + const struct in6_addr *src, const struct in6_addr *dst, > + void *in, size_t len); > int tap_send(const struct ctx *c, const void *data, size_t len); > void tap_handler(struct ctx *c, int fd, uint32_t events, > const struct timespec *now); -- Stefano