Re: [PATCH net v2] tcp: fix NULL pointer dereference when restarting a Fast Open listener

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch is meant to stop request_socks from being stranded in
  rskq_rst_head while they hold a listener reference, but another path…

--

tcp: fix NULL pointer dereference when restarting a Fast Open listener

This patch makes inet_csk_listen_stop() always take fastopenq.lock and
detach rskq_rst_head while holding it. That serializes the drain against
reqsk_fastopen_remove() adding entries to the RST list. The goal is to
stop request_socks from being stranded on rskq_rst_head while holding a
listener reference, and to avoid the NULL rskq_rst_tail dereference seen
when listen() is called again.

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index 37cc8606d6835..9ca59dca5d184 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -1533,17 +1533,15 @@ void inet_csk_listen_stop(struct sock *sk)

[ ... ]

> +	/* Free all the reqs queued in rskq_rst_head. */
> +	spin_lock_bh(&queue->fastopenq.lock);
> +	req = queue->fastopenq.rskq_rst_head;
> +	queue->fastopenq.rskq_rst_head = NULL;
> +	spin_unlock_bh(&queue->fastopenq.lock);
> +	while (req != NULL) {
> +		next = req->dl_next;
> +		reqsk_put(req);
> +		req = next;
>  	}
>  	WARN_ON_ONCE(sk->sk_ack_backlog);
>  }

[Severity: High]
Is this fix complete? The commit message says:

    With this synchronization, reqsk_fastopen_remove() either inserts the
    request_sock before inet_csk_listen_stop() acquires the lock, in which
    case inet_csk_listen_stop() reclaims it, or sees that the listener is
    not in TCP_LISTEN and does not insert anything.

That argument only holds if every exit from TCP_LISTEN goes through
inet_csk_listen_stop(). The failure path in inet_csk_listen_start()
does not:

net/ipv4/inet_connection_sock.c:inet_csk_listen_start() {
    ...
	reqsk_queue_alloc(&icsk->icsk_accept_queue);
    ...
	inet_sk_state_store(sk, TCP_LISTEN);
	err = sk->sk_prot->get_port(sk, inet->inet_num);
	if (!err) {
		...
		err = sk->sk_prot->hash(sk);

		if (likely(!err))
			return 0;
	}

	inet_sk_set_state(sk, TCP_CLOSE);
	return err;
}

Take a listener that went through shutdown(SHUT_RD) or connect(AF_UNSPEC)
while an accepted Fast Open child was still in SYN_RECV. Then listen() is
called on it again. If an RST for that child arrives after TCP_LISTEN is
stored, reqsk_fastopen_remove() passes this check under fastopenq->lock:

	if (!reset || lsk->sk_state != TCP_LISTEN) {

It then links req onto rskq_rst_head/rskq_rst_tail without calling
reqsk_put().

Now suppose get_port() fails afterwards, for example with EADDRINUSE
because another SO_REUSEADDR socket started listening on the port, or
hash() fails. The socket goes back to TCP_CLOSE and rskq_rst_head is never
drained.

A later close() will not reclaim the list, because __tcp_close() only
calls inet_csk_listen_stop() when sk_state == TCP_LISTEN. A later
successful listen() just overwrites the list in reqsk_queue_alloc():

	queue->fastopenq.rskq_rst_head = NULL;
	queue->fastopenq.rskq_rst_tail = NULL;

Does this leak the request_sock? If so, it also leaks the listener
reference taken in reqsk_alloc_noprof(), which only
sock_put(req->rsk_listener) in __reqsk_free() drops. That would pin the
listener sock and its netns reference too.

This looks like the same leak the commit message describes, reached
through a different TCP_LISTEN -> TCP_CLOSE transition.

Should the inet_csk_listen_start() error path also drain fastopenq under
fastopenq.lock? Alternatively, could the drain move to a place that covers
both paths?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/avMkfJrXhCFXn8M_%40v4bel