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

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.

Thanks,
Laurent


  reply	other threads:[~2024-09-27 13:49 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 [this message]
2024-09-30  2:59       ` David Gibson

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=1ccee20e-fbb5-47ab-8a87-939e7c81ed54@redhat.com \
    --to=lvivier@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --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).