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

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

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

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.
> > > 
> > > 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>
> > > ---
> > >   checksum.c |   1 -
> > >   tcp.c      | 100 ++++++++++++++++++++++++++++++++++++++---------------
> > >   2 files changed, 72 insertions(+), 29 deletions(-)
> > > 
> > > 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 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
> > > + * @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
> > 
> > You explain it here, but "payload_offset" is a bit unclear if you're
> > not sure which layer it's talking about.  "l4offset" maybe?
> > 
> > > + */
> > > +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 = 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;
> > 
> > What's a __sum16?
> 
> It's the type of "check" in struct tcphdr.

Huh, ok.

> > > +	int check_idx;
> > > +	uint32_t sum;
> > > +
> > > +	sum = proto_ipv4_header_psum(iov_size(iov, iov_cnt) - payload_offset,
> > > +				     IPPROTO_TCP, src, dst);
> > > +
> > > +	check_idx = iov_skip_bytes(iov, iov_cnt,
> > > +				payload_offset + offsetof(struct tcphdr, check),
> > > +				&check_ofs);
> > > +
> > > +	check = (__sum16 *)((char *)iov[check_idx].iov_base + check_ofs);
> > 
> > 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.
> 
> I'm guessing iov_base and iov_len are 32bit aligned, and address of "check"
> 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 not
> 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.
> > 
> > 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.
> 
> 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.

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