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

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
> _at__at_ -398,7 +398,7 _at__at_
>         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