Re: CVS commit: src/sys/netinet6

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Tue, Dec 26, 2017 at 12:37 PM, Paul Goyette <paul%whooppee.com@localhost> wrote:
> On Tue, 26 Dec 2017, Ryota Ozaki wrote:
>
>> On Tue, Dec 26, 2017 at 11:35 AM, Paul Goyette <paul%whooppee.com@localhost> wrote:
>>>
>>>
>>>> To generate a diff of this commit:
>>>
>>>
>>>
>>> # cvs rdiff -u -r1.139 -r1.140 src/sys/netinet6/nd6_nbr.c
>>> @@ -1097,7 +1097,11 @@ nd6_dad_stoptimer(struct dadq *dp)
>>>  #ifdef NET_MPSAFE
>>>         callout_halt(&dp->dad_timer_ch, NULL);
>>>  #else
>>> -       callout_halt(&dp->dad_timer_ch, softnet_lock);
>>> +       /* XXX still need the trick for softnet_lock */
>>> +       if (mutex_owned(softnet_lock))
>>> +               callout_halt(&dp->dad_timer_ch, softnet_lock);
>>> +       else
>>> +               callout_halt(&dp->dad_timer_ch, NULL);
>>>  #endif
>>>  }
>>>
>>> This goes against the restriction noted in the mutex(9) man page:
>>>
>>>    [mutex_owned()] should not be used to make locking decisions at run
>>>    time.  ...
>>>
>>> Please find a different way to make this run-time decision.
>>
>>
>> I know the restriction well, but for softnet_lock, following the
>> restriction
>> is sometimes hard. I don't have an option to statically decide if
>> softnet_lock is held or not without messing up many codes.
>>
>> An option we can have here is to give up using callout_halt and use
>> callout_stop that may be unsafe.
>>
>> Which is better for us?
>
>
> Well, since the lock _might_ be released (and subsequently reacquired)
> by callout_halt(), it might be easiest to modify all the callers to
> just unlock it before calling nd6_dad_stoptimer(), and reacquire the
> mutex after it returns (as well as re-establishing any values that
> might have changed while the mutex was released).
>
> The callers needs to be prepared for the release/reacquire situation
> anyway, so the change should not be significant.  As noted in the
> callout_halt(9) man page
>
>    ...to avoid race conditions the caller should always assume that
>    [the] interlock has been released and reacquired, and act
>    accordingly.
>
>
> Alternatively, you could modify all the callers to always acquire the
> mutex before calling nd6_dad_stoptimer().

I mean such changes are tough and mess up many codes which we don't hope.

>
> But making a run-time decision with mutex_owned() is not a good idea.

If it must be respected we can back to callout_stop because it's unsafe
but NetBSD 7 uses it without any issues; using callout_stop in
nd6_dad_stoptimer might be safe somehow (if not NET_MPSAFE).

  ozaki-r