Hello!
> If you want to say "Andrew, TCP is safe" then that's cool.
It is supposed to be safe. Please, audit all, but do not spin
too long on one place. 8)
You have already found bugs in ipv{4,6} defragmenters, big dusty hole
in net/sched and bug in igmp (see below).
> handler()
> {
> foo = 1;
> }
>
> mainline()
> {
> del_timer_async(&timer);
> foo = 2;
> }
>
> That is a race. So why do you say del_timer_sync() is not needed for
> static timers?
Because del_timer_sync() is useless in such case.
If "foo" is required to be reliable state, you must spinlock,
because mainline is never single "mainline", there are lots of "mainline"s.
F.e. look around your REVIEWME at ipmr.c.
Either "foo" is just a hint, like some variables in various
garbage collectors and synchronization is not required at all.
The only case, when del_timer_sync() really helps, is when
"mainline" is serialized by kernel lock or a sleeping semaphore
(plus assumption that timers form single thread, which is correct,
but I would not rely on this, otherwise future delevpoment will
be only more difficult) In linux/net/ it occurs only in some control
paths (dev->open etc.). That's why del_timer_sync() is almost
never useful there.
But it is very useful in the areas, which rely on kernel lock,
inside networking device drivers et al.
> Where are the refcounts held?
It is sk->refcnt. Each reference to socket is counted, including
running timers. Socket is destroyed, when the last reference disappears.
Advantage of the scheme --- no points of synchronization
(except for timer implementation, but it can be changed f.e. by using
separate per-cpu timer pools). This property must be hold.
But main advantage is that it is evidently free of any kind of
deadlocks.
Drawback --- trashing sk->refcnt. If we found a way to use
something similar del_timer_sync() to call this before socket
destruction, but insensitive to socket lock, we could avoid
lots of useless work... But I still do not know how to make this
not introducing new synchronization points.
> // Race here.
Right! spin_lock(&im->lock) is forgotten in timer handler. Good spot.
> im->reporter = 1;
> ip_ma_put(im);
> }
Alexey
|