Messages in this thread | | | Date | Tue, 25 Jul 2023 20:10:21 +0100 | Subject | Re: [PATCH v8.1 net-next 06/23] net/tcp: Add TCP-AO sign to outgoing packets | From | Dmitry Safonov <> |
| |
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
| |