TCP question: Is this simultaneous close handling broken?

[Peter Wemm]

On 1/7/14, 3:39 PM, Mike Tancsa wrote:
> On 1/7/2014 5:40 PM, Peter Wemm wrote:
> 
>> The packet may be dropped without processing the FIN flag.
> 
>> MFC after:	never
> 
> Hi,
> 	Are there any potential side effects to this fix ? The original
> author said they were not going to MFC due to possible regressions. I
> know you probably see more FreeBSD traffic then most at Y!, and so are
> very sensitive to this, but thought I would ask for clarification.
> 
> 	---Mike

Actually, I'm very troubled by that entire chunk of code.

This is the actual fix:
------------------------------------------------------------------------
r239672 | rrs | 2012-08-25 02:26:37 -0700 (Sat, 25 Aug 2012) | 12 lines

This small change takes care of a race condition
that can occur when both sides close at the same time.
If that occurs, without this fix the connection enters
FIN1 on both sides and they will forever send FIN|ACK at
each other until the connection times out. This is because
we stopped processing the FIN|ACK and thus did not advance
the sequence and so never ACK'd each others FIN. This
fix adjusts it so we *do* process the FIN properly and
the race goes away ;-)

MFC after:      1 month
------------------------------------------------------------------------

It was MFC'ed all the way to stable/6 in Feb 2013.

r258821 from the PR does not handle the case where a fin arrives while we
are in a retransmit state ourselves due to packet loss and interferes with
the retransmits.  r258821 needs to be backed out.

I'm not convinced that r239672 is complete, or even entirely correct.

For completeness I'm concerned about the by the if (tp->t_flags &
TF_SACK_PERMIT) .. goto drop; scenario.

Shouldn't the TH_FIN checks be moved so that it looks more like this?

@@ -2534,16 +2535,6 @@
                        }
                        tp->snd_nxt = th->th_ack;
                        tp->snd_cwnd = tp->t_maxseg;
-                       if ((thflags & TH_FIN) &&
-                           (TCPS_HAVERCVDFIN(tp->t_state) == 0)) {
-                               /*
-                                * If its a fin we need to process
-                                * it to avoid a race where both
-                                * sides enter FIN-WAIT and send FIN|ACK
-                                * at the same time.
-                                */
-                               break;
-                       }
                        (void) tcp_output(tp);
                        KASSERT(tp->snd_limited <= 2,
                            ("%s: tp->snd_limited too big",
@@ -2553,7 +2544,11 @@
                             (tp->t_dupacks - tp->snd_limited);
                        if (SEQ_GT(onxt, tp->snd_nxt))
                                tp->snd_nxt = onxt;
-                       goto drop;
+                       if ((thflags & TH_FIN) &&
+                           (TCPS_HAVERCVDFIN(tp->t_state) == 0))
+                               break;  /* respond to incoming FIN */
+                       else
+                               goto drop;
                } else if (V_tcp_do_rfc3042) {
                        cc_ack_received(tp, th, CC_DUPACK);
                        u_long oldcwnd = tp->snd_cwnd;

ie: respond to it as we normally would, but check to see if it's a new FIN
state before discarding it?

NetBSD handle this differently.  They seem to check to see if it's a
genuinely redundant/duplicate segment earlier and leverage it here.

It seems like our handling of this is all wrong.
-- 
Peter Wemm - peter at wemm.org; peter at FreeBSD.org; peter at yahoo-inc.com; KI6FJV
UTF-8: for when a ' just won\342\200\231t do.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 246 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/freebsd-net/attachments/20140107/504ac33d/attachment.sig>