Hi Eric,
On Mon, Feb 23, 2026 at 09:36:38AM +0100, Eric Dumazet wrote:
> On Mon, Feb 23, 2026 at 9:01???AM Eric Dumazet <[email protected]> wrote:
> ...
> > >
> > > Fixes: 9ca48d616ed7 ("tcp: do not accept packets beyond window")
> >
> > OK, but this commit is fine ? It seems the issue is coming from buggy peers
> > ?
That "Fixes" tag is a technicality, I did not want to imply that the
commit is broken: 9ca48d616ed7 (which is RFC compliant) turns the
workaround (which isn't RFC compliant) 2bd99aef1b19 ("tcp:
accept bare FIN packets under memory pressure") into dead code.
If we still need that workaround, technically, this is a regression
caused by 9ca48d616ed7. If not, I am perfectly happy to propose a
commit that removes the dead code.
> > Eventually the receive queue would be drained by the application, the
> > peer would retransmit
> > this FIN, and it would be accepted.
If I understand the problem correctly, the workaround was introduced
to break a FIN/ACK loop because the broken peer (macOS) did not use
exponential backoff in that scenario. So yes, it would be accepted,
but until then we and the peer would ping-pong FIN/ACKs.
But as said, if we don't need the workaround I can submit a patch to
remove it.
> ...
> > > + reason = tcp_sequence(sk, TCP_SKB_CB(skb)->seq,
> > > + TCP_SKB_CB(skb)->end_seq - th->fin);
> >
> > I don't think this is the right fix. Basically it says that FIN do not
> > count, but TCP RFC says otherwise.
We can be more specific and only allow that if we have a zero window.
(I thought I would not hurt much to accept that FIN which does not
take up real space)
> > It also adds code in TCP fast path.
Hmm, the call site for tcp_validate_incoming() in
tcp_rcv_established() is:
/*
* Standard slow path.
*/
validate:
if (!tcp_validate_incoming(sk, skb, th, 1))
return;
Am I missing something?
> We can keep fast path unchanged with this variant.
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index
> e7b41abb82aad33d8cab4fcfa989cc4771149b41..156c92450f3ed00357aff2ef3e586b83f3cecb5e
> 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4858,15 +4858,24 @@ static enum skb_drop_reason
> tcp_disordered_ack_check(const struct sock *sk,
> */
>
> static enum skb_drop_reason tcp_sequence(const struct sock *sk,
> - u32 seq, u32 end_seq)
> + u32 seq, u32 end_seq,
> + const struct tcphdr *th)
> {
> const struct tcp_sock *tp = tcp_sk(sk);
> + u32 seq_limit;
>
> if (before(end_seq, tp->rcv_wup))
> return SKB_DROP_REASON_TCP_OLD_SEQUENCE;
>
> - if (after(end_seq, tp->rcv_nxt + tcp_receive_window(tp))) {
> - if (after(seq, tp->rcv_nxt + tcp_receive_window(tp)))
> + seq_limit = tp->rcv_nxt + tcp_receive_window(tp);
> + if (unlikely(after(end_seq, seq_limit))) {
> + /* Some stacks are known to handle FIN incorrectly;
> allow the FIN
> + * to extend beyond the window and check it in detail later.
> + */
> + if (!after(end_seq - th->fin, seq_limit))
> + return SKB_NOT_DROPPED_YET;
> +
> + if (after(seq, seq_limit))
> return SKB_DROP_REASON_TCP_INVALID_SEQUENCE;
>
> /* Only accept this packet if receive queue is empty. */
> @@ -6379,7 +6388,8 @@ static bool tcp_validate_incoming(struct sock
> *sk, struct sk_buff *skb,
>
> step1:
> /* Step 1: check sequence number */
> - reason = tcp_sequence(sk, TCP_SKB_CB(skb)->seq,
> TCP_SKB_CB(skb)->end_seq);
> + reason = tcp_sequence(sk, TCP_SKB_CB(skb)->seq,
> + TCP_SKB_CB(skb)->end_seq, th);
> if (reason) {
> /* RFC793, page 37: "In all states except SYN-SENT, all reset
> * (RST) segments are validated by checking their SEQ-fields."
Sure, I can do that. Do you want me to add the
tcp_receive_window(tp) == 0 check to narrow it down further?
(And a dumb question: As you are the author of that code, how do I
attribute that commit when submitting a v2?)
