Re: [PATCH net] tcp: do not coalesce segments after FIN

[Hyunwoo Kim <imv4bel@gmail.com>]

On Thu, Nov 05, 2026 at 01:22:12PM +0100, Eric Dumazet wrote:
> Le jeu. 5 nov. 2026 à 13:09, Hyunwoo Kim <imv4bel@gmail.com> a écrit :
> >
> > A FIN consumes a sequence number. If another FIN segment is coalesced
> > into an skb that already carries FIN, end_seq advances to account for
> > the additional FIN, but tcp_flags still contains only one FIN bit.
> >
> 
> Yeah, we have been working lately on this issue (initial report from
> Chris Mason)
> 
> For some reason Kuniyuki has not sent his patch yet.
> 
> > When the preceding gap is filled and tcp_ofo_queue() processes the
> > coalesced skb, rcv_nxt advances to the skb's end_seq. When the receive
> > path consumes all data in the skb, copied_seq advances by the data
> > length and one sequence number for FIN. If three or more FIN segments
> > have been coalesced into one skb, tcp_inq() can still return a positive
> > value after the receive queue becomes empty and the SOCK_DONE adjustment
> > is applied.
> >
> > Software kTLS receive is one consumer affected by this state. When
> > strp->copy_mode is false, tls_strp_msg_consume() calls the TCP helper
> > tcp_read_done() to consume the current TLS record. In the reproducer,
> > tcp_read_done() consumed the record data and one FIN before freeing the
> > receive skb.
> >
> 
> TCP_ZEROCOPY_RECEIVE is another one: tcp_zerocopy_receive() takes its
> length from tcp_inq() and follows skb->next without checking for the
> end of the receive queue. Please mention it in the changelog.
> 
> 
> > When kTLS looked for the next record, tcp_inq() reported at least
> > TLS_HEADER_SIZE bytes even though the receive queue was empty.
> > tcp_recv_skb() returned NULL, so tls_strp_load_anchor_with_queue() hit
> > WARN_ON_ONCE() and returned without updating the anchor.
> >
> > Since tcp_inq() had reported at least TLS_HEADER_SIZE,
> > tls_strp_read_sock() proceeded to tls_strp_check_queue_ok().
> > skb_shinfo(strp->anchor)->frag_list still pointed to the freed receive
> > skb. tls_strp_check_queue_ok() then read TCP_SKB_CB(first)->seq through
> > the stale pointer, causing a use-after-free.
> >
> > The reproducer followed this call path:
> >
> >   tls_rx_rec_done()
> >     tls_strp_msg_consume()
> >       tcp_read_done()                       // frees the receive skb
> >     tls_strp_check_rcv()
> >       tls_strp_read_sock()
> >         tls_strp_load_anchor_with_queue()   // returns after WARN_ON_ONCE()
> >           tcp_recv_skb() == NULL
> >         tls_strp_check_queue_ok()
> >           TCP_SKB_CB(first)->seq            // reads the freed receive skb
> >
> > KASAN reported:
> >
> >   BUG: KASAN: slab-use-after-free in tls_strp_check_queue_ok.isra.0+0x5f/0x100
> >   Read of size 4 at addr ffff888103169ca8 by task ktls-server/71
> >   ...
> >   Call Trace:
> >    <TASK>
> >    ...
> >    tls_strp_check_queue_ok.isra.0+0x5f/0x100
> >    tls_strp_check_rcv+0x2a9/0x400
> >    ...
> >    tls_sw_recvmsg+0x3ce/0xa10
> >    ...
> >    sock_recvmsg+0xea/0x100
> >    __sys_recvfrom+0x142/0x1f0
> >    ...
> >    __x64_sys_recvfrom+0x76/0x90
> >    do_syscall_64+0xdd/0x4a0
> >    entry_SYSCALL_64_after_hwframe+0x77/0x7f
> >    ...
> >    </TASK>
> >   ...
> >   Freed by task 71:
> >    ...
> >    napi_skb_cache_put+0x24/0xf0
> >    skb_attempt_defer_free+0x183/0x3b0
> >    tcp_read_done+0x2d4/0x340
> >    tls_strp_msg_consume+0x45/0x80
> >    tls_sw_recvmsg+0x3c4/0xa10
> >    ...
> >
> > In tcp_ooo_try_coalesce() and tcp_add_backlog(), skip coalescing when
> > the destination skb already carries FIN. This still allows a FIN
> > segment to be coalesced into an skb without FIN when the other
> > coalescing checks pass.
> >
> > Fixes: e93a0435f809 ("tcp: allow segment with FIN in tcp_try_coalesce()")
> > Fixes: 4f693b55c3d2 ("tcp: implement coalescing on backlog queue")
> > Cc: stable@vger.kernel.org
> > Assisted-by: LLM
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> >  net/ipv4/tcp_input.c | 3 ++-
> >  net/ipv4/tcp_ipv4.c  | 1 +
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index e1f2d9765f810..1f9f86028da73 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -5314,7 +5314,8 @@ static bool tcp_ooo_try_coalesce(struct sock *sk,
> >                              struct sk_buff *from,
> >                              bool *fragstolen)
> >  {
> > -       bool res = tcp_try_coalesce(sk, to, from, fragstolen);
> > +       bool res = !(TCP_SKB_CB(to)->tcp_flags & TCPHDR_FIN) &&
> > +                  tcp_try_coalesce(sk, to, from, fragstolen);
> 
> Please put the check in tcp_try_coalesce() itself.
> 
> The receive queue callers can not hit this today (tcp_fin() purges
> the ofo queue, and tcp_rcv_state_process() drops anything at or after
> rcv_nxt in CLOSE_WAIT/CLOSING/LAST_ACK), but the check costs nothing
> there, and it keeps the rule "nothing coalesces after a FIN" in one
> place.
> 
> Also, since TCP_SKB_CB(to)->tcp_flags & TCPHDR_FIN should almost never
> be true, it should come after the more relevant tests, just before
> skb_try_coalesce():
> 
>         if (TCP_SKB_CB(from)->seq != TCP_SKB_CB(to)->end_seq)
>                 return false;
> 
>         if (!tcp_skb_can_collapse_rx(to, from))
>                 return false;
> 
>         /* Nothing follows a FIN. */
>         if (unlikely(TCP_SKB_CB(to)->tcp_flags & TCPHDR_FIN))
>                 return false;
> 
>         if (!skb_try_coalesce(to, from, fragstolen, &delta))
>                 return false;
> 
> 
> >
> >         /* In case tcp_drop_reason() is called later, update to->gso_segs */
> >         if (res) {
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index a7f9242f1e429..ef01768292fbc 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1949,6 +1949,7 @@ enum skb_drop_reason tcp_add_backlog(struct sock *sk, struct sk_buff *skb)
> >         thtail = (struct tcphdr *)tail->data;
> >
> >         if (TCP_SKB_CB(tail)->end_seq != TCP_SKB_CB(skb)->seq ||
> > +           (TCP_SKB_CB(tail)->tcp_flags & TCPHDR_FIN) ||
> 
> 
> Same remark: please move this after the other tcp_flags tests, with a
> short comment:
> 
>             /* Nothing follows a FIN, see tcp_try_coalesce() */
>             (TCP_SKB_CB(tail)->tcp_flags & TCPHDR_FIN) ||
> 
> Checking only the tail is the right thing to do. It keeps the common
> case where the last data+FIN segment is merged onto a data tail,
> which happens on every connection close while the socket is owned by
> the user.

Understood. I'll send a v2 addressing your comments.


Best regards,
Hyunwoo Kim