Re: [PATCH v10 net-next 10/23] net/tcp: Wire TCP-AO to request sockets

[Dmitry Safonov]

On 8/16/23 20:45, Simon Horman wrote:
> On Tue, Aug 15, 2023 at 08:14:39PM +0100, Dmitry Safonov wrote:
[..]
>> @@ -1194,9 +1198,51 @@ static void tcp_v6_timewait_ack(struct sock *sk, struct sk_buff *skb)
>>  static void tcp_v6_reqsk_send_ack(const struct sock *sk, struct sk_buff *skb,
>>  				  struct request_sock *req)
>>  {
>> +	struct tcp_md5sig_key *md5_key = NULL;
>> +	const struct in6_addr *addr;
>> +	u8 *traffic_key = NULL;
>>  	int l3index;
>>  
>>  	l3index = tcp_v6_sdif(skb) ? tcp_v6_iif_l3_slave(skb) : 0;
>> +	addr = &ipv6_hdr(skb)->saddr;
>> +
>> +	if (tcp_rsk_used_ao(req)) {
>> +#ifdef CONFIG_TCP_AO
>> +		struct tcp_ao_key *ao_key = NULL;
>> +		const struct tcp_ao_hdr *aoh;
>> +		u8 keyid = 0;
> 
> Hi Dmitry,
> 
> keyid is declared and initialised here.
> 
>> +
>> +		/* Invalid TCP option size or twice included auth */
>> +		if (tcp_parse_auth_options(tcp_hdr(skb), NULL, &aoh))
>> +			return;
>> +		if (!aoh)
>> +			return;
>> +		ao_key = tcp_v6_ao_do_lookup(sk, addr, aoh->rnext_keyid, -1);
>> +		if (unlikely(!ao_key)) {
>> +			/* Send ACK with any matching MKT for the peer */
>> +			ao_key = tcp_v6_ao_do_lookup(sk, addr, -1, -1);
>> +			/* Matching key disappeared (user removed the key?)
>> +			 * let the handshake timeout.
>> +			 */
>> +			if (!ao_key) {
>> +				net_info_ratelimited("TCP-AO key for (%pI6, %d)->(%pI6, %d) suddenly disappeared, won't ACK new connection\n",
>> +						     addr,
>> +						     ntohs(tcp_hdr(skb)->source),
>> +						     &ipv6_hdr(skb)->daddr,
>> +						     ntohs(tcp_hdr(skb)->dest));
>> +				return;
>> +			}
>> +		}
>> +		traffic_key = kmalloc(tcp_ao_digest_size(ao_key), GFP_ATOMIC);
>> +		if (!traffic_key)
>> +			return;
>> +
>> +		keyid = aoh->keyid;
> 
> And reinitialised here.
> But is otherwise unused.
> 
> Flagged in a W=1 build with both clang-16 and gcc-13.

Thanks, will be fixed in v11.

Seems to be another rebase-artifact.
I'll also take a look how didn't I notice this on tests, they probably
need a new selftest for this.

-- 
          Dmitry