[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b01a63a7-eaaa-85db-b04d-8270e82e1080@arista.com>
Date: Tue, 25 Jul 2023 20:10:21 +0100
From: Dmitry Safonov <dima@...sta.com>
To: Simon Horman <simon.horman@...igine.com>
Cc: David Ahern <dsahern@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
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 v8.1 net-next 06/23] net/tcp: Add TCP-AO sign to outgoing
packets
Hi Simon,
On 7/25/23 18:02, Simon Horman wrote:
> On Fri, Jul 21, 2023 at 05:18:57PM +0100, Dmitry Safonov wrote:
>
> ...
>
> Hi Dmitry,
>
>> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
>
> ...
>
>> @@ -619,7 +621,33 @@ static void tcp_options_write(struct tcphdr *th, struct tcp_sock *tp,
>> opts->hash_location = (__u8 *)ptr;
>> ptr += 4;
>> }
>> +#ifdef CONFIG_TCP_AO
>> + if (unlikely(OPTION_AO & options) && tp) {
>
> Smatch warns that here we check if tp is NULL,
> but later on in the same function (existing) code
> uses tp unconditionally.
>
> That code looks like this:
>
> if (unlikely(opts->num_sack_blocks)) {
> struct tcp_sack_block *sp = tp->rx_opt.dsack ?
> tp->duplicate_sack : tp->selective_acks;
>
> I would recommend running Smatch.
> It points out a lot of interesting things.
>
>
>> + struct tcp_ao_key *rnext_key;
>> + struct tcp_ao_info *ao_info;
>> + u8 maclen;
>>
>> + if (WARN_ON_ONCE(!ao_key))
>> + goto out_ao;
>> + ao_info = rcu_dereference_check(tp->ao_info,
>> + lockdep_sock_is_held(&tp->inet_conn.icsk_inet.sk));
>
> Checkpatch complains about indentation here.
>
> Rather than point out each case in the series,
> could I ask you to run ./scripts/checkpatch.pl --strict over the patchset?
Yeah, but then it won't fit 80 columns here. As both aren't hard
requirements I tend to comply with 80 columns more than to indentation.
In this particular case I'll check if it could be a helper function.
If it won't make sense to separate it as a helper, I'll just move it to
the same line than, breaking 80 columns limit.
>> @@ -1363,6 +1424,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;
>> + u32 disn;
>> +
>> + sk_gso_disable(sk);
>> + if (unlikely(tcb->tcp_flags & TCPHDR_SYN)) {
>> + if (tcb->tcp_flags & TCPHDR_ACK)
>> + disn = ao->risn;
>
> Sparse complains that there is an endian missmatch between disn and ao->risn ?
>
> Rather than point out every problem flagged by Sparse,
> could I ask you to run it over the series?
Yeah, I noticed it on netdev patchwork, running it over the patches now.
Thanks,
Dmitry
Powered by blists - more mailing lists