On Fri, Aug 13, 2004 at 09:38:38AM -0700, David S. Miller wrote:
>
> Herbert, if we check inetdev->dead when trying to grab
> a reference it fixes the RCU destroy race you mentioned.
Sorry Dave but that just makes the window smaller. A new
inetdev_destroy() can still start after the in_dev->dead
check and before the inc of the refcnt.
> I really don't want to put that ref drop into the
> RCU callback as that would kill performance.
Maybe I wasn't clear enough about the solution. I've attached
an untested patch below. Hopefully that should make it clearer.
> diff -Nru a/include/linux/inetdevice.h b/include/linux/inetdevice.h
> --- a/include/linux/inetdevice.h 2004-08-13 09:20:01 -07:00
> +++ b/include/linux/inetdevice.h 2004-08-13 09:20:01 -07:00
> @@ -143,9 +143,14 @@
> struct in_device *in_dev;
>
> rcu_read_lock();
> + smp_read_barrier_depends();
This isn't quite right either. It rarely makes sense to have
smp_read_barrier_depends() immediately after an rcu_read_lock()
since it can't be reordered.
Now had you put this barrier between the reading of ip_ptr and
the checking of in_dev->dead, then the race would've been closed.
But I still prefer to move the work into inetdev_destroy.
> in_dev = dev->ip_ptr;
> - if (in_dev)
> - atomic_inc(&in_dev->refcnt);
> + if (in_dev) {
> + if (in_dev->dead)
> + in_dev = NULL;
> + else
> + atomic_inc(&in_dev->refcnt);
> + }
> rcu_read_unlock();
> return in_dev;
> }
> diff -Nru a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> --- a/net/ipv4/devinet.c 2004-08-13 09:20:01 -07:00
> +++ b/net/ipv4/devinet.c 2004-08-13 09:20:01 -07:00
> @@ -210,6 +210,7 @@
> int inet_addr_onlink(struct in_device *in_dev, u32 a, u32 b)
> {
> rcu_read_lock();
> + smp_read_barrier_depends();
> for_primary_ifa(in_dev) {
> if (inet_ifa_match(a, ifa)) {
> if (!b || inet_ifa_match(b, ifa)) {
Ditto.
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
p
Description: Text document
|