Re: CVS commit: src/sys/netinet6

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

Hi,

I'm going to change to use callout_stop because it seems using it
is almost harmless in practical. See the below explanation (tl;dr).


I investigated how using callout_stop affects and figured out
it can be problematic but the probability is quite low.
(That's why NetBSD 7 and earlier using callout_stop have worked
without any problems until now.)

The issue of using callout_stop (not callout_halt) is that
callout_stop doesn't wait for the running(*) callout handler to
finish. In DAD cases, DAD-related data (struct dadq, dp) are
freed after callout_stop so the callout handler could
use-after-free it. However, the handler doesn't access dp where
callout_stop is called; the handler is passed ifa (not dp) and
it looks up dp by ifa (**). At the point that callout_stop is
called, the target dp is removed from the global list and the
handler fails to look up and returns with doing nothing.

(*) If the callout is scheduled but the handler isn't dispatched
  when calling callout_stop, it's just canceled.
(**) dp has a pointer to ifa (dp->dad_ifa)

One issue is on using a passed ifa. It can be a dangling pointer
during executing the callout handler because the handler doesn't
guarantee that the ifa isn't freed. Fortunately the handler uses
the ifa only as an address (to look up dp), which is harmless.
One possible problem of touching an ifa which may point a freed
memory area is when the area is reused as another ifa (ifa~).
In that case a wrong dp that points ifa~ can be looked up in the
callout handler, which causes unexpected behaviors.

I estimate that that happens in theory but is unlikely to happen
in practical. Am I wrong?


Note that of course I agree that using callout_halt is the way
to go (actually it's used in the NET_MPSAFE case) and using
callout_stop is just a temporal solution.

  ozaki-r