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 402815A0271 for ; Thu, 29 Feb 2024 02:48:12 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1709171283; bh=Wjxv65C4aVk3e7ZSHYqf/yWG+gy/ubsZ5B2iFOuF25g=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Oq7oGt1BXIFdf0dS1BDMH7SxioTGW1j9vPRvR4z5JuZbR9cB/RYads125lMlpMF0f YFRLWuqBWfFjYG6xbSczv8rNSFx1lAn4z4AnGE+NdbOyNzFKnNj9HPge0YMDwnjPef cJZPhbX1UypGRFfFpxvS/gJBkd84rGPynZjpVmSLzdK1HZKcEaF9KowxPpGgz0X1BQ lbIIFbGr7YTfBK5MUgO+3E1Dk9X/C35ecIkrUh5b7bXazfr4upIsI+yn6kyzcisCVC rGiiYGNXFEp+nk2lOZRQ1xw/FwqKFtuEBsaz0OWHP39JHnzPxqdTsIqpyXVHQK5hjG ErC5XmhvZrbVA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TlYyW2JNzz4wcH; Thu, 29 Feb 2024 12:48:03 +1100 (AEDT) Date: Thu, 29 Feb 2024 11:38:53 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v3 7/9] checksum: introduce functions to compute the header part checksum for TCP/UDP Message-ID: References: <20240217150725.661467-1-lvivier@redhat.com> <20240217150725.661467-8-lvivier@redhat.com> <04c99072-02ea-46a9-aac6-23116cb05fa1@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JXexAE45IR6FtIYH" Content-Disposition: inline In-Reply-To: <04c99072-02ea-46a9-aac6-23116cb05fa1@redhat.com> Message-ID-Hash: HWSIM5EKBJNSZQTASO6DXG4JZ42XTCYZ X-Message-ID-Hash: HWSIM5EKBJNSZQTASO6DXG4JZ42XTCYZ 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: --JXexAE45IR6FtIYH Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 28, 2024 at 02:26:18PM +0100, Laurent Vivier wrote: > On 2/19/24 04:08, David Gibson wrote: > > On Sat, Feb 17, 2024 at 04:07:23PM +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: > > > v3: > > > - function parameters provide tot_len, saddr, daddr and protoc= ol > > > rather than an iphdr/ipv6hdr > > > 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 | 67 ++++++++++++++++++++++++++++++++++++++++++---------= --- > > > checksum.h | 4 ++++ > > > tcp.c | 44 ++++++++++++++++------------------- > > > udp.c | 10 ++++---- > > > 4 files changed, 81 insertions(+), 44 deletions(-) > > >=20 > > > diff --git a/checksum.c b/checksum.c > > > index 511b296a9a80..55bf1340a257 100644 > > > --- a/checksum.c > > > +++ b/checksum.c > > > @@ -134,6 +134,30 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8= _t protocol, > > > return ~csum_fold(sum); > > > } > > > +/** > > > + * proto_ipv4_header_psum() - Calculates the partial checksum of an > > > + * IPv4 header for UDP or TCP > > > + * @tot_len: Payload length > > > + * @proto: Protocol number > > > + * @saddr: Source address > > > + * @daddr: Destination address > > > + * @proto: proto Protocol number > > > + * 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(ntohs(tot_len) - 20); > > > + > > > + return psum; > > > +} > > > + > > > /** > > > * csum_udp4() - Calculate and set checksum for a UDP over IPv4 pac= ket > > > * @udp4hr: UDP header, initialised apart from checksum > > > @@ -150,14 +174,10 @@ void csum_udp4(struct udphdr *udp4hr, > > > 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)); > > > + uint32_t psum =3D proto_ipv4_header_psum(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 +200,26 @@ void csum_icmp4(struct icmphdr *icmp4hr, const v= oid *payload, size_t len) > > > icmp4hr->checksum =3D csum(payload, len, psum); > > > } > > > +/** > > > + * proto_ipv6_header_psum() - Calculates the partial checksum of an > > > + * IPv6 header for UDP or TCP > > > + * @payload_len: 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 protoc= ol, > > > + struct in6_addr saddr, struct in6_addr daddr) > >=20 > > Hrm, this is passing 2 16-byte IPv6 addresses by value, which might > > not be what we want. >=20 > The idea here is to avoid the pointer alignment problem (&ip6h->saddr and > &ip6h->daddr can be misaligned). Ah, right. That's a neat idea, but I'm not sure it really helps: I think it will just move the misaligned access from inside the function to the call site, where we try to marshal the parameter from something unaligned. > Is it a better solution to copy the content of ip6h->saddr and ip6h->daddr > to some local variables and then provide the pointers of the local variab= les > to proto_ipv6_header_psum()? Honestly, I'm not sure. > > > +{ > > > + uint32_t sum =3D htons(protocol) + payload_len; > >=20 > > Uh.. doesn't that need to be htons(payload_len + sizeof(struct > > ipv6hdr)) rather than simply payload_len? > >=20 >=20 > payload_len is: >=20 > - b->ip6h.payload_len (from udp_update_hdr6()) > - ip6h->payload_len (from tcp_update_check_tcp6()) >=20 > and in ip6h payload_len is: >=20 > - htons(udp6_l2_mh_sock[n].msg_len + sizeof(b->uh)) (from udp_update_hdr6= ()) > - htons(plen + sizeof(struct tcphdr)) (from tcp_fill_ipv6_header()) >=20 > So this is correct... but Ah, right. Not sure why I thought the ip6h length needed to be included. As a rule htons(x) + y is always suspect, because you generally can't do math on values that aren't host endian. We get away with it in these csum functions because the way they're folded means the answers end up the same - as long as we're consistent about it, anyway. > csum_udp6() uses "len" from tap_udp6_send(), so there is a bug here. >=20 > but there is also a problem with proto_ipv4_header_psum() that need host > endianness and tcp_update_check_tcp4() provides network endianness... >=20 > The first idea was to use the value from ip6h payload_len as it is alread= y computed. > But mixing network endianness and host endianness appears to be a bad ide= a... Right. As a rule I really dislike putting non-host-endian values in a plain u16 local or parameter, because it's really easy to think it's just a number, rather than a funny encoding of a number. Likewise, I think it's a lot easier to keep track of things if every field of a struct has a strict endianness, which we never change in place, even temporarily. (Ideally, in fact, I'd prefer to see non-host-endian values always in an encapsulating type that won't let you do math on them, but that's not always practical in C). --=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 --JXexAE45IR6FtIYH Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXf0hgACgkQzQJF27ox 2GfTIQ/+MB/fvkldPV65QBjo8q5GGdgIgNOCGgUP+R5OV/otAfRGUQ8+Z82Flq37 1t/dL+FFCpGcRPWSL0/nAXu/0z7jKytZJqDm7rqu++1wGEEidUbB1zh8g9cH34K5 OiozSwieNKsw73GImoMm0CpgUC6LipTD+Bve5OAzTdEFvwT7JxOOFSiqcXQZNPRf Jqw99IDKq8Oh0Da0AHs5L+nUBsgxHwvfNV9G5PHeTh2L5bEIr2OO5ODtdxbWemzG VeUF2HVz45V/IgUZNj7GeN208S9aAnWT7hZ9fuL/j74hPq62Eba9O+yHlEMJxayF g2wumg32ipXPqQSi0QEC7ECPW4r3ddBWfdmh35G1FCzoPNQ7WNYkXtXZftdizpbX OnOqxTC8LeF1qrkNpFTzlGtjbl9sHF5HzBOYKOzvJ8GQTqLg0Si74zHPVsXU5oo3 jzNBV/qKhoAT4sM2fZrAX6dcvXVq+63kRMhsH2lPI0ftnaxjE32D7yE8JjT5UDYh iIXAnvwOfblWEvYKMgfiW1JOJrNVY76bko1GmLRURHBK/5oXKpUFunYvBLyTqLBD ubQ8tb8iwkwX7RjfSRFimbHBW/UCwH/GsJ9Nr9YffxdiMgMU8UOh11CuX49t74Jd sZ0J3fElXPBrFQ2lCUQ4W0yUW17EAsz2xkaoDAZDtTD11WnbQ9g= =1JhN -----END PGP SIGNATURE----- --JXexAE45IR6FtIYH--