On 06/17/15 at 11:37P, hiren panchasara wrote: > 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.
Does anyone else have any opinion on this? We should commit this to -head soon to get it in for 10.2 time-frame. Cheers, Hiren
pgp3DCyNOhCU8.pgp
Description: PGP signature
