Re: [PATCH net] tcp: purge Fast Open RST queue when listen() fails

[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 fix is incomplete.

--

tcp: purge Fast Open RST queue when listen() fails

This patch moves the rskq_rst_head purge out of inet_csk_listen_stop()
into a new helper, reqsk_fastopen_purge_rst(). It also calls that helper
from the inet_csk_listen_start() error path, after the socket has been
set back to TCP_CLOSE. The goal is to reclaim Fast Open request_socks
that were queued while the socket was briefly in TCP_LISTEN.

Closes: https://lore.kernel.org/netdev/179450310167.2199510.18107423940014989443@kernel.org/

> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index ca3057192e6da..c33b0095468af 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c

[ ... ]

> @@ -1373,6 +1389,12 @@ int inet_csk_listen_start(struct sock *sk)
>  	}
>  
>  	inet_sk_set_state(sk, TCP_CLOSE);
> +
> +	/* A Fast Open child accepted before a previous shutdown() of this
> +	 * listener might have queued its request in rskq_rst_head while
> +	 * we were transiently in TCP_LISTEN state.
> +	 */
> +	reqsk_fastopen_purge_rst(&icsk->icsk_accept_queue);
>  	return err;
>  }

[Severity: High]
Does this error path also need to drain rskq_accept_head?

The commit message says the socket goes back to TCP_CLOSE "without going
through inet_csk_listen_stop()". This path only cleans up rskq_rst_head,
but the accept queue can be filled during the same TCP_LISTEN window.

After shutdown(SHUT_RD) or connect(AF_UNSPEC), inet_csk_listen_stop()
drains the accept queue. TCP_NEW_SYN_RECV request_socks still stay in
ehash with rsk_listener pointing at this socket, until
reqsk_timer_handler() drops them.

Suppose listen() is called again, and the peer's final ACK for one of
those requests arrives while sk_state is TCP_LISTEN:

tcp_v4_rcv()
    if (unlikely(sk->sk_state != TCP_LISTEN))   <- not taken
    tcp_check_req()                             <- no listener re-check
        tcp_v4_syn_recv_sock()
        inet_csk_complete_hashdance()
            inet_csk_reqsk_queue_add()

Under rskq_lock, inet_csk_reqsk_queue_add() still sees TCP_LISTEN and
queues the child:

	} else {
		req->sk = child;
		req->dl_next = NULL;
		...
		queue->rskq_accept_tail = req;
		sk_acceptq_added(sk);
	}

__inet_inherit_port() can succeed here when the port was locked by an
explicit bind(). That is also the setup of the -EADDRINUSE example in
the commit message.

If get_port() or hash() then fails, the socket returns to TCP_CLOSE. The
req and child are still on rskq_accept_head, and sk_ack_backlog is not
zero.

Nothing seems to reclaim them after that:

- inet_csk_accept() rejects sockets that are not listening.

- __tcp_close(), tcp_disconnect() and tcp_abort() only call
  inet_csk_listen_stop() from TCP_LISTEN.

- A later successful listen() overwrites rskq_accept_head and resets
  sk_ack_backlog in reqsk_queue_alloc().

Wouldn't this leak the established child socket, its request_sock, and
the listener reference held through rsk_listener? That is the same kind
of leak this patch fixes for rskq_rst_head.

Could the error path run the full inet_csk_listen_stop() cleanup after
setting TCP_CLOSE? Alternatively, could TCP_LISTEN be set only after
get_port() and hash() succeed?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114111247.21995-1-edumazet%40kernel.org