Re: [PATCH net-next] tcp: refine tcp_sequence() for the FIN exception

[Simon Baatz <gmbnomis@gmail.com>]

Hi Eric,

On Mon, Jun 08, 2026 at 03:14:52PM +0000, Eric Dumazet wrote:
> Commit 0e24d17bd966 ("tcp: implement RFC 7323 window retraction
> receiver requirements") removed the special FIN case that
> was added in commit 1e3bb184e941 ("tcp: re-enable acceptance of
> FIN packets when RWIN is 0").

Commit 0e24d17bd966 did not remove the special handling; it is still
present and covered by the test "tcp_rcv_zero_wnd_fin.pkt".
 
> If a peer sends a segment containing data and a FIN flag before
> it learns about our window retraction and has a buggy TCP stack,
> it might place the FIN one byte beyond what it thinks is the
> right edge of the window (i.e., max_window_edge + 1).

The FIN exception in tcp_data_queue() is not a generic allowance for
incorrect FIN handling.  It is much more specific and only applies
when:

1. the packet is in-sequence 
2. RWIN == 0
3. the packet is a bare FIN
 
> The data portion (end_seq - th->fin) will end exactly at max_window_edge.
> In this case, we will drop the packet if our receive queue is not empty,
> even though the data was sent within the window we previously allowed.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Simon Baatz <gmbnomis@gmail.com>
> ---
>  net/ipv4/tcp_input.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index ab7a4e5435a8a2cbb532d42c54af76d8541c903b..8560a9c6d38207c098d673497caf2c7652c36f5c 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4812,18 +4812,20 @@ static enum skb_drop_reason tcp_sequence(const struct sock *sk,
>  					 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 (unlikely(after(end_seq, tp->rcv_nxt + tcp_max_receive_window(tp)))) {
> +	seq_limit = tp->rcv_nxt + tcp_max_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, tp->rcv_nxt + tcp_receive_window(tp)))
> +		if (!after(end_seq - th->fin, seq_limit))
>  			return SKB_NOT_DROPPED_YET;

It is not clear which additional case this change is intended to
allow.  Are you sure such a packet would not be rejected by later
checks in the data path?

(For the existing FIN exception, the previous condition also seems
broader than necessary. Actually, it should be sufficient to use
"!after(end_seq - th->fin, tp->rcv_nxt)")

>  
> -		if (after(seq, tp->rcv_nxt + tcp_max_receive_window(tp)))
> +		if (after(seq, seq_limit))
>  			return SKB_DROP_REASON_TCP_INVALID_SEQUENCE;
>  
>  		/* Only accept this packet if receive queue is empty. */
> -- 
> 2.54.0.1032.g2f8565e1d1-goog
> 
-- 
Simon Baatz <gmbnomis@gmail.com>