On Sat, Aug 28, 2004 at 11:42:01PM -0700, David S. Miller wrote:
> 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
>
> > 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().
I'm not sure about this statement. I can't see how holding a reference
on dev prevents you from deleting the primary address on dev which will
lead to the call chain on the right.
> Secondly, until neigh_create() takes the tbl lock, it is not in
> the hash tables and therefore neigh_ifdown() could not see it.
That part I agree with :) That's in fact what this race is about:
neigh_ifdown does not guarantee that the hash table will be without
references to idev.
> 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.
In the above scenario, when arp_constructor() runs the in_dev is
still alive and well. It only gets destroyed afterwards.
> I think that covers all the cases, right?
> (please prove me wrong, this looks too easy :-)
Not quite :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@xxxxxxxxxxxxxxxxxxx>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
|