Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 17 Jun 2015 15:10:29 +0200 (CEST)
From:      Jean-Francois HREN <jean-francois.hren@stormshield.eu>
To:        freebsd-net@freebsd.org
Cc:        Damien DEVILLE <damien.deville@stormshield.eu>,  Fabien Thomas <fabien.thomas@stormshield.eu>
Subject:   Sequence number handling issue with TCP data and FIN flag with a transient error
Message-ID:  <1176517609.2815392.1434546629748.JavaMail.zimbra@stormshield.eu>
In-Reply-To: <1180135344.2814172.1434546351593.JavaMail.zimbra@stormshield.eu>

next in thread | previous in thread | raw e-mail | index | archive | help
Hello, while investigating a freeze on a modified FreeBSD 9.3 I stumbled up=
on
a potential bug in netinet/tcp_output.c

If an error occurs while processing a TCP segment with some data and the FI=
N 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=3Dm=
arkup#l1360
and https://svnweb.freebsd.org/base/head/sys/netinet/tcp_output.c?view=3Dma=
rkup#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=3Drevision&revision=3D249372 ) now a=
llows
the same behaviour if an ENOBUFS error occurs in netinet/tcp_output.c

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 @@
 =09=09=09=09tp->sackhint.sack_bytes_rexmit -=3D len;
 =09=09=09=09KASSERT(tp->sackhint.sack_bytes_rexmit >=3D 0,
 =09=09=09=09    ("sackhint bytes rtx >=3D 0"));
-=09=09=09} else
-=09=09=09=09tp->snd_nxt -=3D len;
+=09=09=09}
 =09=09}
 =09=09SOCKBUF_UNLOCK_ASSERT(&so->so_snd);=09/* Check gotos. */
 =09=09switch (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 @@
 =09=09=09=09tp->sackhint.sack_bytes_rexmit -=3D len;
 =09=09=09=09KASSERT(tp->sackhint.sack_bytes_rexmit >=3D 0,
 =09=09=09=09    ("sackhint bytes rtx >=3D 0"));
-=09=09=09} else
+=09=09=09} else {
 =09=09=09=09tp->snd_nxt -=3D len;
+=09=09=09=09if (flags & TH_FIN)
+=09=09=09=09=09tp->snd_nxt--;
+=09=09=09}
 =09=09}
 =09=09SOCKBUF_UNLOCK_ASSERT(&so->so_snd);=09/* Check gotos. */
 =09=09switch (error) {

Jean-Fran=C3=A7ois Hren
ASQ Team Member
http://www.stormshield.eu

STORMSHIELD
Parc Scientifique de la Haute Borne
Parc Horizon - B=C3=A2timent 6
Avenue de l'Horizon
59650 Villeneuve d'Ascq
France




Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1176517609.2815392.1434546629748.JavaMail.zimbra>