Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <a838b389-4c0b-484d-98bd-33f8ef4c88f2@arista.com>
Date:   Wed, 22 Nov 2023 01:19:52 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 7/7] net/tcp: Don't store TCP-AO maclen on reqsk
Content-Language: en-US
To:     Eric Dumazet <edumazet@google.com>
Cc:     David Ahern <dsahern@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        Francesco Ruggeri <fruggeri05@gmail.com>,
        Salam Noureddine <noureddine@arista.com>,
        Simon Horman <horms@kernel.org>, netdev@vger.kernel.org
References: <20231121020111.1143180-1-dima@arista.com>
 <20231121020111.1143180-8-dima@arista.com>
 <CANn89iK-=G7p5CMuJDjioa7+ynZRrOOpd7bK3kPzxCXzygfFCQ@mail.gmail.com>
From:   Dmitry Safonov <dima@arista.com>
In-Reply-To: <CANn89iK-=G7p5CMuJDjioa7+ynZRrOOpd7bK3kPzxCXzygfFCQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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