Date: Wed, 17 Jun 2015 11:37:34 -0700 From: hiren panchasara <hiren@strugglingcoder.info> To: Jean-Francois HREN <jean-francois.hren@stormshield.eu> Cc: freebsd-net@freebsd.org, Damien DEVILLE <damien.deville@stormshield.eu>, Fabien Thomas <fabien.thomas@stormshield.eu> Subject: Re: Sequence number handling issue with TCP data and FIN flag with a transient error Message-ID: <20150617183734.GA53336@strugglingcoder.info> In-Reply-To: <1176517609.2815392.1434546629748.JavaMail.zimbra@stormshield.eu> References: <1180135344.2814172.1434546351593.JavaMail.zimbra@stormshield.eu> <1176517609.2815392.1434546629748.JavaMail.zimbra@stormshield.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--Kj7319i9nmIyA2yE Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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= =3Dmarkup#l1360 > and https://svnweb.freebsd.org/base/head/sys/netinet/tcp_output.c?view=3D= markup#l1439 ). >=20 > In the case of a transient error, this leads to a retransmitted TCP segme= nt with > a shifted by 1 sequence number and a missing first byte in the TCP payloa= d. >=20 > In FreeBSD 9.3, it happens only when an error occurs in netinet/ip_output= =2Ec::ip_output() > or netinet6/ip6_output::ip6_output() but in head, R249372 > ( https://svnweb.freebsd.org/base?view=3Drevision&revision=3D249372 ) now= allows > the same behaviour if an ENOBUFS error occurs in netinet/tcp_output.c Your analysis looks correct to me. >=20 > Tentative solutions would be either to remove the back out of the sequence > number advance completely and to treat transient error cases like real lo= st > packets >=20 > --- netinet/tcp_output.c > +++ netinet/tcp_output.c > @@ -1435,8 +1435,7 @@ > tp->sackhint.sack_bytes_rexmit -=3D len; > KASSERT(tp->sackhint.sack_bytes_rexmit >=3D 0, > ("sackhint bytes rtx >=3D 0")); > - } else > - tp->snd_nxt -=3D len; > + } > } > SOCKBUF_UNLOCK_ASSERT(&so->so_snd); /* Check gotos. */ > switch (error) { >=20 > or to decrease the sequence number advance by 1 if a FIN flag was sent. >=20 > --- netinet/tcp_output.c > +++ netinet/tcp_output.c > @@ -1435,8 +1435,11 @@ > tp->sackhint.sack_bytes_rexmit -=3D len; > KASSERT(tp->sackhint.sack_bytes_rexmit >=3D 0, > ("sackhint bytes rtx >=3D 0")); > - } else > + } else { > tp->snd_nxt -=3D 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 --Kj7319i9nmIyA2yE Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (FreeBSD) iQF8BAEBCgBmBQJVgb5tXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRBNEUyMEZBMUQ4Nzg4RjNGMTdFNjZGMDI4 QjkyNTBFMTU2M0VERkU1AAoJEIuSUOFWPt/lUBIH/2OFy13i19/4LVQf80h2HFd+ b1w/nuH8Q/qCi66JYb/8qiYj9RizkezEHXsonq0P709/Xe8ESr3KbcXo2KE3c1GW weWkeb9kwF1bZDip8ZdLlEmisNhQ8dlPKxeaYq95RR4w4J5PBEysxIY2RPyMmC4U qI+rcn+cVeMH+OCeqMBt5C9sI39DBru1jP3ZJ+msfczgrjNy8VSxo2rMDmdY9ZPr 7M5+8zR+2D8pSRefYfxM6Q5bh8lwgpiQMMQ9WBv4lph7uE8GrXO1I1twtMJEdKku TgZyJyp8/wh8cgG3qCzy6UVBPaCAJXzWG+tI5LIEhs/T+wuG9O3SU2ti/Bl4OKk= =fcI+ -----END PGP SIGNATURE----- --Kj7319i9nmIyA2yE--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150617183734.GA53336>