race between tcp_close and delack ?

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

Hi,
context: on a netbsd-3 amd64 server I've got several instances of this panic:
panic: tcp_output REXMT
panic() at netbsd:panic+0x1c8   
tcp_segsize() at netbsd:tcp_segsize
tcp_timer_persist() at netbsd:tcp_timer_persist+0x73
softclock() at netbsd:softclock+0x2c9
softintr_dispatch() at netbsd:softintr_dispatch+0x99
DDB lost frame for netbsd:Xsoftclock+0x2d, trying 0xffffffff8069bcd0
Xsoftclock() at netbsd:Xsoftclock+0x2d 

I think I've found the cause for this: in tcp_close(), we
check if a timer is being invoked, and set TF_DEAD in this case, otherwise
we just pool_put() the pcb. In the TF_DEAD case, it'll be pool_put() by
tcp_isdead(), called from the timer hanbler.

Problem: we check if a timer is being invoked with tcp_timers_invoking(),
but this forgets the t_delack_ch timer. So we may tcp_close() a pcb
while the tcp_delack() is being called. The pcb will be returned to
the pool, then tcp_delack() continue to run, eventually causing one
of the other timers to be rearmed. In the meantime, this pcb may have
been reused, causing the above panic when the timer fires.
This issue is also present in netbsd-4 but seems to be fixed in -current
(by using proper locking).

Does anyone see anything wrong with the above analysis, or the attached 
patch ? 

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: tcp_subr.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/tcp_subr.c,v
retrieving revision 1.187.2.3
diff -u -r1.187.2.3 tcp_subr.c
--- tcp_subr.c  6 May 2005 08:39:52 -0000       1.187.2.3
+++ tcp_subr.c  9 Oct 2008 20:55:19 -0000
@@ -1071,7 +1071,8 @@
        int dead = (tp->t_flags & TF_DEAD);
 
        if (__predict_false(dead)) {
-               if (tcp_timers_invoking(tp) > 0)
+               if (tcp_timers_invoking(tp) > 0 ||
+                   callout_invoking(&tp->t_delack_ch))
                                /* not quite there yet -- count separately? */
                        return dead;
                tcpstat.tcps_delayed_free++;
@@ -1200,7 +1201,7 @@
                m_free(tp->t_template);
                tp->t_template = NULL;
        }
-       if (tcp_timers_invoking(tp))
+       if (tcp_timers_invoking(tp) || callout_invoking(&tp->t_delack_ch))
                tp->t_flags |= TF_DEAD;
        else
                pool_put(&tcpcb_pool, tp);