Re: race between tcp_close and delack?

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, Oct 10, 2008 at 01:29:00PM +0200, Manuel Bouyer wrote:
> gasp, you're right. So this isn't my bug :( Sigh.

I found another path which could lead to a similar race.
The tcp timers are armed/disarmed, but we don't check if the callback is
being invoked. One of them at last can cause the panic I'm seeing in
tcp_output():
                if (win == 0) {
                        TCP_TIMER_DISARM(tp, TCPT_REXMT);
                        tp->t_rxtshift = 0;
                        tp->snd_nxt = tp->snd_una;
                        if (TCP_TIMER_ISARMED(tp, TCPT_PERSIST) == 0)
                                tcp_setpersist(tp);
                }

If TCPT_REXMT's callback is invoking at this point, it'll be run just
after tcp_output() exists. tcp_timer_rexmt() will restart the TCPT_REXMT
timer, so now we have both TCPT_REXMT and TCPT_PERSIST pending.

There are other places where we stop TCPT_REXMT, and potentially
arm another timer in the same splsoftnet() block (e.g. tcp_input
stops TCPT_REXMT and may call tcp_output, which may arm TCPT_PERSIST,
tcp_sack does it too).

There are also in tcp_output() possible paths where we disarm TCPT_PERSIST,
and later TCPT_REXMT while TCPT_PERSIST may still be invoking. I think
there could also cause the tcp_setpersist panic, as tcp_timer_persist can
call tcp_setpersist().

As TCP_TIMER_DISARM() will clear the pending and expirted states, I think a
possible way to close this race is to check callout_expired()
in the callout handler, and abort the handler if it's not expired.
See attached patch.

It looks like the same issue exists in netbsd-4 and current.
Comments ? Did I miss sometheing ?

-- 
Manuel Bouyer, LIP6, Universite Paris VI.           
Manuel.Bouyer%lip6.fr@localhost
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: tcp_timer.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/tcp_timer.c,v
retrieving revision 1.71
diff -u -r1.71 tcp_timer.c
--- tcp_timer.c 2 Mar 2005 10:20:18 -0000       1.71
+++ tcp_timer.c 10 Oct 2008 16:16:15 -0000
@@ -232,6 +232,10 @@
                splx(s);
                return;
        }
+       if (!callout_expired(&tp->t_delack_ch)) {
+               splx(s);
+               return;
+       }
 
        tp->t_flags |= TF_ACKNOW;
        (void) tcp_output(tp);
@@ -293,6 +297,10 @@
                splx(s);
                return;
        }
+       if (!callout_expired(&tp->t_timer[TCPT_REXMT])) {
+               splx(s);
+               return;
+       }
 
 #ifdef TCP_DEBUG
 #ifdef INET
@@ -453,6 +461,10 @@
                splx(s);
                return;
        }
+       if (!callout_expired(&tp->t_timer[TCPT_PERSIST])) {
+               splx(s);
+               return;
+       }
 
 #ifdef TCP_DEBUG
 #ifdef INET
@@ -520,6 +532,10 @@
                splx(s);
                return;
        }
+       if (!callout_expired(&tp->t_timer[TCPT_KEEP])) {
+               splx(s);
+               return;
+       }
 
 #ifdef TCP_DEBUG
        ostate = tp->t_state;
@@ -607,6 +623,10 @@
                splx(s);
                return;
        }
+       if (!callout_expired(&tp->t_timer[TCPT_2MSL])) {
+               splx(s);
+               return;
+       }
 
        /*
         * 2 MSL timeout went off, clear the SACK scoreboard, reset