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 1E0375A0271 for ; Sat, 2 Mar 2024 05:56:43 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1709355396; bh=jbsHTkV0xk9Sc5/glPRXfSL8IOvG1nvlrF1IIpmX78k=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bIMwJTtLBaNeljwkhhwQhnMihZa9jhbo4iitvQxKykHI/pWe1BUnsVGNPJGyHDyAz EYN9Um0NEhQxhDuMqxeJFeE5/DunfZPQqvQR6rgO87Kb36UVHJKgqsJYIo/aIO0v/S +1AoSKgJY8/kBea85EWJYvxDhrJmKJEcDODBoqJylhAD/zvWWz6C+NLGIhYoYEOYhF SmCjOG+ZMkT+GJHAhVwpPVREmNBjlTScXV1vbKVOdsrHJT46mu+XR8qCddTilD8V8i clVKdnh7Ps2uV0LbZqElzXIWsAQcMaqDnJEDX+cAKIq4h7U2cTpfrFpjJzxgeYQMYo Ocn/JQ2pXrHeA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Tmt3827QFz4wcb; Sat, 2 Mar 2024 15:56:36 +1100 (AEDT) Date: Fri, 1 Mar 2024 23:23:41 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v3 6/9] checksum: use csum_ip4_header() in udp.c and tcp.c Message-ID: References: <20240217150725.661467-1-lvivier@redhat.com> <20240217150725.661467-7-lvivier@redhat.com> <20240229172406.6a8331f1@elisabeth> <20240301085845.0da98048@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="S6WJRRTgu1IpYqBi" Content-Disposition: inline In-Reply-To: <20240301085845.0da98048@elisabeth> Message-ID-Hash: RIBPWPM6KASWVWE6K6CRYJMDSRVB7BPF X-Message-ID-Hash: RIBPWPM6KASWVWE6K6CRYJMDSRVB7BPF 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: Laurent Vivier , 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: --S6WJRRTgu1IpYqBi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 01, 2024 at 08:58:45AM +0100, Stefano Brivio wrote: > On Fri, 1 Mar 2024 10:10:52 +1100 > David Gibson wrote: >=20 > > On Thu, Feb 29, 2024 at 05:24:06PM +0100, Stefano Brivio wrote: > > > On Sat, 17 Feb 2024 16:07:22 +0100 > > > Laurent Vivier wrote: > > > =20 > > > > We can find the same function to compute the IPv4 header > > > > checksum in tcp.c, udp.c and tap.c > > > >=20 > > > > Use the function defined for tap.c, csum_ip4_header(), but > > > > with the code used in tcp.c and udp.c as it doesn't need a fully > > > > initialiazed IPv4 header, only protocol, tot_len, saddr and daddr. > > > >=20 > > > > Signed-off-by: Laurent Vivier > > > > --- > > > >=20 > > > > Notes: > > > > v3: > > > > - function parameters provide tot_len, saddr, daddr and proto= col > > > > rather than an iphdr > > > > =20 > > > > v2: > > > > - use csum_ip4_header() from checksum.c > > > > - use code from tcp.c and udp.c in csum_ip4_header() > > > > - use "const struct iphfr *", check is not updated by the > > > > function but by the caller. > > > >=20 > > > > checksum.c | 17 +++++++++++++---- > > > > checksum.h | 3 ++- > > > > tap.c | 3 ++- > > > > tcp.c | 24 +++--------------------- > > > > udp.c | 20 ++------------------ > > > > 5 files changed, 22 insertions(+), 45 deletions(-) > > > >=20 > > > > diff --git a/checksum.c b/checksum.c > > > > index 74e3742bc6f6..511b296a9a80 100644 > > > > --- a/checksum.c > > > > +++ b/checksum.c > > > > @@ -57,6 +57,7 @@ > > > > #include > > > > =20 > > > > #include "util.h" > > > > +#include "ip.h" > > > > #include "checksum.h" > > > > =20 > > > > /* Checksums are optional for UDP over IPv4, so we usually just set > > > > @@ -116,13 +117,21 @@ uint16_t csum_fold(uint32_t sum) > > > > uint16_t csum(const void *buf, size_t len, uint32_t init); > > > > =20 > > > > /** > > > > - * csum_ip4_header() - Calculate and set IPv4 header checksum > > > > + * csum_ip4_header() - Calculate IPv4 header checksum > > > > * @ip4h: IPv4 header > > > > */ > > > > -void csum_ip4_header(struct iphdr *ip4h) > > > > +uint16_t csum_ip4_header(uint16_t tot_len, uint8_t protocol, > > > > + uint32_t saddr, uint32_t daddr) > > > > { > > > > - ip4h->check =3D 0; > > > > - ip4h->check =3D csum(ip4h, (size_t)ip4h->ihl * 4, 0); > > > > + uint32_t sum =3D L2_BUF_IP4_PSUM(protocol); =20 > > >=20 > > > Now that we use this macro, Coverity Scan realises that it's broken: > > >=20 > > > #define L2_BUF_IP4_PSUM(proto) ((uint32_t)htons_constant(0x4500) + = \ > > > (uint32_t)htons_constant(0xff00 | (proto))) > > >=20 > > > ...but proto is eight (lower) bits, so this actually ignores 'proto'.= =20 > >=20 > > Uh... how so? >=20 > Oops, sorry, it's not broken, and this is a false positive due to the > fact that __bswap_constant_16() (which htons_constant() resolves to, on > little-endian) is defined, for example in glibc, as: >=20 > #define __bswap_constant_16(x) \ > ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)) >=20 > and in this case the first term of the | resolves to a constant value, > 0xff, because 0xffxx >> 8 is 0xff for any value of xx. Right. This really seems overzealous of coverity: it seems like any occasion where the compiler would constant fold could result in a similar warning. > I couldn't think of a "solution", yet. Making it an inline function rather than a macro might be enough to convince Coverity. Otherwise we could just mark it as a false positive in the Coverity web 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 --S6WJRRTgu1IpYqBi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmXhyKoACgkQzQJF27ox 2GdimBAAgqf2aaJQAQS/ZNRGMLW2wWVGH7Eb9QBqaSFO7go/OJQiE1OFH/BJkINA DtpuillGiJfrY+htL385p/Sj8UuT4eKCYTXVpAWU2hXdQkRe+ACmdTwwTwg93K/Z UXyC5PYEHbKStQ4H/XboKyZ94IeBpjVwpdtxGuV/PGZsR21QDNgt89wUEhmIr32u Yb+gqveozsSW3s34tvVLQw+HemL6zXZYF6Mh9Tq9Bc3uZA0ZOpFx6x16XKJj4l+L YjZpwizE3WV9Cc1vpecLf9jSkwho5FyJXCSoa2TtPOGwHC38/YUdlt2W8RvSh1s/ p1ULZkI9+5eer4heJCnCW6AJ2CRenPZwu1yNkzd3sorXi9qTSiStzHzRIB0ouny3 8iQ8zxZpm29fuCja5id44swOpeb1ThcJ9J6NJCU3cfTC0W3obY0AqnJBlRFjaR6i 5Nqd3r+hKDVCMOu+n+YSWI0cwdlMabgjmttT6F/7u8TsJGcutoWT5bq+nWULaUDK mM3HKe2hO0N0DYp9y594HKsjctBDNtVl+vbhxZR4mjAQ27GMN3PaDOCbDy/UIYra /5qsFhRG+aPar7hqVi93DU5pwZ654oCfch4kTfs6Npvbuk3PNLAEM7NfbegUSLQo 7lal3X6yZnbNlQwa8OONlSLfxAfYLetOrWxrbjgDtKHB7oBf4pg= =A3qS -----END PGP SIGNATURE----- --S6WJRRTgu1IpYqBi--