Re: neigh_create/inetdev_destroy race?

["David S. Miller" <davem@davemloft.net>]

On Mon, 16 Aug 2004 20:51:31 +1000
Herbert Xu <herbert@gondor.apana.org.au> 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 :-)