Re: [PATCH v9 net-next 16/23] net/tcp: Ignore specific ICMPs for TCP-AO connections

[Dmitry Safonov]

On 8/8/23 14:43, Eric Dumazet wrote:
> On Wed, Aug 2, 2023 at 7:27 PM Dmitry Safonov <dima@xxxxxxxxxx> wrote:
[..]
>>
>> +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)

>> +{
>> +       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.

>> +       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.

[..]
>> +++ 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.

>> +       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