Re: [PATCH net-next v2] net: mptcp, Fast Open Mechanism

[Dmytro SHYTYI <dmytro@shytyi.net>]

Hello Paolo,

On 18/01/2022 11:02, Paolo Abeni wrote:
> On Mon, 2022-01-17 at 21:48 +0000, Dmytro SHYTYI wrote:
>> Hi Paolo,
>>
>> AFAIK, TFO option requires more that 8 bytes, if cookie len is 8 bytes:
>>
>> In file /net/ipv4/tcp_output.c (line 689):
>>
>> static void tcp_options_write(__be32 *ptr, struct tcp_sock *tp,
>>                                 struct tcp_out_options *opts)
>> ....
>> {
>>           if (unlikely(OPTION_FAST_OPEN_COOKIE & options)) {
>>                   struct tcp_fastopen_cookie *foc = opts->fastopen_cookie;
>>                   u8 *p = (u8 *)ptr;
>>                   u32 len; /* Fast Open option length */
>>
>>                   if (foc->exp) {
>>                           len = TCPOLEN_EXP_FASTOPEN_BASE + foc->len;
>>                           *ptr = htonl((TCPOPT_EXP << 24) | (len << 16) |
>>                                        TCPOPT_FASTOPEN_MAGIC);
>>                           p += TCPOLEN_EXP_FASTOPEN_BASE;
>>                   } else {
>>                           len = TCPOLEN_FASTOPEN_BASE + foc->len;
>>                           *p++ = TCPOPT_FASTOPEN;
>>                           *p++ = len;
>>                   }
>>
>>                   memcpy(p, foc->val, foc->len);
>>                   if ((len & 3) == 2) {
>>                           p[foc->len] = TCPOPT_NOP;
>>                           p[foc->len + 1] = TCPOPT_NOP;
>>                   }
>>                   ptr += (len + 3) >> 2;
>>           }
>> ...
>>
>> Here we do have  "len = TCPOLEN_FASTOPEN_BASE + foc->len" that is
>> slightly more than 8 bytes.
>>
>> This is the cause of introducing 4 byte cookie for MPTCP implementation.
>>
>> The RFC 8684 B.1. says:
>>     This is done with the TCP cookie request/response
>>      options, of 2 bytes and 6-18 bytes, respectively (depending on the
>>      chosen cookie length).
>>
>> So in my understanding, the "selected cookie size +
>> TCPOLEN_FASTOPEN_BASE" = 6, is not prohibited by, and in the allowed
>> range of the RFC 8684.
> 
> You are right, I underlooked the TFO option writing.
> 
>> As you suggested I will document the size of the cookie, if there is no
>> objections to above information :)
> 
> Shrinking the TFO cookie size has both security and functional impact:
> 
> - we do not want to change the TFO cookie size for TCP connection in
> MPTCP enabled build.
> - dynamically selecting the TFO cookie size depending on the specific
> rsk type will add more complexity to the TCP code.

I agree with functional and future security impact in case if we reduce 
the cookie size.

> rfc8684 suggests we could remove the TS option from the synack packet.
> I think it's worthy to try such idea.
> 
> To do that, we need to change the 'mptcp_synack_options()' hook
> signature, to accept a 'struct tcp_out_options' as the 3rd argument, so
> that mptcp_synack_options() can clear the TCPOLEN_TSTAMP_ALIGNED bit
> when needed.

With this we can try to keep "cookie len == 8". It will simplify things. 
I will look into it.

> Thanks
> 
> Paolo
> 

Thanks,
Dmytro.