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

[Laurent Vivier <lvivier@redhat.com>]

On 13/06/2024 12:14, Stefano Brivio wrote:
> 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.
> 

I'm wondering if I can do:

+       seq = conn->seq_to_tap;
         ret = tcp_prepare_flags(c, conn, flags, &payload->th,
                                 payload->opts, &optlen);
         if (ret <= 0)
                 return ret;

         l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL,
-                                       conn->seq_to_tap);
+                                       seq);

to keep  the flags in tcp_prepare_flags()?

Thanks,
Laurent