On Mon, 16 Aug 2004 20:51:31 +1000
Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> > > CPU0 CPU1
> > > neigh_create
> > > inet_del_ifa
> > > notifier_call_chain
> > > neigh_ifdown
> > > inetdev_destroy
> > > arp_constructor
> > > neigh->parms =
> > > in_dev->arp_parms
> > > in_dev->dead = 1
> > > in_dev->dev->ip_ptr =
> > > NULL
> > > neigh_parms_release
> > > n->parms->neigh_setup => BUG
> > Is there anything other than hostess_sv11.c, sealevel.c, and shaper.c
> > which are using n->parms->neigh_setup at all?
> > This seems to be a very obscure special case hack, which perhaps we
> > can removee entirely.
> That maybe the case, but the race has nothing to do with neigh_setup.
> Even if you remove neigh_setup altogether, the very next line in
> neigh_create will dereference n->parms by looking up base_reachable_time.
Wait a second, how can neigh_ifdown() even find this thing?
Firstly, neigh_create() takes a reference to the device, which
in turn holds onto the inetdev preventing inetdev_destroy().
Secondly, until neigh_create() takes the tbl lock, it is not in
the hash tables and therefore neigh_ifdown() could not see it.
Thirdly, arp_constructor() does in_dev_get() and checks the
return value. If it fails, by racing with inetdev_destroy(),
neigh_create() will return an error and not do bogus derefing.
I think that covers all the cases, right?
(please prove me wrong, this looks too easy :-)