From: David Gibson <david@gibson.dropbear.id.au>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()
Date: Fri, 1 Mar 2024 10:54:00 +1100 [thread overview]
Message-ID: <ZeEZGCWY2xusLPPR@zatzit> (raw)
In-Reply-To: <20240229165955.829476-9-lvivier@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 7713 bytes --]
On Thu, Feb 29, 2024 at 05:59:55PM +0100, Laurent Vivier wrote:
> Replace the macro SET_TCP_HEADER_COMMON_V4_V6() by a new function
> tcp_fill_header().
>
> Move IPv4 and IPv6 code from tcp_l2_buf_fill_headers() to
> tcp_fill_ipv4_header() and tcp_fill_ipv6_header()
>
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>
> Notes:
> v4:
> - group all the ip6g initialisations together and
> remove flow_lbl preset to 0
> - add ASSERT(a4)
>
> v3:
> - add to sub-series part 1
>
> v2:
> - extract header filling functions from
> "tcp: extract buffer management from tcp_send_flag()"
> - rename them tcp_fill_flag_header()/tcp_fill_ipv4_header(),
> tcp_fill_ipv6_header()
> - use upside-down Christmas tree arguments order
> - replace (void *) by (struct tcphdr *)
>
> tcp.c | 154 +++++++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 104 insertions(+), 50 deletions(-)
>
> diff --git a/tcp.c b/tcp.c
> index 5b2fdf662a6c..ced22534a103 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1326,6 +1326,108 @@ void tcp_defer_handler(struct ctx *c)
> tcp_l2_data_buf_flush(c);
> }
>
> +/**
> + * tcp_fill_header() - Fill the TCP header fields for a given TCP segment.
> + *
> + * @th: Pointer to the TCP header structure
> + * @conn: Pointer to the TCP connection structure
> + * @seq: Sequence number
> + */
> +static void tcp_fill_header(struct tcphdr *th,
> + const struct tcp_tap_conn *conn, uint32_t seq)
> +{
> + th->source = htons(conn->fport);
> + th->dest = htons(conn->eport);
> + th->seq = htonl(seq);
> + th->ack_seq = htonl(conn->seq_ack_to_tap);
> + if (conn->events & ESTABLISHED) {
> + th->window = htons(conn->wnd_to_tap);
> + } else {
> + unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap;
> +
> + th->window = htons(MIN(wnd, USHRT_MAX));
> + }
> +}
> +
> +/**
> + * tcp_fill_ipv4_header() - Fill 802.3, IPv4, TCP headers in pre-cooked buffers
I don't love the name, since it does also fill the TCP header. Maybe
'tcp_fill_headers4()'?
> + * @c: Execution context
> + * @conn: Connection pointer
> + * @iph: Pointer to IPv4 header, immediately followed by a TCP header
Again, really don't love accessing beyond a given pointer's type.
> + * @plen: Payload length (including TCP header options)
> + * @check: Checksum, if already known
> + * @seq: Sequence number for this segment
> + *
> + * Return: IP frame length including L2 headers, host order
AFAICT the return value does *not* include the L2 headers..
> + */
> +static size_t tcp_fill_ipv4_header(const struct ctx *c,
> + const struct tcp_tap_conn *conn,
> + struct iphdr *iph, size_t plen,
> + const uint16_t *check, uint32_t seq)
> +{
> + size_t ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> + const struct in_addr *a4 = inany_v4(&conn->faddr);
> + struct tcphdr *th = (struct tcphdr *)(iph + 1);
> +
> + ASSERT(a4);
> +
> + iph->tot_len = htons(ip_len);
> + iph->saddr = a4->s_addr;
> + iph->daddr = c->ip4.addr_seen.s_addr;
> +
> + iph->check = check ? *check :
> + csum_ip4_header(iph->tot_len, IPPROTO_TCP,
> + iph->saddr, iph->daddr);
> +
> +
> + tcp_fill_header(th, conn, seq);
> +
> + tcp_update_check_tcp4(iph);
It's a bit ugly that tcp_fill_header() fills the TCP header, but *not*
the checksum. Could we handle this by passing the pseudo-header psum
into tcp_fill_header()? Then the logic for that in
tcp_update_check_tcp4() would become part of this function.
> + return ip_len;
> +}
> +
> +/**
> + * tcp_fill_ipv6_header() - Fill 802.3, IPv6, TCP headers in pre-cooked buffers
> + * @c: Execution context
> + * @conn: Connection pointer
> + * @ip6h: Pointer to IPv6 header, immediately followed by a TCP header
> + * @plen: Payload length (including TCP header options)
> + * @check: Checksum, if already known
> + * @seq: Sequence number for this segment
> + *
> + * Return: The total length of the IPv6 packet, host order
> + */
> +static size_t tcp_fill_ipv6_header(const struct ctx *c,
> + const struct tcp_tap_conn *conn,
> + struct ipv6hdr *ip6h, size_t plen,
> + uint32_t seq)
> +{
> + size_t ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> + struct tcphdr *th = (struct tcphdr *)(ip6h + 1);
> +
> + ip6h->payload_len = htons(plen + sizeof(struct tcphdr));
> + ip6h->saddr = conn->faddr.a6;
> + if (IN6_IS_ADDR_LINKLOCAL(&ip6h->saddr))
> + ip6h->daddr = c->ip6.addr_ll_seen;
> + else
> + ip6h->daddr = c->ip6.addr_seen;
> +
> + ip6h->hop_limit = 255;
> + ip6h->version = 6;
> + ip6h->nexthdr = IPPROTO_TCP;
> +
> + ip6h->flow_lbl[0] = (conn->sock >> 16) & 0xf;
> + ip6h->flow_lbl[1] = (conn->sock >> 8) & 0xff;
> + ip6h->flow_lbl[2] = (conn->sock >> 0) & 0xff;
> +
> + tcp_fill_header(th, conn, seq);
> +
> + tcp_update_check_tcp6(ip6h);
> +
> + return ip_len;
> +}
> +
> /**
> * tcp_l2_buf_fill_headers() - Fill 802.3, IP, TCP headers in pre-cooked buffers
> * @c: Execution context
> @@ -1345,67 +1447,19 @@ static size_t tcp_l2_buf_fill_headers(const struct ctx *c,
> const struct in_addr *a4 = inany_v4(&conn->faddr);
> size_t ip_len, tlen;
>
> -#define SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq) \
> -do { \
> - b->th.source = htons(conn->fport); \
> - b->th.dest = htons(conn->eport); \
> - b->th.seq = htonl(seq); \
> - b->th.ack_seq = htonl(conn->seq_ack_to_tap); \
> - if (conn->events & ESTABLISHED) { \
> - b->th.window = htons(conn->wnd_to_tap); \
> - } else { \
> - unsigned wnd = conn->wnd_to_tap << conn->ws_to_tap; \
> - \
> - b->th.window = htons(MIN(wnd, USHRT_MAX)); \
> - } \
> -} while (0)
> -
> if (a4) {
> struct tcp4_l2_buf_t *b = (struct tcp4_l2_buf_t *)p;
>
> - ip_len = plen + sizeof(struct iphdr) + sizeof(struct tcphdr);
> - b->iph.tot_len = htons(ip_len);
> - b->iph.saddr = a4->s_addr;
> - b->iph.daddr = c->ip4.addr_seen.s_addr;
> -
> - b->iph.check = check ? *check :
> - csum_ip4_header(b->iph.tot_len, IPPROTO_TCP,
> - b->iph.saddr, b->iph.daddr);
> -
> - SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> - tcp_update_check_tcp4(&b->iph);
> + ip_len = tcp_fill_ipv4_header(c, conn, &b->iph, plen, check, seq);
>
> tlen = tap_iov_len(c, &b->taph, ip_len);
> } else {
> struct tcp6_l2_buf_t *b = (struct tcp6_l2_buf_t *)p;
>
> - ip_len = plen + sizeof(struct ipv6hdr) + sizeof(struct tcphdr);
> -
> - b->ip6h.payload_len = htons(plen + sizeof(struct tcphdr));
> - b->ip6h.saddr = conn->faddr.a6;
> - if (IN6_IS_ADDR_LINKLOCAL(&b->ip6h.saddr))
> - b->ip6h.daddr = c->ip6.addr_ll_seen;
> - else
> - b->ip6h.daddr = c->ip6.addr_seen;
> -
> - memset(b->ip6h.flow_lbl, 0, 3);
> -
> - SET_TCP_HEADER_COMMON_V4_V6(b, conn, seq);
> -
> - tcp_update_check_tcp6(&b->ip6h);
> -
> - b->ip6h.hop_limit = 255;
> - b->ip6h.version = 6;
> - b->ip6h.nexthdr = IPPROTO_TCP;
> -
> - b->ip6h.flow_lbl[0] = (conn->sock >> 16) & 0xf;
> - b->ip6h.flow_lbl[1] = (conn->sock >> 8) & 0xff;
> - b->ip6h.flow_lbl[2] = (conn->sock >> 0) & 0xff;
> + ip_len = tcp_fill_ipv6_header(c, conn, &b->ip6h, plen, seq);
>
> tlen = tap_iov_len(c, &b->taph, ip_len);
> }
> -#undef SET_TCP_HEADER_COMMON_V4_V6
>
> return tlen;
> }
--
David Gibson | 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 --]
next prev parent reply other threads:[~2024-02-29 23:54 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-29 16:59 [PATCH v4 0/8] Add vhost-user support to passt (part 1) Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 1/8] pcap: add pcap_iov() Laurent Vivier
2024-02-29 23:16 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 2/8] checksum: align buffers Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 3/8] checksum: add csum_iov() Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 4/8] util: move IP stuff from util.[ch] to ip.[ch] Laurent Vivier
2024-02-29 23:19 ` David Gibson
2024-03-01 12:06 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c Laurent Vivier
2024-02-29 23:25 ` David Gibson
2024-03-01 12:31 ` Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 6/8] checksum: introduce functions to compute the header part checksum for TCP/UDP Laurent Vivier
2024-02-29 23:42 ` David Gibson
2024-02-29 16:59 ` [PATCH v4 7/8] tap: make tap_update_mac() generic Laurent Vivier
2024-02-29 16:59 ` [PATCH v4 8/8] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers() Laurent Vivier
2024-02-29 23:54 ` David Gibson [this message]
2024-03-03 13:50 ` Laurent Vivier
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=ZeEZGCWY2xusLPPR@zatzit \
--to=david@gibson.dropbear.id.au \
--cc=lvivier@redhat.com \
--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).