On 8/8/23 14:43, Eric Dumazet wrote:

On Wed, Aug 2, 2023 at 7:27 PM Dmitry Safonov [off-list ref] wrote:

[..]

quoted

+bool tcp_ao_ignore_icmp(struct sock *sk, int type, int code)

const struct sock *sk ?

Well, I can't really: atomic64_inc(&ao->counters.dropped_icmp)

quoted

+{
+       bool ignore_icmp = false;
+       struct tcp_ao_info *ao;
+
+       /* RFC5925, 7.8:
+        * >> A TCP-AO implementation MUST default to ignore incoming ICMPv4
+        * messages of Type 3 (destination unreachable), Codes 2-4 (protocol
+        * unreachable, port unreachable, and fragmentation needed -- ’hard
+        * errors’), and ICMPv6 Type 1 (destination unreachable), Code 1
+        * (administratively prohibited) and Code 4 (port unreachable) intended
+        * for connections in synchronized states (ESTABLISHED, FIN-WAIT-1, FIN-
+        * WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT) that match MKTs.
+        */

I know this sounds silly, but you should read sk->sk_family once.

Or risk another KCSAN report with IPV6_ADDRFORM

if (sk->sk_family == AF_INET) {
    ...
} else {
    /* AF_INET case */
}

Oh, I didn't know about IPV6_ADDRFORM. Sure, will read it once.

quoted

+       if (sk->sk_family == AF_INET) {
+               if (type != ICMP_DEST_UNREACH)
+                       return false;
+               if (code < ICMP_PROT_UNREACH || code > ICMP_FRAG_NEEDED)
+                       return false;
+       } else if (sk->sk_family == AF_INET6) {
+               if (type != ICMPV6_DEST_UNREACH)
+                       return false;
+               if (code != ICMPV6_ADM_PROHIBITED && code != ICMPV6_PORT_UNREACH)
+                       return false;
+       } else {

No WARN_ON_ONCE(1) here please.

Ok.

[..]

quoted

+++ b/net/ipv4/tcp_ipv4.c

@@ -494,6 +494,8 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
                return -ENOENT;
        }
        if (sk->sk_state == TCP_TIME_WAIT) {
+               /* To increase the counter of ignored icmps for TCP-AO */
+               tcp_ao_ignore_icmp(sk, type, code);
                inet_twsk_put(inet_twsk(sk));
                return 0;
        }

@@ -508,6 +510,9 @@ int tcp_v4_err(struct sk_buff *skb, u32 info)
        }

        bh_lock_sock(sk);

Do we need to hold the spinlock before calling tcp_ao_ignore_icmp() ?

I don't think so. And I think originally I've written it out of
bh_lock_sock(), but now I can't remember which paranoid thought resulted
in moving it under the lock. Anyway, will move it out again.

quoted

+       if (tcp_ao_ignore_icmp(sk, type, code))
+               goto out;
+
        /* If too many ICMPs get dropped on busy
         * servers this needs to be solved differently.
         * We do take care of PMTU discovery (RFC1191) special case :

[..]

Thanks,
           Dmitry