Re: [PATCH 7/7] net/tcp: Don't store TCP-AO maclen on reqsk

[Dmitry Safonov <dima@arista.com>]

On 11/21/23 08:13, Eric Dumazet wrote:
> On Tue, Nov 21, 2023 at 3:01 AM Dmitry Safonov <dima@arista.com> wrote:
>>
>> This extra check doesn't work for a handshake when SYN segment has
>> (current_key.maclen != rnext_key.maclen). It could be amended to
>> preserve rnext_key.maclen instead of current_key.maclen, but that
>> requires a lookup on listen socket.
>>
>> Originally, this extra maclen check was introduced just because it was
>> cheap. Drop it and convert tcp_request_sock::maclen into boolean
>> tcp_request_sock::used_tcp_ao.
>>
>> Fixes: 06b22ef29591 ("net/tcp: Wire TCP-AO to request sockets")
>> Signed-off-by: Dmitry Safonov <dima@arista.com>
>> ---
>>  include/linux/tcp.h   | 10 ++++------
>>  net/ipv4/tcp_ao.c     |  4 ++--
>>  net/ipv4/tcp_input.c  |  5 +++--
>>  net/ipv4/tcp_output.c |  9 +++------
>>  4 files changed, 12 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>> index 68f3d315d2e1..3af897b00920 100644
>> --- a/include/linux/tcp.h
>> +++ b/include/linux/tcp.h
>> @@ -155,6 +155,9 @@ struct tcp_request_sock {
>>         bool                            req_usec_ts;
>>  #if IS_ENABLED(CONFIG_MPTCP)
>>         bool                            drop_req;
>> +#endif
>> +#ifdef CONFIG_TCP_AO
>> +       bool                            used_tcp_ao;
> 
> Why adding another 8bit field here and creating a hole ?

Sorry about it, it seems that I

(1) checked with CONFIG_MPTCP=n and it seemed like a hole
(2) was planning to unify it with other booleans under bitfileds
(3) found that some bitfileds require protection as set not only
    on initialization, so in the end it either should be flags+set_bit()
    or something well-thought on (that separate bitfileds won't be
    able to change at the same time)
(4) decided to focus on fixing the issue, rather than doing 2 things
    with the same patch

Will fix it up for v2, thanks!

> 
>>  #endif
>>         u32                             txhash;
>>         u32                             rcv_isn;
>> @@ -169,7 +172,6 @@ struct tcp_request_sock {
>>  #ifdef CONFIG_TCP_AO
>>         u8                              ao_keyid;
>>         u8                              ao_rcv_next;
>> -       u8                              maclen;
> 
> Just rename maclen here to  used_tcp_ao ?
> 
>>  #endif
>>  };
>>

Thanks,
             Dmitry