netdev
[Top] [All Lists]

Re: neigh_create/inetdev_destroy race?

To: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Subject: Re: neigh_create/inetdev_destroy race?
From: "David S. Miller" <davem@xxxxxxxxxxxxx>
Date: Sat, 28 Aug 2004 23:42:01 -0700
Cc: shemminger@xxxxxxxx, netdev@xxxxxxxxxxx
In-reply-to: <20040816105131.GA11299@xxxxxxxxxxxxxxxxxxx>
References: <20040813215602.GA15870@xxxxxxxxxxxxxxxxxxx> <20040813151923.3311b4f0.davem@xxxxxxxxxx> <20040814003428.GA17760@xxxxxxxxxxxxxxxxxxx> <20040813173924.6d05be15.davem@xxxxxxxxxx> <20040814005411.GA18350@xxxxxxxxxxxxxxxxxxx> <20040814012513.GA721@xxxxxxxxxxxxxxxxxxx> <20040814013030.GA2042@xxxxxxxxxxxxxxxxxxx> <20040814050848.GA11874@xxxxxxxxxxxxxxxxxxx> <20040814062703.GA4806@xxxxxxxxxxxxxxxxxxx> <20040815191450.77532d5d.davem@xxxxxxxxxx> <20040816105131.GA11299@xxxxxxxxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
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 :-)

<Prev in Thread] Current Thread [Next in Thread>