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

[Simon Horman <simon.horman@corigine.com>]

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?

...

> @@ -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?

...