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

[hiren panchasara]

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
-------------- 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/20150622/0a05bacb/attachment.sig>