From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202408 header.b=QehVQZ9U; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 66DDE5A026E for ; Mon, 30 Sep 2024 06:24:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1727670274; bh=oWgX1nJ+4nuAj4EBe7LGQNitBeRJaOBqOgp9FlsWHrE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QehVQZ9UxIeNRi+vq7jux2XpGCDQbJWIk8fkeTfcMpsoHSGT61nU+DkFvw1xsjXzx Z9kEk5TMXni/mGpmTe1j7+NNxbSBvkioJqhRsr9zc0EpSIlVCwfr5OePxjYrPEe/4w zAt00mwsRAeBOLyjI618nFTmT8Z/AFa1bfmfqtxLV9e+DWhgIDkdt0QztkSoQg4sMP WyLW0bs/HKQ2i5lajW2rL123avFC5iiXuMO/hKR8urBoAvtet7JvWrQ3XcJCTrQARb A4NedQsFt2JClhQ9bpkvQFkt5eRBM96rOT+9ybe+tUEwiYUyu+UbokBTZLKQJeSDb2 a4hHt5KZfqAvw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XH7JL56ltz4wcl; Mon, 30 Sep 2024 14:24:34 +1000 (AEST) Date: Mon, 30 Sep 2024 12:59:15 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v2 4/4] tcp: Update TCP checksum using an iovec array Message-ID: References: <20240925081125.205974-1-lvivier@redhat.com> <20240925081125.205974-5-lvivier@redhat.com> <1ccee20e-fbb5-47ab-8a87-939e7c81ed54@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lEXQo23r+6h4qCUQ" Content-Disposition: inline In-Reply-To: <1ccee20e-fbb5-47ab-8a87-939e7c81ed54@redhat.com> Message-ID-Hash: MFT5PAESPOVU5KYLZKZK4DHLWIJGFLJ7 X-Message-ID-Hash: MFT5PAESPOVU5KYLZKZK4DHLWIJGFLJ7 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: --lEXQo23r+6h4qCUQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 27, 2024 at 03:49:50PM +0200, Laurent Vivier wrote: > On 26/09/2024 03:45, David Gibson wrote: > > On Wed, Sep 25, 2024 at 10:11:25AM +0200, Laurent Vivier wrote: > > > TCP header and payload are supposed to be in the same buffer, > > > and tcp_update_check_tcp4()/tcp_update_check_tcp6() compute > > > the checksum from the base address of the header using the > > > length of the IP payload. > > >=20 > > > In the future (for vhost-user) we need to dispatch the TCP header and > > > the TCP payload through several buffers. To be able to manage that, we > > > provide an iovec array that points to the data of the TCP frame. > > > We provide also an offset to be able to provide an array that contains > > > the TCP frame embedded in an lower level frame, and this offset points > > > to the TCP header inside the iovec array. > > >=20 > > > Signed-off-by: Laurent Vivier > > > --- > > >=20 > > > Notes: > > > v2: > > > - s/payload_offset/l4offset/ > > > - check memory address of the checksum (alignment, iovec bound= aries) > > >=20 > > > checksum.c | 1 - > > > tcp.c | 116 ++++++++++++++++++++++++++++++++++++++++----------= --- > > > 2 files changed, 88 insertions(+), 29 deletions(-) > > >=20 > > > diff --git a/checksum.c b/checksum.c > > > index 68ffaddb5bb0..4854c1937c39 100644 > > > --- a/checksum.c > > > +++ b/checksum.c > > > @@ -503,7 +503,6 @@ uint16_t csum(const void *buf, size_t len, uint32= _t init) > > > * > > > * Return: 16-bit folded, complemented checksum > > > */ > > > -/* cppcheck-suppress unusedFunction */ > > > uint16_t csum_iov(const struct iovec *iov, size_t n, size_t offset, > > > uint32_t init) > > > { > > > diff --git a/tcp.c b/tcp.c > > > index c9472d905520..f0a6f7a507a7 100644 > > > --- a/tcp.c > > > +++ b/tcp.c > > > @@ -755,36 +755,81 @@ static void tcp_sock_set_bufsize(const struct c= tx *c, int s) > > > } > > > /** > > > - * tcp_update_check_tcp4() - Update TCP checksum from stored one > > > - * @iph: IPv4 header > > > - * @bp: TCP header followed by TCP payload > > > - */ > > > -static void tcp_update_check_tcp4(const struct iphdr *iph, > > > - struct tcp_payload_t *bp) > > > + * tcp_update_check_tcp4() - Calculate TCP checksum for IPv6 > >=20 > > Nit: s/IPv6/IPv4/ > >=20 > > > + * @src: IPv4 source address > > > + * @dst: IPv4 destination address > > > + * @iov: Pointer to the array of IO vectors > > > + * @iov_cnt: Length of the array > > > + * @l4offset: IPv4 payload offset in the iovec array > > > + */ > > > +void tcp_update_check_tcp4(struct in_addr src, > > > + struct in_addr dst, > > > + const struct iovec *iov, int iov_cnt, > > > + size_t l4offset) > > > { > > > - uint16_t l4len =3D ntohs(iph->tot_len) - sizeof(struct iphdr); > > > - struct in_addr saddr =3D { .s_addr =3D iph->saddr }; > > > - struct in_addr daddr =3D { .s_addr =3D iph->daddr }; > > > - uint32_t sum =3D proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, = daddr); > > > + size_t check_ofs; > > > + __sum16 *check; > > > + int check_idx; > > > + uint32_t sum; > > > + > > > + sum =3D proto_ipv4_header_psum(iov_size(iov, iov_cnt) - l4offset, > > > + IPPROTO_TCP, src, dst); > >=20 > > Previously, we took the size from the IP header, which we'd previously > > calculated. It seems a shame to replace that with a call to > > iov_size() which will make another pass through the whole vector. > >=20 > > > + > > > + check_idx =3D iov_skip_bytes(iov, iov_cnt, > > > + l4offset + offsetof(struct tcphdr, check), > > > + &check_ofs); > > > + > > > + if (check_idx >=3D iov_cnt) > > > + die("TCP4 buffer is too small"); > > > + if (check_ofs + sizeof(*check) > iov[check_idx].iov_len) > > > + die("TCP4 checksum field memory is not contiguous"); > > > + > > > + check =3D (__sum16 *)((char *)iov[check_idx].iov_base + check_ofs); > >=20 > > Strictly speaking, it's UB to even *create* an improperly aligned > > pointer, even if you never dereference it. So the alignment check > > should go before casting to (__sum16 *). > >=20 > > > - bp->th.check =3D 0; > > > - bp->th.check =3D csum(bp, l4len, sum); > > > + if ((uintptr_t)check & (__alignof__(*check) - 1)) > > > + die("TCP4 checksum field is not correctly aligned in memory"); > >=20 > > I really think it would be worth packaging this logic (skip_bytes + > > contiguous check + alignment check + pointer cast) into another helper > > (iov_field()?). I strongly suspect we'll have further use for it down > > the line. > >=20 >=20 > I'm addressing all your other comments but I don't really have the time to > write a generic and clean function to do that. I prefer to duplicate the > code for the moment, we will be able to cleanup this in the future. Ok, fair enough. If I get a chance, I might take a crack at writing a suitable helper. --=20 David Gibson (he or they) | 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 --lEXQo23r+6h4qCUQ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmb6FAIACgkQzQJF27ox 2GcQAhAAg7hY/sN6k4Yh6TMcrlnRLHLHyIZ6sFwYnkdJEtJxiUS0Opnioh9BcpMs Wp3SmGhnhLyt3f2kn6V3uAOrKgg75jPVO+6P07DCeYpY2FcWNGNgxI2GUcIq/LQr Pq2Zj2Lt1dOtcmLaFXP7/Qpni9wHQUYIz5vTjLK2KgddYBLYoG08E8eZsDt1tNby 9xSL4gMrJp4lgELPjH+uJPzuUPfLe8UDye+2N5Rolieg68VqhWiE31ITSc7EfwRW eZLzvZARmv9ziLxEa7fh7GP1Zcidy0YdrN7JyL0Md6pKjO8j/qf0GZfJj7cUg/1a hUrN3eu8O6oPrFT5L7oNKYe2edwa4yHf69eiCkjTnx46fUoyh/QV0cnIqH4hf4qN IfAsI0i2NE2rr7PO3CDGh0yEtOyd+b32fQRwT+3S4h/mO/MCvNFBTL2t9DErBSzI Uqjxdbl19TuHF/U0IUfDAjoHISp+pqQraQkXLimKyrKHZfrrtIKIDYArZe1IUgjc I9m+L03k3iFqsyvGAQlZjeo6msbsnyn3a1aWNpzqDVBesGdS7C3vcDXzw6LSHVyR 0HqmLzSCOYBoGZmyD9/Y5d7ebkRzM9blpBnZCU80PVwWP+0L46XG+sII43PYuZPO dsef36IMiok0BGus6z4dCficc+XegEMA0OW1/KtU2JxED5kU4KA= =0t/4 -----END PGP SIGNATURE----- --lEXQo23r+6h4qCUQ--