Re: [PATCH v8.1 net-next 06/23] net/tcp: Add TCP-AO sign to outgoing packets

[Dmitry Safonov <dima@arista.com>]

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