Re: [PATCH v2 4/4] tcp: Update TCP checksum using an iovec array

[David Gibson <david@gibson.dropbear.id.au>]

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