Re: [PATCH v6 1/8] tcp: extract buffer management from tcp_send_flag()

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 13 Jun 2024 10:24:11 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> Hi,
> 
> I think the problem can be because tcp_l2_buf_fill_headers() has been moved out of 
> tcp_prepare_flags() and so moved after:
> 
>          if (th->fin || th->syn)
>                  conn->seq_to_tap++;
> 
> and con->seq_to_tap is also a parameter of tcp_l2_buf_fill_headers(). So it is increased 
> before and not after.
> 
> Could you try:
> 
> diff --git a/tcp.c b/tcp.c
> index 6800209d4122..647f42291fcf 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1607,6 +1607,7 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn *conn,
>          if (!tcp_update_seqack_wnd(c, conn, flags, &tinfo) && !flags)
>                  return 0;
> 
> +       *optlen = 0;
>          if (flags & SYN) {
>                  int mss;
> 
> @@ -1643,7 +1644,6 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn *conn,
>                  *data++ = conn->ws_to_tap;
>          } else if (!(flags & RST)) {
>                  flags |= ACK;
> -               *optlen = 0;
>          }
> 
>          th->doff = (sizeof(*th) + *optlen) / 4;
> @@ -1663,10 +1663,6 @@ static int tcp_prepare_flags(struct ctx *c, struct tcp_tap_conn *conn,
>          if (th->fin)
>                  conn_flag(c, conn, ACK_FROM_TAP_DUE);
> 
> -       /* RFC 793, 3.1: "[...] and the first data octet is ISN+1." */
> -       if (th->fin || th->syn)
> -               conn->seq_to_tap++;
> -
>          return 1;
>   }
> 
> @@ -1702,6 +1698,10 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, 
> int flags)
>                                          conn->seq_to_tap);
>          iov[TCP_IOV_PAYLOAD].iov_len = l4len;
> 
> +       /* RFC 793, 3.1: "[...] and the first data octet is ISN+1." */
> +       if (th->fin || th->syn)
> +               conn->seq_to_tap++;
> +
>          if (flags & DUP_ACK) {
>                  struct iovec *dup_iov;
>                  int i;

Ah, yes, good catch, I missed this one! It works. Note that it needs to
be:

	if ((flags & FIN) || (flags & SYN))
		...

because we don't have a TCP header there, yet.

-- 
Stefano