Sequence number handling issue with TCP data and FIN flag with a transient error

[hiren panchasara]

On 06/17/15 at 03:10P, Jean-Francois HREN wrote:
> Hello, while investigating a freeze on a modified FreeBSD 9.3 I stumbled upon
> a potential bug in netinet/tcp_output.c
> 
> If an error occurs while processing a TCP segment with some data and the FIN flag,
> the back out of the sequence number advance does not take into account
> the increase by 1 due to the FIN flag
> (see https://svnweb.freebsd.org/base/head/sys/netinet/tcp_output.c?view=markup#l1360
> and https://svnweb.freebsd.org/base/head/sys/netinet/tcp_output.c?view=markup#l1439 ).
> 
> In the case of a transient error, this leads to a retransmitted TCP segment with
> a shifted by 1 sequence number and a missing first byte in the TCP payload.
> 
> In FreeBSD 9.3, it happens only when an error occurs in netinet/ip_output.c::ip_output()
> or netinet6/ip6_output::ip6_output() but in head, R249372
> ( https://svnweb.freebsd.org/base?view=revision&revision=249372 ) now allows
> the same behaviour if an ENOBUFS error occurs in netinet/tcp_output.c

Your analysis looks correct to me.
> 
> Tentative solutions would be either to remove the back out of the sequence
> number advance completely and to treat transient error cases like real lost
> packets
> 
> --- netinet/tcp_output.c
> +++ netinet/tcp_output.c
> @@ -1435,8 +1435,7 @@
>  				tp->sackhint.sack_bytes_rexmit -= len;
>  				KASSERT(tp->sackhint.sack_bytes_rexmit >= 0,
>  				    ("sackhint bytes rtx >= 0"));
> -			} else
> -				tp->snd_nxt -= len;
> +			}
>  		}
>  		SOCKBUF_UNLOCK_ASSERT(&so->so_snd);	/* Check gotos. */
>  		switch (error) {
> 
> or to decrease the sequence number advance by 1 if a FIN flag was sent.
> 
> --- netinet/tcp_output.c
> +++ netinet/tcp_output.c
> @@ -1435,8 +1435,11 @@
>  				tp->sackhint.sack_bytes_rexmit -= len;
>  				KASSERT(tp->sackhint.sack_bytes_rexmit >= 0,
>  				    ("sackhint bytes rtx >= 0"));
> -			} else
> +			} else {
>  				tp->snd_nxt -= len;
> +				if (flags & TH_FIN)
> +					tp->snd_nxt--;
> +			}
>  		}
>  		SOCKBUF_UNLOCK_ASSERT(&so->so_snd);	/* Check gotos. */
>  		switch (error) {

I like the second approach better.

Cheers,
Hiren
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 618 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20150617/5986c2f0/attachment.sig>