On Fri, Sep 03, 2004 at 09:00:53AM -0700, Stephen Hemminger wrote:
>
> > You'll also notice that I've put all dereferences of dev->*_ptr under
> > the rcu_read_lock(). Without this we may get a neigh_parms that's
> > already been released.
>
> I haven't looked at the exact code in detail, but don't you need
> use rcu_dereference() as well to make sure and get the
> smp_read_barrier_depends
> on Alpha.
Not really because we're not depending on *dev->neigh_parms to be set to
NULLon shutdown. In fact *dev->neigh_parms never gets set to NULL at all.
If it did we'd have trouble cleaning up dead entries from the hash table.
So there is no data-dependent read here whose order must be preserved
when *dev is destroyed.
But hang on a second, I had forgotten about the creation path. Indeed
that is buggy without a barrier for every path except IPv6. Without
the barrier, we may be reading NULL pointers from parms which may
result in stale neigh entries lingering around. Or worse we may read
complete garbage that was there before the memset on *dev was done.
Fortunately the last bit probably can only be triggered if you're
stepping through gdb :)
So here is a patch to make sure that there is a barrier between the
reading of dev->*_ptr and *dev->neigh_parms.
With these barriers in place, it's clear that *dev->neigh_parms can no
longer be NULL since once the parms are allocated, that pointer is never
reset to NULL again. Therefore I've also removed the parms check in
these paths.
They were bogus to begin with since if they ever triggered then we'll
have dead neigh entries stuck in the hash table.
Unfortunately I couldn't arrange for this to happen with DECnet due
to the dn_db->parms.up() call that's sandwiched between the assignment
of dev->dn_ptr and dn_db->neigh_parms. So I've kept the parms check
there but it will now fail instead of continuing. I've also added an
smp_wmb() there so that at least we won't be reading garbage from
dn_db->neigh_parms.
DECnet is also buggy since there is no locking at all in the destruction
path. It either needs locking or RCU like IPv4.
Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
Thanks a lot,
--
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
p
Description: Text document
|