kern/88725: /usr/sbin/ppp panic with 2005.10.21 netinet6 changes

[Victor Snezhko]

The following reply was made to PR kern/88725; it has been noted by GNATS.

From: Victor Snezhko <snezhko at indorsoft.ru>
To: John Baldwin <jhb at freebsd.org>
Cc: freebsd-current at freebsd.org,  SUZUKI Shinsuke <suz at freebsd.org>,
	  snezhko at indorsoft.ru,  max at love2party.net,  bug-followup at freebsd.org
Subject: Re: kern/88725: /usr/sbin/ppp panic with 2005.10.21 netinet6
 changes
Date: Fri, 11 Nov 2005 15:09:36 +0600

 John Baldwin <jhb at freebsd.org> writes:
 
 >>> Mark Tinguely has found the offending timer.
 >>> The following patch fixes the problem for me:
 >>
 >> Thanks.  sounds right for me.
 >> So please commit it if when you've finished the test with fresh -current.
 >
 > As a general rule you should be using callout_drain() before freeing a callout 
 > to handle the race condition where the callout is running on another CPU (so 
 > callout_stop can't stop it) while you are freeing it.  Note that you can not 
 > use callout_drain() if you are holding any locks, though.  In those cases you 
 > will need to defer the callout_drain() and free() until you have dropped the 
 > locks.  Here's one example fix:
 >
 > Index: nd6.c
 > ===================================================================
 > RCS file: /usr/cvs/src/sys/netinet6/nd6.c,v
 > retrieving revision 1.62
 > diff -u -r1.62 nd6.c
 > --- nd6.c       22 Oct 2005 05:07:16 -0000      1.62
 > +++ nd6.c       3 Nov 2005 19:56:42 -0000
 > @@ -398,7 +398,7 @@
 >         if (tick < 0) {
 >                 ln->ln_expire = 0;
 >                 ln->ln_ntick = 0;
 > -               callout_stop(&ln->ln_timer_ch);
 > +               callout_drain(&ln->ln_timer_ch);
 >         } else {
 >                 ln->ln_expire = time_second + tick / hz;
 >                 if (tick > INT_MAX) {
 
 The code that was committed (and introduced armed timer that was
 freed) is full of callout_stops and contains not a single
 callout_drain.
 
 So I think in order to be consistent we shouldn't fix two problems at
 once. The right way would be to commit the fix with callout stop at
 first (and close the PR) and then investigate whether we can replace
 stops with drains without introducing a deadlock (for each timer
 separately). In this case we will at least have a working system to
 cvsdown to it if there will be issues with callout_drain.
 
 I have tested the patch by Mark Tinguely (that fixes mld6.c) on the
 fresh -current (cvsupped ~2 days ago), it works there too
 (unsurprisingly). So it may be committed, I suppose (without the debug
 printf, of course). 
 
 -- 
 WBR, Victor V. Snezhko
 EMail: snezhko at indorsoft.ru