From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id E408E5A027D for ; Fri, 1 Mar 2024 00:42:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1709250137; bh=6W0GduBP+lWWYeroWLAr2vuztpLjnvooiIgeaD6pqHw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=B6KGfKRTToS7qbAm/4L3xjX9AhYfFXvkHtLhqe/0ZmaRYNhRpRLiRoII3QjwiUG9K 9APBXqjLGBi5G3DGfWaKUbo9MXUAw5lsMHcTaDeVQx4IdNq0scTObull64hUS9g3De oaPFrxAT8gd93FoFk7je4EGzb/ztwSvyoPyqylExWgY9IilpnmLOlmaO6WqziJJlwt su8lTBl5LikIY+E87VGPjxuITg8fVhKRum2OExDgg+tUEIsGaLTCec1bJDpRzTHHl0 6dbuq7CHCaRypW0wIVkQyXa2aOeFHGH6TyJCvURkt7B6+T9tuAZreSgnuQoQrxdq3e cM5uPRqlIhlrw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Tm76x2cYWz4wcq; Fri, 1 Mar 2024 10:42:17 +1100 (AEDT) Date: Fri, 1 Mar 2024 10:42:08 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Message-ID: References: <20240229165955.829476-1-lvivier@redhat.com> <20240229165955.829476-7-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1YTHRds24XL7FQfg" Content-Disposition: inline In-Reply-To: <20240229165955.829476-7-lvivier@redhat.com> Message-ID-Hash: 2D2YVHVVLHVRISTVR7SQWTSNKSLAGZJP X-Message-ID-Hash: 2D2YVHVVLHVRISTVR7SQWTSNKSLAGZJP X-MailFrom: dgibson@gandalf.ozlabs.org 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: --1YTHRds24XL7FQfg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 29, 2024 at 05:59:53PM +0100, Laurent Vivier wrote: > The TCP and UDP checksums are computed using the data in the TCP/UDP > payload but also some informations in the IP header (protocol, > length, source and destination addresses). >=20 > We add two functions, proto_ipv4_header_psum() and > proto_ipv6_header_psum(), to compute the checksum of the IP > header part. >=20 > Signed-off-by: Laurent Vivier > --- >=20 > Notes: > v4: > - fix payload length endianness > =20 > v3: > - function parameters provide tot_len, saddr, daddr and protocol > rather than an iphdr/ipv6hdr > =20 > v2: > - move new function to checksum.c > - use _psum rather than _checksum in the name > - replace csum_udp4() and csum_udp6() by the new function >=20 > checksum.c | 69 ++++++++++++++++++++++++++++++++++++++++++------------ > checksum.h | 4 ++++ > tcp.c | 45 ++++++++++++++++------------------- > udp.c | 13 ++++++---- > 4 files changed, 86 insertions(+), 45 deletions(-) >=20 > diff --git a/checksum.c b/checksum.c > index 511b296a9a80..93c8d5205c2b 100644 > --- a/checksum.c > +++ b/checksum.c > @@ -134,6 +134,30 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8_t p= rotocol, > return ~csum_fold(sum); > } > =20 > +/** > + * proto_ipv4_header_psum() - Calculates the partial checksum of an > + * IPv4 header for UDP or TCP > + * @tot_len: IPv4 Payload length > + * @proto: Protocol number > + * @saddr: Source address > + * @daddr: Destination address > + * @proto: proto Protocol number Needs to note that tot_len is in host order, but saddr and daddr are in network order. Usually, I'd take host order as assumed for a plain integer type, but since it's mixed here, we should annotate them all. Alternatively, we could pass saddr and daddr as struct in_addr. In general I've tried to pass IPv4 addresses with that type, rather than in_addr_t or uint32_t. Looking at the callers, it seems like it's a mixed bag whether that's messier or cleaner in this case. > + * Returns: Partial checksum of the IPv4 header > + */ > +uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol, > + uint32_t saddr, uint32_t daddr) > +{ > + uint32_t psum =3D htons(protocol); > + > + psum +=3D (saddr >> 16) & 0xffff; > + psum +=3D saddr & 0xffff; > + psum +=3D (daddr >> 16) & 0xffff; > + psum +=3D daddr & 0xffff; > + psum +=3D htons(tot_len); > + > + return psum; > +} > + > /** > * csum_udp4() - Calculate and set checksum for a UDP over IPv4 packet > * @udp4hr: UDP header, initialised apart from checksum > @@ -150,14 +174,12 @@ void csum_udp4(struct udphdr *udp4hr, > udp4hr->check =3D 0; > =20 > if (UDP4_REAL_CHECKSUMS) { > - /* UNTESTED: if we did want real UDPv4 checksums, this > - * is roughly what we'd need */ > - uint32_t psum =3D csum_fold(saddr.s_addr) > - + csum_fold(daddr.s_addr) > - + htons(len + sizeof(*udp4hr)) > - + htons(IPPROTO_UDP); > - /* Add in partial checksum for the UDP header alone */ > - psum +=3D sum_16b(udp4hr, sizeof(*udp4hr)); > + uint16_t tot_len =3D len + sizeof(struct udphdr); > + uint32_t psum =3D proto_ipv4_header_psum(tot_len, > + IPPROTO_UDP, > + saddr.s_addr, > + daddr.s_addr); > + psum =3D csum_unfolded(udp4hr, sizeof(struct udphdr), psum); > udp4hr->check =3D csum(payload, len, psum); > } > } > @@ -180,6 +202,26 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void = *payload, size_t len) > icmp4hr->checksum =3D csum(payload, len, psum); > } > =20 > +/** > + * proto_ipv6_header_psum() - Calculates the partial checksum of an > + * IPv6 header for UDP or TCP > + * @payload_len: IPv6 payload length > + * @proto: Protocol number > + * @saddr: Source address > + * @daddr: Destination address > + * Returns: Partial checksum of the IPv6 header > + */ > +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol, > + struct in6_addr saddr, struct in6_addr daddr) I don't see any point to passing the addresses by value here. You take their address, so they must be written back to memory if passed in registers. At the call sites, you still have the dereference so it doesn't help with alignment. > +{ > + uint32_t sum =3D htons(protocol) + htons(payload_len); > + > + sum +=3D sum_16b(&saddr, sizeof(saddr)); > + sum +=3D sum_16b(&daddr, sizeof(daddr)); > + > + return sum; > +} > + > /** > * csum_udp6() - Calculate and set checksum for a UDP over IPv6 packet > * @udp6hr: UDP header, initialised apart from checksum > @@ -190,14 +232,11 @@ void csum_udp6(struct udphdr *udp6hr, > const struct in6_addr *saddr, const struct in6_addr *daddr, > const void *payload, size_t len) > { > - /* Partial checksum for the pseudo-IPv6 header */ > - uint32_t psum =3D sum_16b(saddr, sizeof(*saddr)) + > - sum_16b(daddr, sizeof(*daddr)) + > - htons(len + sizeof(*udp6hr)) + htons(IPPROTO_UDP); > - > + uint32_t psum =3D proto_ipv6_header_psum(len + sizeof(struct udphdr), > + IPPROTO_UDP, *saddr, *daddr); > udp6hr->check =3D 0; > - /* Add in partial checksum for the UDP header alone */ > - psum +=3D sum_16b(udp6hr, sizeof(*udp6hr)); > + > + psum =3D csum_unfolded(udp6hr, sizeof(struct udphdr), psum); > udp6hr->check =3D csum(payload, len, psum); > } > =20 > diff --git a/checksum.h b/checksum.h > index 92db73612b6e..b2b5b8e8b77e 100644 > --- a/checksum.h > +++ b/checksum.h > @@ -15,10 +15,14 @@ uint16_t csum_fold(uint32_t sum); > uint16_t csum_unaligned(const void *buf, size_t len, uint32_t init); > uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol, > uint32_t saddr, uint32_t daddr); > +uint32_t proto_ipv4_header_psum(uint16_t tot_len, uint8_t protocol, > + uint32_t saddr, uint32_t daddr); > void csum_udp4(struct udphdr *udp4hr, > struct in_addr saddr, struct in_addr daddr, > const void *payload, size_t len); > void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len); > +uint32_t proto_ipv6_header_psum(uint16_t payload_len, uint8_t protocol, > + struct in6_addr saddr, struct in6_addr daddr); > void csum_udp6(struct udphdr *udp6hr, > const struct in6_addr *saddr, const struct in6_addr *daddr, > const void *payload, size_t len); > diff --git a/tcp.c b/tcp.c > index ea0802c6b102..d78efa5401bb 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -939,39 +939,30 @@ static void tcp_sock_set_bufsize(const struct ctx *= c, int s) > * tcp_update_check_tcp4() - Update TCP checksum from stored one > * @buf: L2 packet buffer with final IPv4 header Function comment no longer matches the parameters. > */ > -static void tcp_update_check_tcp4(struct tcp4_l2_buf_t *buf) > +static void tcp_update_check_tcp4(struct iphdr *iph) Hmm... so this takes only a pointer to iph, but writes to the TCP header it assumes is beyond that, and reads from the payload it assumes is beyond that. That seems like a dangerous interface to me (not to mention that I fear it could trigger TBAA traps). > { > - uint16_t tlen =3D ntohs(buf->iph.tot_len) - 20; > - uint32_t sum =3D htons(IPPROTO_TCP); > + uint16_t tlen =3D ntohs(iph->tot_len) - sizeof(struct iphdr); > + uint32_t sum =3D proto_ipv4_header_psum(tlen, IPPROTO_TCP, > + iph->saddr, iph->daddr); > + struct tcphdr *th =3D (struct tcphdr *)(iph + 1); > =20 > - sum +=3D (buf->iph.saddr >> 16) & 0xffff; > - sum +=3D buf->iph.saddr & 0xffff; > - sum +=3D (buf->iph.daddr >> 16) & 0xffff; > - sum +=3D buf->iph.daddr & 0xffff; > - sum +=3D htons(ntohs(buf->iph.tot_len) - 20); > - > - buf->th.check =3D 0; > - buf->th.check =3D csum(&buf->th, tlen, sum); > + th->check =3D 0; > + th->check =3D csum(th, tlen, sum); > } > =20 > /** > * tcp_update_check_tcp6() - Calculate TCP checksum for IPv6 > * @buf: L2 packet buffer with final IPv6 header > */ > -static void tcp_update_check_tcp6(struct tcp6_l2_buf_t *buf) > +static void tcp_update_check_tcp6(struct ipv6hdr *ip6h) Same comments as for the IPv4 version. > { > - int len =3D ntohs(buf->ip6h.payload_len) + sizeof(struct ipv6hdr); > - > - buf->ip6h.hop_limit =3D IPPROTO_TCP; > - buf->ip6h.version =3D 0; > - buf->ip6h.nexthdr =3D 0; > + struct tcphdr *th =3D (struct tcphdr *)(ip6h + 1); > + uint16_t payload_len =3D ntohs(ip6h->payload_len); > + uint32_t sum =3D proto_ipv6_header_psum(payload_len, IPPROTO_TCP, > + ip6h->saddr, ip6h->daddr); > =20 > - buf->th.check =3D 0; > - buf->th.check =3D csum(&buf->ip6h, len, 0); > - > - buf->ip6h.hop_limit =3D 255; > - buf->ip6h.version =3D 6; > - buf->ip6h.nexthdr =3D IPPROTO_TCP; > + th->check =3D 0; > + th->check =3D csum(th, payload_len, sum); > } > =20 > /** > @@ -1383,7 +1374,7 @@ do { \ > =20 > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > =20 > - tcp_update_check_tcp4(b); > + tcp_update_check_tcp4(&b->iph); > =20 > tlen =3D tap_iov_len(c, &b->taph, ip_len); > } else { > @@ -1402,7 +1393,11 @@ do { \ > =20 > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > =20 > - tcp_update_check_tcp6(b); > + tcp_update_check_tcp6(&b->ip6h); > + > + b->ip6h.hop_limit =3D 255; > + b->ip6h.version =3D 6; > + b->ip6h.nexthdr =3D IPPROTO_TCP; > =20 > b->ip6h.flow_lbl[0] =3D (conn->sock >> 16) & 0xf; > b->ip6h.flow_lbl[1] =3D (conn->sock >> 8) & 0xff; > diff --git a/udp.c b/udp.c > index d517c99dcc69..410ace16a6a2 100644 > --- a/udp.c > +++ b/udp.c > @@ -625,6 +625,7 @@ static size_t udp_update_hdr6(const struct ctx *c, in= t n, in_port_t dstport, > { > struct udp6_l2_buf_t *b =3D &udp6_l2_buf[n]; > struct in6_addr *src; > + uint16_t payload_len; > in_port_t src_port; > size_t ip_len; > =20 > @@ -633,7 +634,8 @@ static size_t udp_update_hdr6(const struct ctx *c, in= t n, in_port_t dstport, > =20 > ip_len =3D udp6_l2_mh_sock[n].msg_len + sizeof(b->ip6h) + sizeof(b->uh); > =20 > - b->ip6h.payload_len =3D htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh= )); > + payload_len =3D udp6_l2_mh_sock[n].msg_len + sizeof(b->uh); > + b->ip6h.payload_len =3D htons(payload_len); > =20 > if (IN6_IS_ADDR_LINKLOCAL(src)) { > b->ip6h.daddr =3D c->ip6.addr_ll_seen; > @@ -675,10 +677,11 @@ static size_t udp_update_hdr6(const struct ctx *c, = int n, in_port_t dstport, > b->uh.source =3D b->s_in6.sin6_port; > b->uh.dest =3D htons(dstport); > b->uh.len =3D b->ip6h.payload_len; > - > - b->ip6h.hop_limit =3D IPPROTO_UDP; > - b->ip6h.version =3D b->ip6h.nexthdr =3D b->uh.check =3D 0; > - b->uh.check =3D csum(&b->ip6h, ip_len, 0); > + b->uh.check =3D 0; > + b->uh.check =3D csum(&b->uh, payload_len, > + proto_ipv6_header_psum(payload_len, IPPROTO_UDP, > + b->ip6h.saddr, > + b->ip6h.daddr)); > b->ip6h.version =3D 6; > b->ip6h.nexthdr =3D IPPROTO_UDP; > b->ip6h.hop_limit =3D 255; --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --1YTHRds24XL7FQfg Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXhFk8ACgkQzQJF27ox 2Ge3oQ/8CQ2pZFzOnn8mNw7oONDCjzBUZB4nIm7vcyvcffz1yb1Y0bfV5Yhepbui JCgs12/SneteJvGPt3JUC51yn139CNEG9ebZcie3f6ADg272d6KANtNYpXK6W9c7 lgsNGk1mdOWajGRRzp/WjnkpCYOe6JC57Ln4SimHk7flPw3Eh0Y6Nyyi6Or0zFwh Pw4FB6Z05PIVKPYGUl93fukE5FO4fEko9rz8wU/Pp7hFq1FmBW19BXwGJsoKBoL7 H3z5IoBmgQW51JYk05I0QGAsRXCir8KTbvbPGxRuev2hpE7EiZaTh3Z/EtB+2E+l 9Eog+zOxlY945Qy/lN9KG1Tob5D6A1dupwHezPdvRp7rtOXeRsyjso0LMvaM5wMu HvMSU5P+Ejs9No3LIFqen0iJpDGgks2QIR0HpwH35zBe73rc8e1aJsnbNaNZL15z /QrIyuJ8Xa5GuBTOEnyCMULONfA5kCtlPYBaK+8d8diBXNjHI3wirq5etbsdDrtN E6mmXdnI8cZUmnevuhlxSUFaFgdlwj3enOhcj0N5QQRjCbYU8hg+d5GjCSa6v5DL gz/ULP16hEP9r9CcIt3AXqHbJGVpqEYdvLg7+Nf4kvZlMxVSy2obZ/7YqQpiimRC 6yqP8dnzeGWnXTw2CwHS4NO2L0LCSO6HDOOgtjbxGQWAL922BVw= =+yTF -----END PGP SIGNATURE----- --1YTHRds24XL7FQfg--