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

[Hyunwoo Kim <imv4bel@gmail.com>]

On Fri, Nov 13, 2026 at 09:59:54AM +0100, Eric Dumazet wrote:
> Le jeu. 12 nov. 2026 à 18:05, <netdev-bot+sashiko@kernel.org> a écrit :
> >
> > 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
> 
> This looks legitimate, but it is a separate and pre-existing issue,
> not something added or made worse by this patch.
> 
> inet_csk_listen_start() sets TCP_LISTEN before get_port()/hash() and
> goes back to TCP_CLOSE on failure without purging rskq_rst_head, so an
> RST for a Fast Open child accepted in a previous life of the listener
> can queue a request there during that window. Nothing reclaims it
> afterwards.
> 
> It needs listen() to fail (e.g. -EADDRINUSE because another
> SO_REUSEADDR socket started listening on the port) with the RST
> landing while get_port() runs, so the window is quite small.
> 
> Hyunwoo, no need to respin, your v2 is fine as is for the
> inet_csk_listen_stop() side.
> 
> Reviewed-by: Eric Dumazet <edumazet@kernel.org>

Thanks for the review.

> 
> I will send a follow-up on top of it, moving the purge to a helper
> that is also called from the inet_csk_listen_start() error path.

I'll review the follow-up patch once it's posted.


Best regards,
Hyunwoo Kim