Re: KASSERT() in kern_timeout.c

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

On Mon, Apr 19, 2010 at 02:41:42PM +0200, Manuel Bouyer wrote:
> > That won't work well, because syn_cache_put() is called from
> > syn_cache_timer(). In addition syn_cache_rm() which does a callout_stop
> > is always called before syn_cache_put(), or syn_cache_put() is called
> > before the timer is armed. 
> > would the attacched diff be OK ?
> 
> Forgot to free sc_route and sc_ipopts in the dropit: case. Updated
> patch attached.

This doesn't work because the timer is stopped in syn_cache_rm(), so
syn entries are never freed.
The attached patch fixes this issue, now all syn entries are freed
when the timer fires (I tested with nemesis flooding a test system with
syn entries). the remaning issue would be that the pool_cache_put is
delayed by several seconds, this could maybe cause memory starvation.

> > 
> > BTW, there is a comment in syn_cache_timer() saying "calls pool_put but
> > see spl above", but syn_cache_timer() has no spl calls. Should it raise
> > the IPL to splsoftnet() ?
> 
> This remains ...

I also added splsoftnet calls before pool_put().

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

Index: tcp_input.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/tcp_input.c,v
retrieving revision 1.291.4.2
diff -u -p -u -r1.291.4.2 tcp_input.c
--- tcp_input.c 26 Sep 2009 18:34:29 -0000      1.291.4.2
+++ tcp_input.c 19 Apr 2010 14:27:13 -0000
@@ -3333,7 +3333,6 @@ syn_cache_rm(struct syn_cache *sc)
        sc->sc_tp = NULL;
        LIST_REMOVE(sc, sc_tpq);
        tcp_syn_cache[sc->sc_bucketidx].sch_length--;
-       callout_stop(&sc->sc_timer);
        syn_cache_count--;
 }
 
@@ -3343,12 +3342,7 @@ syn_cache_put(struct syn_cache *sc)
        if (sc->sc_ipopts)
                (void) m_free(sc->sc_ipopts);
        rtcache_free(&sc->sc_route);
-       if (callout_invoking(&sc->sc_timer))
-               sc->sc_flags |= SCF_DEAD;
-       else {
-               callout_destroy(&sc->sc_timer);
-               pool_put(&syn_cache_pool, sc);
-       }
+       sc->sc_flags |= SCF_DEAD;
 }
 
 void
@@ -3467,6 +3461,7 @@ void
 syn_cache_timer(void *arg)
 {
        struct syn_cache *sc = arg;
+       int s;
 
        mutex_enter(softnet_lock);
        KERNEL_LOCK(1, NULL);
@@ -3475,7 +3470,9 @@ syn_cache_timer(void *arg)
        if (__predict_false(sc->sc_flags & SCF_DEAD)) {
                TCP_STATINC(TCP_STAT_SC_DELAYED_FREE);
                callout_destroy(&sc->sc_timer);
+               s = splsoftnet();
                pool_put(&syn_cache_pool, sc);
+               splx(s);
                KERNEL_UNLOCK_ONE(NULL);
                mutex_exit(softnet_lock);
                return;
@@ -3509,7 +3506,13 @@ syn_cache_timer(void *arg)
  dropit:
        TCP_STATINC(TCP_STAT_SC_TIMED_OUT);
        syn_cache_rm(sc);
-       syn_cache_put(sc);      /* calls pool_put but see spl above */
+       if (sc->sc_ipopts)
+               (void) m_free(sc->sc_ipopts);
+       rtcache_free(&sc->sc_route);
+       callout_destroy(&sc->sc_timer);
+       s = splsoftnet();
+       pool_put(&syn_cache_pool, sc);
+       splx(s);
        KERNEL_UNLOCK_ONE(NULL);
        mutex_exit(softnet_lock);
 }