On Wed, 30 Aug 2006 03:21:26 +0400
Alexey Kuznetsov <[EMAIL PROTECTED]> wrote:
> Hello!
>
> > This should not be any more racy than the existing code.
>
> Existing code is not racy.
Race 1: w/o RCU
Cpu 0: is in neigh_lookup
gets read_lock()
finds entry
++refcount to 2
updates it
Cpu 1: is in forced_gc()
waits at write_lock()
releases read_lock()
drops ref count to 1.
sees ref count is 1
deletes it
Race 1: w RCU
Cpu 0: is in __neigh_lookup
updates it
Cpu 1: is in forced_gc()
leaves refcount=1
sees ref count is 1
deletes it
>
> Critical place is interpretation of refcnt==1. Current code assumes,
> that when refcnt=1 and entry is in hash table, nobody can take this
> entry (table is locked). So, it can be unlinked from the table.
>
> See? __neigh_lookup()/__neigh_lookup_errno() _MUST_ return alive
> and hashed entry. And will stay hashed until the reference is held.
> Or until neigh entry is forced for destruction by device going down,
> in this case referring dst entries will die as well.
Why must it be hashed, it could always get zapped just after the update.
> If dst cache grabs an entry, which is purged from table because for some
> time it had refcnt==1, you got a valid dst entry referring to dead
> neighbour entry.
Hmm.. Since this is a slow path, grabbing the write_lock on the neighbour
entry would block the gc from zapping it.
in __neigh_lookup()
neigh_hold();
write_lock(&n->lock);
if (n->dead) {
write_unlock()
neigh_release()
goto rescan;
}
write_unlock()
--
Stephen Hemminger <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
