Re: [PATCH net v2 6/8] tcp: fix use-after-free in the lockless listener path

[Hyunwoo Kim]

On Tue, Sep 01, 2026 at 10:03:51AM +0200, Paolo Abeni wrote:
> On 9/1/26 9:37 AM, Hyunwoo Kim wrote:
> > On Mon, Aug 24, 2026 at 12:32:50PM +0900, Hyunwoo Kim wrote:
> >> tcp_v{4,6}_rcv() calls tcp_v{4,6}_do_rcv() without holding the socket
> >> lock when sk->sk_state is TCP_LISTEN. Every other path into
> >> tcp_v{4,6}_do_rcv() holds it.
> >>
> >> tcp_v{4,6}_do_rcv() and tcp_rcv_state_process() below it read
> >> sk->sk_state again. A listener can leave TCP_LISTEN through
> >> connect(AF_UNSPEC), and if that happens in between, the second read
> >> returns a different state.
> >>
> >> tcp_rcv_established() or tcp_rcv_state_process() then runs without the
> >> lock. If the second read returns TCP_SYN_SENT, the incoming SYN is
> >> treated as a crossed SYN and reaches tcp_send_synack(). When the SYN skb
> >> at the head of the retransmit queue is skb_cloned(), that function
> >> replaces it with a copy and releases the original with
> >> tcp_rtx_queue_unlink_and_free().
> >>
> >> The original is the skb that a thread on another CPU is transmitting
> >> right now in __tcp_transmit_skb(). skb_cloned() is true because the
> >> clone made for that transmit is still alive. Once the transmit returns,
> >> tcp_update_skb_after_send() calls list_move_tail() on the skb's
> >> tcp_tsorted_anchor.
> >>
> >> In short:
> >>
> >>   socket(AF_INET) -> bind() -> listen()   // the socket that changes state
> >>   socket(AF_INET) -> bind() -> listen()   // the peer
> >>
> >>   Several threads keep opening new sockets and connecting to the first
> >>   socket's address.
> >>
> >>   Another thread repeats this on the first socket:
> >>     connect(AF_UNSPEC)      // TCP_LISTEN -> TCP_CLOSE
> >>     connect(peer address)   // TCP_CLOSE -> TCP_SYN_SENT
> >>                             // another CPU still sees a listener, handles
> >>                             // one of those SYNs without the lock and
> >>                             // releases the SYN skb that this connect()
> >>                             // is transmitting
> >>                             // -> use-after-free
> >>     connect(AF_UNSPEC)
> >>     listen()                // TCP_LISTEN again
> >>
> >> KASAN log:
> >>
> >>   BUG: KASAN: slab-use-after-free in __list_del_entry_valid_or_report+0x14/0x140
> >>   Read of size 8 at addr ffff88800a5d1460 by task poc/125
> >>   ...
> >>   Call Trace:
> >>    __list_del_entry_valid_or_report+0x14/0x140
> >>    tcp_update_skb_after_send+0x62/0x170
> >>    __tcp_transmit_skb+0xe33/0x1e40
> >>    tcp_connect+0x1b67/0x2490
> >>    tcp_v4_connect+0x998/0xab0
> >>    __inet_stream_connect+0x22c/0x700
> >>    inet_stream_connect+0x48/0x70
> >>    __sys_connect+0x101/0x130
> >>   ...
> >>   Allocated by task 125:
> >>    __alloc_skb+0xd1/0x370
> >>    tcp_stream_alloc_skb+0x2d/0x2b0
> >>    tcp_connect+0x72d/0x2490
> >>    tcp_v4_connect+0x998/0xab0
> >>    __inet_stream_connect+0x22c/0x700
> >>    inet_stream_connect+0x48/0x70
> >>    __sys_connect+0x101/0x130
> >>   ...
> >>   The buggy address belongs to the object at ffff88800a5d1400
> >>    which belongs to the cache skbuff_fclone_cache of size 472
> >>
> >> Instead of taking the lock, keep the lockless path from reading
> >> sk->sk_state again to decide how to process the packet. Move the
> >> TCP_LISTEN handling out of tcp_rcv_state_process() into
> >> tcp_rcv_listen_state_process(), and let the TCP_LISTEN branch of
> >> tcp_v{4,6}_rcv() call a new tcp_v{4,6}_rcv_listen(). Listener processing
> >> does not change. The TCP_LISTEN arm of tcp_v{4,6}_do_rcv() is left
> >> alone, because a socket can finish listen() after the state check and a
> >> backlogged skb is then processed there.
> >>
> >> Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Hyunwoo Kim <imv4bel@xxxxxxxxx>
> > 
> > Looking at this further, unhashing the listener and then calling
> > synchronize_net() lets the disconnect path handle it. MPTCP needs a fix
> > too, though, because it closes and reuses the first subflow directly
> > without going through tcp_disconnect().
> 
> This looks like a more palatable approach: this patch in the current
> format looked way too invasive to me.
> 
> > This also closes the trigger path for patches 4, 5 and 8. I would still
> > keep those, since they add no work to the fast path and they remove the
> > root cause itself. Their changelogs would have to change though.
> 
> I'm unsure accepting new connections is not fast-path: the connection
> per second rate is a relevant metric for a sever, even if the additional
> cleanup is possibly not visible in most benchmarks.
> 
> Still I would avoid additional unneeded patches.

Yeah, I will drop those patches when I send v3.


Best regards,
Hyunwoo Kim