Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d84888b2-8b5a-103c-3e8a-1be5e5833288@arista.com>
Date:   Thu, 10 Aug 2023 17:26:52 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v9 net-next 16/23] net/tcp: Ignore specific ICMPs for
 TCP-AO connections
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,
        Andy Lutomirski <luto@amacapital.net>,
        Ard Biesheuvel <ardb@kernel.org>,
        Bob Gilligan <gilligan@arista.com>,
        Dan Carpenter <error27@gmail.com>,
        David Laight <David.Laight@aculab.com>,
        Dmitry Safonov <0x7f454c46@gmail.com>,
        Donald Cassidy <dcassidy@redhat.com>,
        Eric Biggers <ebiggers@kernel.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Francesco Ruggeri <fruggeri05@gmail.com>,
        "Gaillardetz, Dominik" <dgaillar@ciena.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        Ivan Delalande <colona@arista.com>,
        Leonard Crestez <cdleonard@gmail.com>,
        Salam Noureddine <noureddine@arista.com>,
        "Tetreault, Francois" <ftetreau@ciena.com>, netdev@vger.kernel.org
References: <20230802172654.1467777-1-dima@arista.com>
 <20230802172654.1467777-17-dima@arista.com>
 <CANn89iKjT3i-0rZLu8WE_P94aN65rj8uBAw3MyMPhsnMKWSs_A@mail.gmail.com>
From:   Dmitry Safonov <dima@arista.com>
In-Reply-To: <CANn89iKjT3i-0rZLu8WE_P94aN65rj8uBAw3MyMPhsnMKWSs_A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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