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=qjU1nhuP; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id CC79C5A004C for ; Wed, 25 Sep 2024 09:02:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1727247718; bh=1UWauOoe+yZ27reIUfqxU6WINDotHMneNLkD2wa35jM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qjU1nhuPSiwePbG0828f8d63HBr9THLOPhTKyd9LEvueALHC0DOm0oV+/FPl84hkX pjDykMIwEcaLSzUpHi64B7JVLT2uETd+4DcpxRMtEyjn6z2GB7KlyXNA7e1PxrNTU/ hdbR+wAbrUtVNQ6ULtBsrSl3XozpLzMreA6ECtITlGO4AQx5FPq9x0PujHA7xVqsAZ HFAMCZBDD7YH219wSAfJKKciq1Is2YpBiwqm2wi1SvAPtg9JdHnWlI2e9AfoYzKELn N8Gb/J5/Tab2oK1OBk22BjToIkDJr0yozspIFPVU+DRaRB0ove/f83jIA07ccuYmFU OED3mBmL52esw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XD72G1Wfcz4xPR; Wed, 25 Sep 2024 17:01:58 +1000 (AEST) Date: Wed, 25 Sep 2024 17:01:52 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH 4/4] tcp: Update TCP checksum using an iovec array Message-ID: References: <20240924154642.182857-1-lvivier@redhat.com> <20240924154642.182857-5-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="c5dN2dBZ8YcoQLbw" Content-Disposition: inline In-Reply-To: Message-ID-Hash: KAGOX7M5STRZFQQTXC4NIMR5GJML6MAG X-Message-ID-Hash: KAGOX7M5STRZFQQTXC4NIMR5GJML6MAG 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: --c5dN2dBZ8YcoQLbw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 25, 2024 at 08:40:28AM +0200, Laurent Vivier wrote: > On 25/09/2024 03:12, David Gibson wrote: > > On Tue, Sep 24, 2024 at 05:46:42PM +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 > > > --- > > > checksum.c | 1 - > > > tcp.c | 100 ++++++++++++++++++++++++++++++++++++++------------= --- > > > 2 files changed, 72 insertions(+), 29 deletions(-) > > >=20 > > > diff --git a/checksum.c b/checksum.c > > > index f80db4d309a2..96ccfe2af50b 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..efd4037ed008 100644 > > > --- a/tcp.c > > > +++ b/tcp.c > > > @@ -755,36 +755,65 @@ 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 > > > + * @src: IPv4 source address > > > + * @dst: IPv4 destination address > > > + * @iov: Pointer to the array of IO vectors > > > + * @iov_cnt: Length of the array > > > + * @payload_offset: IPv4 payload offset in the iovec array > >=20 > > You explain it here, but "payload_offset" is a bit unclear if you're > > not sure which layer it's talking about. "l4offset" maybe? > >=20 > > > + */ > > > +void tcp_update_check_tcp4(struct in_addr src, > > > + struct in_addr dst, > > > + const struct iovec *iov, int iov_cnt, > > > + size_t payload_offset) > > > { > > > - 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; > >=20 > > What's a __sum16? >=20 > It's the type of "check" in struct tcphdr. Huh, ok. > > > + int check_idx; > > > + uint32_t sum; > > > + > > > + sum =3D proto_ipv4_header_psum(iov_size(iov, iov_cnt) - payload_off= set, > > > + IPPROTO_TCP, src, dst); > > > + > > > + check_idx =3D iov_skip_bytes(iov, iov_cnt, > > > + payload_offset + offsetof(struct tcphdr, check), > > > + &check_ofs); > > > + > > > + check =3D (__sum16 *)((char *)iov[check_idx].iov_base + check_ofs); > >=20 > > So.. it's not likely, but it's possible for the first byte of the > > checksum to be in one iovec and the second byte in another. This > > whole construction is a bit awkward too. >=20 > I'm guessing iov_base and iov_len are 32bit aligned, and address of "chec= k" > too (as it is from tcphdr in memory, that should be 32bit aligned, and > offset of "check" is 16), so a 16 bit value cannot be shared between two > iovecs. I'm guessing that any 32bit value we take from a structure will n= ot > cross boundary of an iovec. That's probably true.. but I think we should actually verify / assert that somewhere. > > I think we want another helper on top of iov_skip_bytes(). It would > > retreive a pointer to a field of a given length and offset within the > > IOV, returning NULL if that can't be found contiguously. It could > > have a macro wrapper that fills in some of the details based on a > > type. For now I'd imagine we just give up if it returns NULL, but > > that's enough to reduce a potential out of bounds memory access to > > merely breaking one connection. If we ever need it, we can add a slow > > path to handle that case. > >=20 > > There are a couple of other curly cases to consider too, alas: what if > > the field you request does exist contiguously, but isn't properly > > aligned for the type we want to access it as? Then there's the > > question of whether doing this will run afoul of the type-based > > aliasing rules. >=20 > In our case, "check" is a field of "struct tcphdr", I think it's sane to > think it is correctly aligned. It will be correctly aligned as long as the struct tcphdr itself is correctly aligned. We can probably count on that being 2-byte aligned w.r.t. the start of the frame, but probably no more since the L2 header is 14 bytes long. If there were odd-length iovs, and the TCP header wasn't in the first, that could destroy its alignment though. Actually... the same thing could happen in the first IOV if iov_base wasn't aligned, which is admittedly probably even less likely. > I don't want to write complicated code only to write the checksum of > the tcp header. I agree, but I think we should at least test and bail with an error message if our assumptions about the alignments of the IOVs were given aren't true. --=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 --c5dN2dBZ8YcoQLbw Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbztV8ACgkQzQJF27ox 2Gc6IRAAgx7QF23iOjAoBcb0tHCuCCQlHWkO8uZsIP9+tNTUg2i5FL1OFiL3TEXh GJprbd5YYuRFy6DhNmgi6796hS3gQjtIXgwsrRIuJk9Q1WmYhVc3mpsuxRmsCoTn Hj6PSHmFuXARbU8fC0dQ6pcWhY/hBs95P+JOQk3tIykn7nocGpCVVYTHFXt9YM88 EnWFhrnTzlEm7xOh+VvdjOc7Qtod1c4KsthHG14+5yihTTO8DnuJNb3pkpwra107 Umr7aAo+3vjEEmgRhehDxx6QuuVaumWAG0zVD15JUCcGFRF446fDwjLQLDMXNeMj q3rQmrYlyVJfqdQMDxMPeM3mdvkkqxDzQnITugvPYdAVDbBWqFungvkglNFvCwHl Rs08GipUfTvCR+9ClnPeNfSsfe/uY0wJufis+0/i8pTHNK3M20gC+EVJ6j9PmoAU VdVfOLX5NNHcLe09R292InK+wCMI6WHE/6rm6km/SdKA5u2fyVQnCqSLC/S7Xfa8 jAll3O7iqOTwh+euS6Hl1/57pZYarpiJo8+lf1LwbKw8zL0UuQBg6xSU67EydTDO qCSTRPBule39mOoJ6z58zehOy1b8vnlSiEKdtgPuTSm+sMVmawg8oX1/lIlPHqPN lnsvfZ1lyHGaoNEVCYmcGFKiEE9sK99l3sD4tX6tRNB/u1Q9CsQ= =OpZh -----END PGP SIGNATURE----- --c5dN2dBZ8YcoQLbw--