[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d84888b2-8b5a-103c-3e8a-1be5e5833288@arista.com>
Date: Thu, 10 Aug 2023 17:26:52 +0100
From: Dmitry Safonov <dima@...sta.com>
To: Eric Dumazet <edumazet@...gle.com>
Cc: David Ahern <dsahern@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
linux-kernel@...r.kernel.org,
Andy Lutomirski <luto@...capital.net>,
Ard Biesheuvel <ardb@...nel.org>,
Bob Gilligan <gilligan@...sta.com>,
Dan Carpenter <error27@...il.com>,
David Laight <David.Laight@...lab.com>,
Dmitry Safonov <0x7f454c46@...il.com>,
Donald Cassidy <dcassidy@...hat.com>,
Eric Biggers <ebiggers@...nel.org>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
Francesco Ruggeri <fruggeri05@...il.com>,
"Gaillardetz, Dominik" <dgaillar@...na.com>,
Herbert Xu <herbert@...dor.apana.org.au>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
Ivan Delalande <colona@...sta.com>,
Leonard Crestez <cdleonard@...il.com>,
Salam Noureddine <noureddine@...sta.com>,
"Tetreault, Francois" <ftetreau@...na.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v9 net-next 16/23] net/tcp: Ignore specific ICMPs for
TCP-AO connections
On 8/8/23 14:43, Eric Dumazet wrote:
> On Wed, Aug 2, 2023 at 7:27 PM Dmitry Safonov <dima@...sta.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
Powered by blists - more mailing lists