Re: KASSERT() in kern_timeout.c

[Andrew Doran <ad%NetBSD.org@localhost>]

On Mon, Apr 19, 2010 at 04:32:07PM +0200, Manuel Bouyer wrote:
> 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().

Most of the spl calls in the protcol processing stack are unneeded.
In particular there is no general need for splsoftnet() calls,
and no need around pool access.  splsoftnet()'s only legitimate
use is in sorecieve() and sosend() where it's an optimization.

> 
> -- 
> 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);
>  }