From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 649AE5A027A for ; Thu, 15 Feb 2024 04:12:59 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1707966776; bh=oTee7wEE0Lzy/s8HTksQ+XzdDqJd1fRMz5Nm9333xvo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ftZ+oiIxf0PobdoPNtRD9tEyhpGmUy0Kduh8pAKBkPj8FuBQUlnD3b6hd1rB3wOds 0EaOU1Q23DoU/BLhIReOHX44PLX2OcPAKQMusqInmEGNgKbqReRoVo2Fk+daN8M9g/ cffqARHGpxHRfo8Df5VykuZIeNLZGRKYJ3/H/6+ZAjkDtVfpCDNz1RILFHxxdKuMu5 ggjTIq8QiwzeReRe/y6mlZhKG+C8rRWAdqcqNmb80GoHxPmu8G8BsQYBfcC/lweu+1 7i+aayDhgbShTw41jp2cTlA3lk0liJ8c9nlEXBUy35dEa5gu5N+pwK3PhmsJQl0gVo dwgic2owyWR4w== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Tb0Vw1xxvz4wxv; Thu, 15 Feb 2024 14:12:56 +1100 (AEDT) Date: Thu, 15 Feb 2024 14:12:51 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 7/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Message-ID: References: <20240214085628.210783-1-lvivier@redhat.com> <20240214085628.210783-8-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="AtLlefsjnNnfGb9T" Content-Disposition: inline In-Reply-To: <20240214085628.210783-8-lvivier@redhat.com> Message-ID-Hash: 3GQSLMCJTCZXCLAF3OVP4ISWFVVMAIWC X-Message-ID-Hash: 3GQSLMCJTCZXCLAF3OVP4ISWFVVMAIWC 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: --AtLlefsjnNnfGb9T Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 14, 2024 at 09:56:27AM +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: > 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 | 70 ++++++++++++++++++++---------------------------------- > checksum.h | 11 ++++----- > tap.c | 19 +++++++++++++-- > tcp.c | 42 +++++++++++++------------------- > udp.c | 11 +++++---- > 5 files changed, 72 insertions(+), 81 deletions(-) >=20 > diff --git a/checksum.c b/checksum.c > index 5613187a1c82..90dad96ee2c1 100644 > --- a/checksum.c > +++ b/checksum.c > @@ -59,12 +59,6 @@ > #include "util.h" > #include "ip.h" > =20 > -/* Checksums are optional for UDP over IPv4, so we usually just set > - * them to 0. Change this to 1 to calculate real UDP over IPv4 > - * checksums > - */ > -#define UDP4_REAL_CHECKSUMS 0 > - > /** > * sum_16b() - Calculate sum of 16-bit words > * @buf: Input buffer > @@ -133,31 +127,23 @@ uint16_t csum_ip4_header(const struct iphdr *ip4h) > } > =20 > /** > - * csum_udp4() - Calculate and set checksum for a UDP over IPv4 packet > - * @udp4hr: UDP header, initialised apart from checksum > - * @saddr: IPv4 source address > - * @daddr: IPv4 destination address > - * @payload: ICMPv4 packet payload > - * @len: Length of @payload (not including UDP) > + * proto_ipv4_header_psum() - Calculates the partial checksum of an > + * IPv4 header for UDP or TCP > + * @param: ip4h Pointer to the IPv4 header structure > + * @proto: proto Protocol number > + * Returns: Partial checksum of the IPv4 header > */ > -void csum_udp4(struct udphdr *udp4hr, > - struct in_addr saddr, struct in_addr daddr, > - const void *payload, size_t len) > +uint32_t proto_ipv4_header_psum(struct iphdr *ip4h, uint8_t proto) As per comments on the previous patch, I think there are some advantages to passing the specific header fields as parameters, rather than assuming they're already writen to the header structure. Especially since that's closer to the interface of the pre-existing functions. > { > - /* UDP checksums are optional, so don't bother */ > - udp4hr->check =3D 0; > - > - 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)); > - udp4hr->check =3D csum(payload, len, psum); > - } > + uint32_t sum =3D htons(proto); > + > + sum +=3D (ip4h->saddr >> 16) & 0xffff; > + sum +=3D ip4h->saddr & 0xffff; > + sum +=3D (ip4h->daddr >> 16) & 0xffff; > + sum +=3D ip4h->daddr & 0xffff; > + sum +=3D htons(ntohs(ip4h->tot_len) - 20); > + > + return sum; > } > =20 > /** > @@ -179,24 +165,20 @@ void csum_icmp4(struct icmphdr *icmp4hr, const void= *payload, size_t len) > } > =20 > /** > - * csum_udp6() - Calculate and set checksum for a UDP over IPv6 packet > - * @udp6hr: UDP header, initialised apart from checksum > - * @payload: UDP packet payload > - * @len: Length of @payload (not including UDP header) > + * proto_ipv6_header_psum() - Calculates the partial checksum of an > + * IPv6 header for UDP or TCP > + * @param: ip6h Pointer to the IPv4 header structure > + * @proto: proto Protocol number > + * Returns: Partial checksum of the IPv6 header > */ > -void csum_udp6(struct udphdr *udp6hr, > - const struct in6_addr *saddr, const struct in6_addr *daddr, > - const void *payload, size_t len) > +uint32_t proto_ipv6_header_psum(struct ipv6hdr *ip6h, uint8_t proto) > { > - /* 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 sum =3D htons(proto) + ip6h->payload_len; > + > + sum +=3D sum_16b(&ip6h->saddr, sizeof(ip6h->saddr)); > + sum +=3D sum_16b(&ip6h->daddr, sizeof(ip6h->daddr)); > =20 > - udp6hr->check =3D 0; > - /* Add in partial checksum for the UDP header alone */ > - psum +=3D sum_16b(udp6hr, sizeof(*udp6hr)); > - udp6hr->check =3D csum(payload, len, psum); > + return sum; > } > =20 > /** > diff --git a/checksum.h b/checksum.h > index b87ecd720df5..10533f708853 100644 > --- a/checksum.h > +++ b/checksum.h > @@ -6,24 +6,23 @@ > #ifndef CHECKSUM_H > #define CHECKSUM_H > =20 > +struct iphdr; > struct udphdr; > struct icmphdr; > +struct ipv6hdr; > struct icmp6hdr; > =20 > uint32_t sum_16b(const void *buf, size_t len); > 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(const struct iphdr *ip4h); > -void csum_udp4(struct udphdr *udp4hr, > - struct in_addr saddr, struct in_addr daddr, > - const void *payload, size_t len); > +uint32_t proto_ipv4_header_psum(struct iphdr *ip4h, uint8_t proto); > void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len); > -void csum_udp6(struct udphdr *udp6hr, > - const struct in6_addr *saddr, const struct in6_addr *daddr, > - const void *payload, size_t len); > +uint32_t proto_ipv6_header_psum(struct ipv6hdr *ip6h, uint8_t proto); > void csum_icmp6(struct icmp6hdr *icmp6hr, > const struct in6_addr *saddr, const struct in6_addr *daddr, > const void *payload, size_t len); > +uint32_t csum_unfolded(const void *buf, size_t len, uint32_t init); > uint16_t csum(const void *buf, size_t len, uint32_t init); > uint16_t csum_iov(struct iovec *iov, unsigned int n, uint32_t init); > =20 > diff --git a/tap.c b/tap.c > index 70f36a55314f..02b51100d089 100644 > --- a/tap.c > +++ b/tap.c > @@ -58,6 +58,12 @@ > #include "tap.h" > #include "log.h" > =20 > +/* Checksums are optional for UDP over IPv4, so we usually just set > + * them to 0. Change this to 1 to calculate real UDP over IPv4 > + * checksums > + */ > +#define UDP4_REAL_CHECKSUMS 0 > + > /* IPv4 (plus ARP) and IPv6 message batches from tap/guest to IP handler= s */ > static PACKET_POOL_NOINIT(pool_tap4, TAP_MSGS, pkt_buf); > static PACKET_POOL_NOINIT(pool_tap6, TAP_MSGS, pkt_buf); > @@ -188,7 +194,12 @@ void tap_udp4_send(const struct ctx *c, struct in_ad= dr src, in_port_t sport, > uh->source =3D htons(sport); > uh->dest =3D htons(dport); > uh->len =3D htons(udplen); > - csum_udp4(uh, src, dst, in, len); > + uh->check =3D 0; > + if (UDP4_REAL_CHECKSUMS) { > + uint32_t sum =3D proto_ipv4_header_psum(ip4h, IPPROTO_UDP); > + sum =3D csum_unfolded(uh, sizeof(struct udphdr), sum); > + uh->check =3D csum(in, len, sum); > + } > memcpy(data, in, len); > =20 > if (tap_send(c, buf, len + (data - buf)) < 0) > @@ -271,11 +282,15 @@ void tap_udp6_send(const struct ctx *c, > void *uhp =3D tap_push_ip6h(ip6h, src, dst, udplen, IPPROTO_UDP, flow); > struct udphdr *uh =3D (struct udphdr *)uhp; > char *data =3D (char *)(uh + 1); > + uint32_t sum; > =20 > uh->source =3D htons(sport); > uh->dest =3D htons(dport); > uh->len =3D htons(udplen); > - csum_udp6(uh, src, dst, in, len); > + uh->check =3D 0; > + sum =3D proto_ipv6_header_psum(ip6h, IPPROTO_UDP); > + sum =3D csum_unfolded(uh, sizeof(struct udphdr), sum); > + uh->check =3D csum(in, len, sum); I think it would still be good to have a single-call helper for the UDP checksums since we need them in two places: here for the "slow path" used by DHCP etc. and then in udp.c for the "fast path". > memcpy(data, in, len); > =20 > if (tap_send(c, buf, len + (data - buf)) < 1) > diff --git a/tcp.c b/tcp.c > index 35e240f4ffc3..6a0020f708c0 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -938,39 +938,25 @@ 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 > */ > -static void tcp_update_check_tcp4(struct tcp4_l2_buf_t *buf) > +static uint16_t tcp_update_check_tcp4(struct iphdr *iph) > { > - uint16_t tlen =3D ntohs(buf->iph.tot_len) - 20; > - uint32_t sum =3D htons(IPPROTO_TCP); > + struct tcphdr *th =3D (struct tcphdr *)(iph + 1); > + uint16_t tlen =3D ntohs(iph->tot_len) - 20; > + uint32_t sum =3D proto_ipv4_header_psum(iph, IPPROTO_TCP); > =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); > + return 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 uint16_t tcp_update_check_tcp6(struct ipv6hdr *ip6h) > { > - 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); > + uint32_t sum =3D proto_ipv6_header_psum(ip6h, IPPROTO_TCP); > =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; > + return csum(th, ntohs(ip6h->payload_len), sum); > } > =20 > /** > @@ -1380,7 +1366,8 @@ do { \ > =20 > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > =20 > - tcp_update_check_tcp4(b); > + b->th.check =3D 0; I think this initialisation should be folded into tcp_update_check_tcp4(). Otherwise th.check =3D=3D 0 is a pretty non-obvious pre-condition for that function. > + b->th.check =3D tcp_update_check_tcp4(&b->iph); > =20 > tlen =3D tap_iov_len(c, &b->taph, ip_len); > } else { > @@ -1399,7 +1386,12 @@ do { \ > =20 > SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq); > =20 > - tcp_update_check_tcp6(b); > + b->th.check =3D 0; Same for v6, of course. > + b->th.check =3D 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 e645c800a823..bf24288d5751 100644 > --- a/udp.c > +++ b/udp.c > @@ -618,6 +618,9 @@ static size_t udp_update_hdr4(const struct ctx *c, in= t n, in_port_t dstport, Hmm.. pre-existing bug(?) but udp_update_hdr4() should probably respect the UDP4_REAL_CHECKSUMS option as well. Using a common helper for there and tap_udp4_send() which checks it would be nice. > * > * Return: size of tap frame with headers > */ > +#pragma GCC diagnostic push > +/* ignore unaligned pointer value warning for &b->ip6h */ > +#pragma GCC diagnostic ignored "-Waddress-of-packed-member" > static size_t udp_update_hdr6(const struct ctx *c, int n, in_port_t dstp= ort, > const struct timespec *now) > { > @@ -673,16 +676,16 @@ 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, ntohs(b->ip6h.payload_len), > + proto_ipv6_header_psum(&b->ip6h, IPPROTO_UDP)); > b->ip6h.version =3D 6; > b->ip6h.nexthdr =3D IPPROTO_UDP; > b->ip6h.hop_limit =3D 255; > =20 > return tap_iov_len(c, &b->taph, ip_len); > } > +#pragma GCC diagnostic pop > =20 > /** > * udp_tap_send() - Prepare UDP datagrams and send to tap interface --=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 --AtLlefsjnNnfGb9T Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXNgTMACgkQzQJF27ox 2GcB/Q/9EZIBz34xpRqU0LPc674315nW17sCSHqfwaT85mxOyN7lKaC2T4tuDR6x vrxGaL4cA2XuRqZPiOS80dWQyGC0VuXTegmECRa8+LkBYPHHPCWFnUrbl9iaBCrB Ocyx8Ibu3Oz1TTThHHJvDkfSbTXKMLriYv2oEOEC/n78OFoZwnY9ZMIGuvU2X9ki g0+1n5X920oRiZncwen9foGyBAsamf9i1fnQ04wFjRuBatAM7N8KDee11xq+EYiE wety40S8lSmuRJKORpS80Q8R3NJi2hmmjY2jR9l3c6qrC7Pht9C9XVTaX3Kor/T6 SWllpc4Mk/3t6W25f47vOO5QMalsL2mXOmIMqtU/NB3AlWX3TaMkwkHA0pgumwdj pbxocrBunaZdjzygnnpgSElyGVL27gSkzyUO5+Xu5RgMZaw6JiCPbg7hFWsL4rXl mSaH7YlpWgCJj6kf1keA9Ve14Cq7OisOayJR/yRzkUAFhEHYOaPieM3R4oxS7avB iK9bF8tcON1ciFEp7DsPpKoWw95r90UwWpNKqv7mPRC5ahNbnYkSdME3Np+kQ4VS VSiso4mko2WhT0+YC8zqLE9AmVl5z/YqSlmGtzZp38z1hpubOmL7zsAK09SxmT7I AA9VEdvcYh08b+y6NW//U3BIvdWLmybV5/jfs1Tip1EgjqiSRNs= =KgVI -----END PGP SIGNATURE----- --AtLlefsjnNnfGb9T--