Re: [PATCH net v3] tcp: reset late connection after listening socket close

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
tcp: reset late connection after listening socket close

When __inet_inherit_port() returns -ENOENT because the listener already
released its bound port, the child socket was dropped silently and the peer
could hang forever. This patch adds an -ENOENT case to both
tcp_v{4,6}_syn_recv_sock() that sends an RST with the listening socket
before dropping the child.

Link: https://lore.kernel.org/87sf0ldk41.fsf@urd.knielsen-hq.org # [1]

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index b8887cdd66c5..9a14c2e56ec3 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c

[ ... ]

> @@ -1756,8 +1757,12 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
>  		goto put_and_exit; /* OOM, release back memory */
>  #endif
>  
> -	if (__inet_inherit_port(sk, newsk) < 0)
> +	ret = __inet_inherit_port(sk, newsk);
> +	if (unlikely(ret < 0)) {
> +		if (ret == -ENOENT)
> +			goto send_reset_and_exit;
>  		goto put_and_exit;
> +	}
>  	*own_req = inet_ehash_nolisten(newsk, req_to_sk(req_unhash),
>  				       &found_dup_sk);
>  	if (likely(*own_req)) {

[ ... ]

> @@ -1784,6 +1789,8 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
>  exit:
>  	tcp_listendrop(sk);
>  	return NULL;
> +send_reset_and_exit:
> +	tcp_v4_send_reset(sk, skb, SK_RST_REASON_TCP_STATE);

[Severity: High]
Can this oops when the listener has TCP-AO keys installed?

As the commit message explains, this label is reached because
tcp_set_state(sk, TCP_CLOSE) already ran inet_put_port(), so sk->sk_state
is normally TCP_CLOSE here, and the socket was never established.

tcp_v4_send_reset() then feeds that socket to the AO code:

net/ipv4/tcp_ipv4.c:tcp_v4_send_reset() {
	...
	if (aoh && tcp_v4_ao_sign_reset(sk, skb, aoh, &arg, &rep.th, rep.opt))
		return;
	...
}

and tcp_ao_prepare_reset() only special-cases listen/request sockets:

net/ipv4/tcp_ao.c:tcp_ao_prepare_reset() {
	...
	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_NEW_SYN_RECV)) {
	...
	} else {
		*key = tcp_ao_established_key(sk, ao_info, aoh->rnext_keyid, -1);
		if (!*key)
			return -ENOENT;
		*traffic_key = snd_other_key(*key);
		rnext_key = READ_ONCE(ao_info->rnext_key);
		*keyid = rnext_key->rcvid;
	...
}

TCP_CLOSE is not in that mask, so the else branch runs on a socket whose
ao_info->rnext_key was never set.  tcp_ao_add_cmd() deliberately skips it
for listen/close sockets:

net/ipv4/tcp_ao.c:tcp_ao_add_cmd() {
	...
	if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) {
		tcp_ao_cache_traffic_keys(sk, ao_info, key);
		if (first) {
			ao_info->current_key = key;
			ao_info->rnext_key = key;
		}
	}
	...
}

and include/net/tcp_ao.h documents "Never dereference any of them from
listen sockets".  Does rnext_key->rcvid then dereference NULL in softirq
context?

If rnext_key happens to be non-NULL, *traffic_key = snd_other_key(*key)
returns traffic key bytes that were never computed, since
tcp_ao_cache_traffic_keys() is skipped for LISTEN/CLOSE and
tcp_ao_key_alloc() uses sock_kmalloc() without zeroing.  Would the RST be
MACed with uninitialized memory and dropped by the peer?

In the narrow window where the listener is still TCP_LISTEN, the LISTEN
branch derives the key with sisn = th->seq and disn = 0, which only matches
a SYN, not the final handshake ACK that got us here.  Does that leave the
RST with an invalid MAC (or no RST at all when tcp_ao_do_lookup() fails and
tcp_v4_ao_sign_reset() returns early), so the hang this patch fixes remains
for TCP-AO connections while the child is still destroyed?

[Severity: Medium]
Is this path also reachable from TCP Fast Open, and if so, is the RST
correct there?

tcp_fastopen_create_child() calls into this function with the TFO SYN skb:

net/ipv4/tcp_fastopen.c:tcp_fastopen_create_child() {
	child = inet_csk(sk)->icsk_af_ops->syn_recv_sock(sk, skb, req, NULL,
							 NULL, &own_req, NULL);
	if (!child)
		return NULL;
	...
}

For that skb th->ack is 0, so tcp_v4_send_reset() emits RST+ACK
acknowledging the SYN.  The NULL return does not abort the handshake:
tcp_try_fastopen() returns NULL and tcp_conn_request() continues in the
else branch:

net/ipv4/tcp_input.c:tcp_conn_request() {
	} else {
		tcp_rsk(req)->tfo_listener = false;
		if (!want_cookie &&
		    unlikely(!inet_csk_reqsk_queue_hash_add(sk, req)))
			...
		af_ops->send_synack(sk, dst, &fl, req, &foc,
	...
}

Does that mean the same receive pass sends an RST and then a SYN-ACK for
the same 4-tuple, and leaves a request sock hashed on a listener that is
being torn down?  There is currently no way for this label to tell the
caller that the flow was already reset, so the fast open fallback cannot be
suppressed.

>  put_and_exit:
>  	newinet->inet_opt = NULL;
>  	inet_csk_prepare_forced_close(newsk);

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 9e9155b1b3aa..ecb0b405703c 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c

[ ... ]

> @@ -1547,6 +1552,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
>  exit:
>  	tcp_listendrop(sk);
>  	return NULL;
> +send_reset_and_exit:
> +	tcp_v6_send_reset(sk, skb, SK_RST_REASON_TCP_STATE);
>  put_and_exit:
>  	inet_csk_prepare_forced_close(newsk);
>  	tcp_done(newsk);

Both questions above apply here too: tcp_v6_send_reset() calls
tcp_ao_prepare_reset() with the same closed listener, and
tcp_fastopen_create_child() reaches tcp_v6_syn_recv_sock() through
icsk_af_ops->syn_recv_sock() as well.
-- 
pw-bot: cr