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
next prev parent 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).