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=LmcbbVu6; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id D170A5A004F for ; Thu, 26 Sep 2024 05:42:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1727322125; bh=7pziDfYX8wwM1m+4n9YjhhLUzVc591NeTCWjnFYbuGw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LmcbbVu6WM7AZIKr85hO0PJ5s22H/0jmZ04AH/zlXKQXUdVFehSA9RrwCyX3hp+Ye XAwqy0Jysb14Gy8zaTtXoEr9R1qMl/ZgL/eo6gH7/kd4MO8CFmKdRAr9YE6B/BlFq+ cFaLz/pXVy/BSAcfRbI/sPzziSklcMhKwpA4tiulmkxlglJqRENxytfGP7mP1uVu++ 6IztiKxhrbzeAmHJ83vj7nn7F3s6WC08PxDo1uB1u4T0n5edyzKvUjFTukF/VddHnY 6wh45qYuvBUoyzM5k0AmMzYRVmpo87eT6xPDToXFJ9fvo2suqcslGHjGEe0+njKYEd ptLwGpIjGUuYA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XDfY90L6Qz4xQf; Thu, 26 Sep 2024 13:42:05 +1000 (AEST) Date: Thu, 26 Sep 2024 11:56:49 +1000 From: David Gibson To: Stefano Brivio 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> <20240925193919.6bfe0df4@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="X/vBkFPruyu1AYny" Content-Disposition: inline In-Reply-To: <20240925193919.6bfe0df4@elisabeth> Message-ID-Hash: 42VT2DNOVHFEDD3PS4GH7O2EZDVN57P6 X-Message-ID-Hash: 42VT2DNOVHFEDD3PS4GH7O2EZDVN57P6 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: --X/vBkFPruyu1AYny Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Sep 25, 2024 at 07:39:19PM +0200, Stefano Brivio wrote: > On Wed, 25 Sep 2024 10:11:25 +0200 > Laurent Vivier wrote: >=20 > > 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 boundari= es) > >=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 ctx= *c, int s) > > } > > =20 > > /** > > - * 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 > > + * @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, da= ddr); > > + 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); > > + > > + 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"); >=20 > I'm not really fond of those die() calls. First off, they should report > a couple more details (at least check_idx, iov_cnt). >=20 > Second, we could fail gracefully (hence, we should) instead of aborting > the whole thing: those could be err() calls. It's a question of how plausible a graceful recovery is at this point. If we hit this, the guest has given us buffers that aren't even 2-byte aligned. Is it reasonable to keep trying to working with a guest that does that? > If we fail to calculate checksums, we can leave them as zero, and the > receiver will drop those frames anyway, if you don't want to add > complexity (propagation of return values) for something that should > never happen. I think the checksum is (in the general vhost-user case) uninitialised, not zero, at this point. To set it to zero, we'd still need to get a usable pointer to it. Not that that really changes what would happen - 0 has the same chance of being right by accident as an uninitialised value. > The rest looks good to me, but I didn't go through David's comments so > I'll wait for his review before applying. --=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 --X/vBkFPruyu1AYny Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmb0v2AACgkQzQJF27ox 2Gch/Q/6A5lsTs5pCbLbHTjYrAyx4SfVEy16ya8XalRarRFT+iyo0fVT7aIQaEVV fWsHc65OGgTw6ZZe94wH+xrLwsLXtrO4qxHrsJS4XSaiPsnpUrX2sHm1m6Y/zaWK Klonq4HXKfzlcWezQkoqRIojSM6hIkq4ppeikLb52znkrWsBIZrV+K+HjBVdiNKr 8sKgyF/KuRnpgDszXt1mukgPC6BpSsS4NTVHLIEKzwdjdygmVAoKU00yIC17gBgz Eud6K5gf+o5VCB9RAsVuWfEwZkTTkaSEntyNOqO/QvbrmJmhnBgQVQHQd3B2ml9P yqNokk/oRcsVP/jVQiZ5tHThWNbKIAg818FYrw+sT+4OsUJjkBS1pl4Rhe3wDxZ9 tW2AIgg/ePn0xu2kg7T4Iwlhz4Q3doZQjBcByS4wJ6fl9h6oVTanft74JNvBXFVP ZyxgBBf/xDsZmeKwsKchPd1qZQKVMM0v4tbQ7KCfes5VRvV8ijotRE/Q/+yPwAk2 1/Dwl8A/cdgzaN0dXqIGrGpduBr1HkeVXixfoyEDojsvgw6qN+vnngY8VCOslvrY t4o2evie+squIjX39uSTThMNvD1Ff7ZhIIAkEiS1OIYjlCqn10lHBFcj4lMAGb6G 3NogVTi/N9q++oZmNZ2Gvw0DzT8DvbmkevNhyCar8eyABmgBoIg= =vlp9 -----END PGP SIGNATURE----- --X/vBkFPruyu1AYny--