Re: [PATCH v3 9/9] tcp: Introduce ipv4_fill_headers()/ipv6_fill_headers()

[Stefano Brivio <sbrivio@redhat.com>]

On Sat, 17 Feb 2024 16:07:25 +0100
Laurent Vivier <lvivier@redhat.com> 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:
>     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 aa03c20712f6..bc57a4f6e611 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1324,6 +1324,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
> + * @c:		Execution context
> + * @conn:	Connection pointer
> + * @iph:	Pointer to IPv4 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: IP frame length including L2 headers, host order
> + */
> +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);
> +
> +	iph->tot_len = htons(ip_len);
> +	iph->saddr = a4->s_addr;

The reasoning behind the fact that a4 isn't NULL here is relatively
simple to follow: you already check for inany_v4(&conn->faddr) in the
caller, if it evaluates to true, you call this.

Still, it's a bit too convoluted for Coverity's taste. Could you
perhaps add an ASSERT(a4) before this block to make it obvious? It's a
bit annoying that we extract the address twice, but I don't see a much
better alternative compared to what you did.

-- 
Stefano