Re: [PATCH v14 net-next 08/23] net/tcp: Add AO sign to RST packets

[Dmitry Safonov <dima@arista.com>]

On 10/11/23 19:04, Eric Dumazet wrote:
> On Tue, Oct 10, 2023 at 1:07 AM Dmitry Safonov <dima@arista.com> wrote:
[..]
>>  struct tcp_key {
>>         union {
>> -               struct tcp_ao_key *ao_key;
>> +               struct {
>> +                       struct tcp_ao_key *ao_key;
>> +                       u32 sne;
>> +                       char *traffic_key;
> 
> Move sne after traffic_key to avoid a hole on 64bit arches.

Sure, thanks!

[..]
>> @@ -435,6 +495,46 @@ struct tcp_ao_key *tcp_v4_ao_lookup(const struct sock *sk, struct sock *addr_sk,
>>         return tcp_ao_do_lookup(sk, addr, AF_INET, sndid, rcvid);
>>  }
>>
>> +int tcp_ao_prepare_reset(const struct sock *sk, struct sk_buff *skb,
>> +                        const struct tcp_ao_hdr *aoh, int l3index,
>> +                        struct tcp_ao_key **key, char **traffic_key,
>> +                        bool *allocated_traffic_key, u8 *keyid, u32 *sne)
>> +{
>> +       struct tcp_ao_info *ao_info;
>> +
>> +       *allocated_traffic_key = false;
>> +       /* If there's no socket - than initial sisn/disn are unknown.
>> +        * Drop the segment. RFC5925 (7.7) advises to require graceful
>> +        * restart [RFC4724]. Alternatively, the RFC5925 advises to
>> +        * save/restore traffic keys before/after reboot.
>> +        * Linux TCP-AO support provides TCP_AO_ADD_KEY and TCP_AO_REPAIR
>> +        * options to restore a socket post-reboot.
>> +        */
>> +       if (!sk)
>> +               return -ENOTCONN;
>> +
>> +       if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_NEW_SYN_RECV)) {
>> +               return -1;
>> +       } else {
>> +               struct tcp_ao_key *rnext_key;
>> +
>> +               if (sk->sk_state == TCP_TIME_WAIT)
> 
> Why not adding TCPF_TIME_WAIT in the prior test ?

Well, twsk is wired up here in the next patch "net/tcp: Add TCP-AO sign
to twsk", while the condition for request sockets is the patch after that.
I probably can move it to the same condition just to polish the code in
the middle of patch set, I guess it felt not critical when I split it by
patches.

> 
>> +                       return -1;

[..]

Thanks,
           Dmitry