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

2015-07-01 Thread hiren panchasara 
Created https://reviews.freebsd.org/D2970 to get faster closure on this
issue.

Cheers,
Hiren

ps: To comment, you need to sign up for an account. :-(  I am not a fan
of the whole thing but people seem to respond faster on
reviews.freebsd.org so I went with it.


pgpYf6BdgHz3N.pgp
Description: PGP signature

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

2015-06-22 Thread 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=revisionrevision=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

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

2015-06-17 Thread 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=revisionrevision=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


pgpzITCe9ef4C.pgp
Description: PGP signature

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

2015-06-17 Thread Jean-Francois HREN 
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=revisionrevision=249372 ) now allows
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 @@
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) {

Jean-François Hren
ASQ Team Member
http://www.stormshield.eu

STORMSHIELD
Parc Scientifique de la Haute Borne
Parc Horizon - Bâtiment 6
Avenue de l'Horizon
59650 Villeneuve d'Ascq
France

___
freebsd-net@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to freebsd-net-unsubscr...@freebsd.org