public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v2 4/4] tcp: Update TCP checksum using an iovec array
Date: Mon, 30 Sep 2024 12:59:15 +1000	[thread overview]
Message-ID: <ZvoUA99erbbw0zZF@zatzit.fritz.box> (raw)
In-Reply-To: <1ccee20e-fbb5-47ab-8a87-939e7c81ed54@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 5122 bytes --]

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.
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > > 
> > > Notes:
> > >      v2:
> > >        - s/payload_offset/l4offset/
> > >        - check memory address of the checksum (alignment, iovec boundaries)
> > > 
> > >   checksum.c |   1 -
> > >   tcp.c      | 116 ++++++++++++++++++++++++++++++++++++++++-------------
> > >   2 files changed, 88 insertions(+), 29 deletions(-)
> > > 
> > > 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)
> > >   }
> > >   /**
> > > - * 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
> > 
> > Nit: s/IPv6/IPv4/
> > 
> > > + * @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 = ntohs(iph->tot_len) - sizeof(struct iphdr);
> > > -	struct in_addr saddr = { .s_addr = iph->saddr };
> > > -	struct in_addr daddr = { .s_addr = iph->daddr };
> > > -	uint32_t sum = proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, daddr);
> > > +	size_t check_ofs;
> > > +	__sum16 *check;
> > > +	int check_idx;
> > > +	uint32_t sum;
> > > +
> > > +	sum = proto_ipv4_header_psum(iov_size(iov, iov_cnt) - l4offset,
> > > +				     IPPROTO_TCP, src, dst);
> > 
> > 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.
> > 
> > > +
> > > +	check_idx = iov_skip_bytes(iov, iov_cnt,
> > > +				   l4offset + offsetof(struct tcphdr, check),
> > > +				   &check_ofs);
> > > +
> > > +	if (check_idx >= 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 = (__sum16 *)((char *)iov[check_idx].iov_base + check_ofs);
> > 
> > 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 *).
> > 
> > > -	bp->th.check = 0;
> > > -	bp->th.check = csum(bp, l4len, sum);
> > > +	if ((uintptr_t)check & (__alignof__(*check) - 1))
> > > +		die("TCP4 checksum field is not correctly aligned in memory");
> > 
> > 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.
> > 
> 
> 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.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

      reply	other threads:[~2024-09-30  4:24 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-25  8:11 [PATCH v2 0/4] tcp: use csum_iov() in tcp_update_check_tcp[4|6]() Laurent Vivier
2024-09-25  8:11 ` [PATCH v2 1/4] tcp: Use tcp_payload_t rather than tcphdr Laurent Vivier
2024-09-25  8:11 ` [PATCH v2 2/4] pcap: Add an offset argument in pcap_iov() Laurent Vivier
2024-09-25  8:11 ` [PATCH v2 3/4] checksum: Add an offset argument in csum_iov() Laurent Vivier
2024-09-25 17:39   ` Stefano Brivio
2024-09-26  1:31   ` David Gibson
2024-09-25  8:11 ` [PATCH v2 4/4] tcp: Update TCP checksum using an iovec array Laurent Vivier
2024-09-25 17:39   ` Stefano Brivio
2024-09-26  1:56     ` David Gibson
2024-09-26  3:54       ` Stefano Brivio
2024-09-26  1:45   ` David Gibson
2024-09-27 13:49     ` Laurent Vivier
2024-09-30  2:59       ` David Gibson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZvoUA99erbbw0zZF@zatzit.fritz.box \
    --to=david@gibson.dropbear.id.au \
    --cc=lvivier@redhat.com \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).