On 9/12/23 17:47, Eric Dumazet wrote:

On Mon, Sep 11, 2023 at 11:04 PM Dmitry Safonov [off-list ref] wrote:

[..]

quoted

@@ -1378,6 +1430,34 @@ static int __tcp_transmit_skb(struct sock *sk, struct sk_buff *skb,
                                               md5, sk, skb);
        }
 #endif
+#ifdef CONFIG_TCP_AO
+       if (ao) {
+               u8 *traffic_key;
+               void *tkey_buf = NULL;
+               __be32 disn;
+
+               sk_gso_disable(sk);

Why is this needed here in a fast path ? This should happen once.

It seems you copied MD5 old logic, I think we should do better.

Yeah, I think it survived from TCP-AO PoC where it was mostly TCP-MD5
copy'n'paste. And survived by the reason that it it's small and seems
yet done for TCP-MD5.

Which doesn't mean it can't be better. The MD5 code seems to have been
introduced in:
https://marc.info/?l=linux-netdev&m=121445989106145&w=2

Currently, the described child sk issue can't happen as tcp_md5sig_info
is allocated in tcp_md5sig_info_add() regardless if it is setsockopt()
or child socket allocation. And tcp_md5sig_info_add() does
sk_gso_disable(sk).

I presume, sk_gso_disable() can be removed from both TCP-AO/TCP-MD5.
The only exception will be twsk, where it doesn't seem to matter.

quoted

+               if (unlikely(tcb->tcp_flags & TCPHDR_SYN)) {
+                       if (tcb->tcp_flags & TCPHDR_ACK)
+                               disn = ao->risn;
+                       else
+                               disn = 0;
+
+                       tkey_buf = kmalloc(tcp_ao_digest_size(ao_key), GFP_ATOMIC);
+                       if (!tkey_buf)
+                               return -ENOMEM;

This leaks an skb.

Ouch! Thanks for noticing, will repair.

quoted

+                       traffic_key = tkey_buf;
+                       tp->af_specific->ao_calc_key_sk(ao_key, traffic_key,
+                                                       sk, ao->lisn, disn, true);
+               } else {
+                       traffic_key = snd_other_key(ao_key);
+               }
+               tp->af_specific->calc_ao_hash(opts.hash_location, ao_key, sk, skb,
+                                             traffic_key,
+                                             opts.hash_location - (u8 *)th, 0);
+               kfree(tkey_buf);
+       }
+#endif

        /* BPF prog is the last one writing header option */
        bpf_skops_write_hdr_opt(sk, skb, NULL, NULL, 0, &opts);

[..]

quoted

@@ -1837,8 +1921,17 @@ unsigned int tcp_current_mss(struct sock *sk)
                if (mtu != inet_csk(sk)->icsk_pmtu_cookie)
                        mss_now = tcp_sync_mss(sk, mtu);
        }
-
-       header_len = tcp_established_options(sk, NULL, &opts, &md5) +
+#ifdef CONFIG_TCP_AO
+       ao_info = rcu_dereference_check(tp->ao_info, lockdep_sock_is_held(sk));
+       if (ao_info)
+               /* TODO: verify if we can access current_key or we need to pass
+                * it from every caller of tcp_current_mss instead. The reason
+                * is that the current_key pointer can change asynchronously
+                * from the rx path.
+                */
+               ao_key = READ_ONCE(ao_info->current_key);
+#endif
+       header_len = tcp_established_options(sk, NULL, &opts, &md5, ao_key) +
                     sizeof(struct tcphdr);

Adding yet another argument in TCP fast path routines is very unfortunate...

Could we merge md5/ao_key, and have a field giving the type ?

Hmm, I'll try to refactor this a little.

Thanks for taking a look,
          Dmitry