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 7B0935A0269 for ; Tue, 18 Oct 2022 14:08:11 +0200 (CEST) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4MsCLH4CTzz4xG5; Tue, 18 Oct 2022 23:08:07 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1666094887; bh=z9+OGC6peivs2VVKkbLdVvJspUSTZCp3oiJNvXQ4YQg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=pgM+nR0yC6Kq9K7ahzAGFlme0qQ/N3GhtlzYjq90YoeW8ERNENKtJ9dR4k46alpMS OVohA2FcCxzrsq5+qMsBc3kLfjrJUnbWy+Jk6s/5fEUj7MF0uOaqRd6fjNXTS76uYB 1tRvZYmVM1v5VjJqb1DB/Sfw2KFYlTidkeZ8v3eA= Date: Tue, 18 Oct 2022 23:06:11 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 02/14] Add csum_icmp4() helper for calculating ICMPv4 checksums Message-ID: References: <20221017085807.473470-1-david@gibson.dropbear.id.au> <20221017085807.473470-3-david@gibson.dropbear.id.au> <20221018050151.5739f1ad@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xrgJxZpUjwutWDvu" Content-Disposition: inline In-Reply-To: <20221018050151.5739f1ad@elisabeth> Message-ID-Hash: YCZATETCTHBJXTFJTO5GUSQHNULIL6BU X-Message-ID-Hash: YCZATETCTHBJXTFJTO5GUSQHNULIL6BU 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.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: --xrgJxZpUjwutWDvu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 18, 2022 at 05:01:51AM +0200, Stefano Brivio wrote: > On Mon, 17 Oct 2022 19:57:55 +1100 > David Gibson wrote: >=20 > > Although tap_ip_send() is currently the only place calculating ICMPv4 > > checksums, create a helper function for symmetry with ICMPv6. For futu= re > > flexibility it allows the ICMPv6 header and payload to be in separate > > buffers. > >=20 > > Signed-off-by: David Gibson > > --- > > checksum.c | 15 +++++++++++++++ > > checksum.h | 2 ++ > > tap.c | 4 +--- > > 3 files changed, 18 insertions(+), 3 deletions(-) > >=20 > > diff --git a/checksum.c b/checksum.c > > index 0e207c8..c8b6b42 100644 > > --- a/checksum.c > > +++ b/checksum.c > > @@ -52,6 +52,7 @@ > > #include > > #include > > =20 > > +#include > > #include > > =20 > > /** > > @@ -107,6 +108,20 @@ uint16_t csum_unaligned(const void *buf, size_t le= n, uint32_t init) > > return (uint16_t)~csum_fold(sum_16b(buf, len) + init); > > } > > =20 > > +/** > > + * csum_icmp4() - Calculate checksum for an ICMPv4 packet >=20 > "Calculate and set"? Done. > By the way, there's no such thing as ICMPv4 -- > it's ICMP. Technically, yes, but I kind of wanted to make it clear at a glance that these are IPv4 specific functions. I'd also like to avoid the implication that v4 is the "normal" sort. I've changed from "ICMPv4" to "ICMP" in the comments, but I've left the '4's in the various names > > + * @icmp4hr: ICMPv4 header, initialized apart from checksum >=20 > ...-ised, if you respin. For consistency, I would call this 'ih'. >=20 > > + * @payload: ICMPv4 packet payload > > + * @len: Length of @payload (not including ICMPv4 header) > > + */ > > +void csum_icmp4(struct icmphdr *icmp4hr, const void *payload, size_t l= en) >=20 > I guess csum_icmp() is preferable. Indeed, for TCP and UDP 'tcp4' and > 'udp4' make sense because those are the same protocols over IPv4 and > IPv6. See above. > > +{ > > + /* Partial checksum for ICMPv4 header alone */ > > + uint32_t hrsum =3D sum_16b(icmp4hr, sizeof(*icmp4hr)); >=20 > A white line would be nice here. Done. >=20 > I would call this psum (same as in csum_icmp6()) Changed to 'psum'. > or hdrsum, 'hr' isn't > really used for "header" elsewhere. Well.. except as a suffix, 'ihr' etc. > > + icmp4hr->checksum =3D 0; > > + icmp4hr->checksum =3D csum_unaligned(payload, len, hrsum); > > +} > > + > > /** > > * csum_icmp6() - Calculate checksum for an ICMPv6 packet > > * @icmp6hr: ICMPv6 header, initialized apart from checksum > > diff --git a/checksum.h b/checksum.h > > index 2c72200..ff95cf9 100644 > > --- a/checksum.h > > +++ b/checksum.h > > @@ -6,11 +6,13 @@ > > #ifndef CHECKSUM_H > > #define CHECKSUM_H > > =20 > > +struct icmphdr; > > 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); > > +void csum_icmp4(struct icmphdr *ih, const void *payload, size_t len); > > void csum_icmp6(struct icmp6hdr *ih, > > const struct in6_addr *saddr, > > const struct in6_addr *daddr, > > diff --git a/tap.c b/tap.c > > index aafc92b..f082901 100644 > > --- a/tap.c > > +++ b/tap.c > > @@ -148,9 +148,7 @@ void tap_ip_send(const struct ctx *c, const struct = in6_addr *src, uint8_t proto, > > uh->check =3D 0; > > } else if (iph->protocol =3D=3D IPPROTO_ICMP) { > > struct icmphdr *ih =3D (struct icmphdr *)(iph + 1); > > - > > - ih->checksum =3D 0; > > - ih->checksum =3D csum_unaligned(ih, len, 0); > > + csum_icmp4(ih, ih + 1, len - sizeof(*ih)); > > } > > =20 > > if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1) < 0) >=20 --=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 --xrgJxZpUjwutWDvu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmNOlqwACgkQgypY4gEw YSJk6Q/+OiIUuSmJKpVUg5ltzRkNrRUV/o2D3F57chc84QlIPzAYU6wWW328/V+7 th7wZn5MinGLGVXSBG04vZZdNUNH2iQ3Q2gRyfXB5e+oGNfoyqwYiLjsltLt/p5W F9JOpz3nYrxCytBNMXZAhz05XvkYODpQvdAxnmOFkPW28108PHYenLSo4qLg/V6D Vo85c/x45Wit16QYcpFkrJ2Fm7qOcSpzb8cH+sjuZy/zQz+N5WBrZVf0Kk/d8CA2 BdtjDuhYTQJkrFF6lqQWy5YkPEtyNpNsecNyTjHvZRGPTf523wmQG3c0gMgaNBjb roaxuItDQo6n/gQ0JoiVeASz4kVqFOU4gHCUKpbvhQD9m/ADr4zE/UyJTvVZLNKF UBegQY2M21QviFe5h4mI21U3RO4g5wG4PIFog2su9puSwDuSZcRRY8p3SpLR23Uc TFWQH46VHjWSLs8RwS8eID5PT6Vs9FMuGDKyO6ORfpzU+9ZfdOdf3hl2skJqCmRr C+Ya4FLaY44h1uQqJvo5U0yxcP67AFHS6r7eMnGpd94/jZeBVBeEIpEEhYaOJ8Pe go44MsxMHc4FXlEgjaIuUBrnTsB4lgEVnWXg9wLDdqOvoPbNpNgdApSiay/akfxh 2dO69QPtxaeXCCKBXTVzVEr4ABw6jyUC5dn47re1F02uSK/80Ck= =ruGE -----END PGP SIGNATURE----- --xrgJxZpUjwutWDvu--