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

[Laurent Vivier <lvivier@redhat.com>]

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.

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

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

I don't want to write complicated code only to write the checksum of the tcp header.

Thanks,
Laurent