From: David S. Miller <hidden> Date: 2004-08-12 23:59:54
[ Robert, Jamal, Alexey, the previous version I sent you guys
privately early today had a minor bug, in inet_free_ifa()
we now need to use in_dev_put() instead of __in_dev_put() ]
The main motivation for this was to make fib_validate_source()
cheaper, as currently it needs a global lock in order to
access the inet device interface lists.
This makes is all use RCU.
I kept the non-RCU lock usage in multicast address list
handling in net/ipv4/igmp.c, but that could use RCU as
well if we wanted to.
While doing this I noticed that devinet.c had these two
counters (inet_ifa_count and inet_dev_count) which were
updated but nobody ever read, so these got killed.
Someone poke holes in this patch please :-)
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/08/12 16:42:07-07:00 davem@nuts.davemloft.net
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# Multicast ipv4 address handling still uses rwlock
# and spinlock synchronization.
#
# Signed-off-by: David S. Miller [off-list ref]
#
# net/sctp/protocol.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +3 -5
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/irda/irlan/irlan_eth.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +2 -2
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/ipv4/route.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +3 -3
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/ipv4/igmp.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +47 -45
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/ipv4/icmp.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +2 -2
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/ipv4/fib_frontend.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +2 -2
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# net/ipv4/devinet.c
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +49 -63
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
# include/linux/inetdevice.h
# 2004/08/12 16:41:13-07:00 davem@nuts.davemloft.net +17 -11
# [IPV4]: Move inetdev/ifa locking over to RCU.
#
diff -Nru a/include/linux/inetdevice.h b/include/linux/inetdevice.h
@@ -88,12 +88,9 @@staticvoiddevinet_sysctl_unregister(structipv4_devconf*p);#endif-intinet_ifa_count;-intinet_dev_count;-/* Locks all the inet devices. */-rwlock_tinetdev_lock=RW_LOCK_UNLOCKED;+staticspinlock_tinetdev_lock=SPIN_LOCK_UNLOCKED;staticstructin_ifaddr*inet_alloc_ifa(void){
@@ -188,16 +189,16 @@while((ifa=in_dev->ifa_list)!=NULL){inet_del_ifa(in_dev,&in_dev->ifa_list,0);-inet_free_ifa(ifa);+call_rcu(&ifa->rcu_head,inet_rcu_free_ifa);}#ifdef CONFIG_SYSCTLdevinet_sysctl_unregister(&in_dev->cnf);#endif-write_lock_bh(&inetdev_lock);+spin_lock_bh(&inetdev_lock);in_dev->dev->ip_ptr=NULL;/* in_dev_put following below will kill the in_device */-write_unlock_bh(&inetdev_lock);+spin_unlock_bh(&inetdev_lock);#ifdef CONFIG_SYSCTLneigh_sysctl_unregister(in_dev->arp_parms);
@@ -1033,7 +1034,7 @@kfree(pmc);}/* clear dead sources, too */-read_lock(&in_dev->lock);+read_lock(&in_dev->mc_list_lock);for(pmc=in_dev->mc_list;pmc;pmc=pmc->next){structip_sf_list*psf,*psf_next;
@@ -148,13 +148,12 @@structin_ifaddr*ifa;structsctp_sockaddr_entry*addr;-read_lock(&inetdev_lock);+rcu_read_lock();if((in_dev=__in_dev_get(dev))==NULL){-read_unlock(&inetdev_lock);+rcu_read_unlock();return;}-read_lock(&in_dev->lock);for(ifa=in_dev->ifa_list;ifa;ifa=ifa->ifa_next){/* Add the address to the local list. */addr=t_new(structsctp_sockaddr_entry,GFP_ATOMIC);
@@ -166,8 +165,7 @@}}-read_unlock(&in_dev->lock);-read_unlock(&inetdev_lock);+rcu_read_unlock();}/* Extract our IP addresses from the system and stash them in the
This doesn't look right. The only thing in_dev_get() does in
the critical section is incrementing the refcnt.
But here we're only delaying the destruction of the idev rather
than the dropping of the refcnt. So the following race can occur
with preemption:
CPU0 CPU1
in_dev_get
rcu_read_lock
in_dev = dev->ip_ptr
inetdev_destroy
dev->ip_ptr = NULL
in_dev_put
--refcnt == 0
call_rcu
refcnt++
rcu_read_unlock
PREEMPT
sched point
in_dev_rcu_destroy
use in_dev => BUG
The obvious thing to do is to move the call_rcu into inetdev_destroy
and get it to call in_dev_put.
But we still need to make sure that nobody increments the count again
without checking in_dev->dev->ip_ptr != NULL.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
@@ -88,12 +88,9 @@staticvoiddevinet_sysctl_unregister(structipv4_devconf*p);#endif-intinet_ifa_count;-intinet_dev_count;-/* Locks all the inet devices. */-rwlock_tinetdev_lock=RW_LOCK_UNLOCKED;+staticspinlock_tinetdev_lock=SPIN_LOCK_UNLOCKED;staticstructin_ifaddr*inet_alloc_ifa(void){
@@ -188,16 +189,16 @@while((ifa=in_dev->ifa_list)!=NULL){inet_del_ifa(in_dev,&in_dev->ifa_list,0);-inet_free_ifa(ifa);+call_rcu(&ifa->rcu_head,inet_rcu_free_ifa);}#ifdef CONFIG_SYSCTLdevinet_sysctl_unregister(&in_dev->cnf);#endif-write_lock_bh(&inetdev_lock);+spin_lock_bh(&inetdev_lock);in_dev->dev->ip_ptr=NULL;/* in_dev_put following below will kill the in_device */-write_unlock_bh(&inetdev_lock);+spin_unlock_bh(&inetdev_lock);#ifdef CONFIG_SYSCTLneigh_sysctl_unregister(in_dev->arp_parms);
@@ -1033,7 +1034,7 @@kfree(pmc);}/* clear dead sources, too */-read_lock(&in_dev->lock);+read_lock(&in_dev->mc_list_lock);for(pmc=in_dev->mc_list;pmc;pmc=pmc->next){structip_sf_list*psf,*psf_next;
@@ -148,13 +148,12 @@structin_ifaddr*ifa;structsctp_sockaddr_entry*addr;-read_lock(&inetdev_lock);+rcu_read_lock();if((in_dev=__in_dev_get(dev))==NULL){-read_unlock(&inetdev_lock);+rcu_read_unlock();return;}-read_lock(&in_dev->lock);for(ifa=in_dev->ifa_list;ifa;ifa=ifa->ifa_next){/* Add the address to the local list. */addr=t_new(structsctp_sockaddr_entry,GFP_ATOMIC);
@@ -166,8 +165,7 @@}}-read_unlock(&in_dev->lock);-read_unlock(&inetdev_lock);+rcu_read_unlock();}/* Extract our IP addresses from the system and stash them in the
From: David S. Miller <hidden> Date: 2004-08-13 16:38:38
Thanks guys. This patch should fix both problems.
Herbert, if we check inetdev->dead when trying to grab
a reference it fixes the RCU destroy race you mentioned.
I really don't want to put that ref drop into the
RCU callback as that would kill performance.
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/08/13 09:17:07-07:00 davem@nuts.davemloft.net
# [IPV4]: Fix two bugs in inetdev RCU handling.
#
# - Two missing smp_read_barrier_depends() noticed by
# Stephen Hemminger.
# - Fix RCU inetdev destroy race spotted by Herbert Xu.
# Check in_dev->dead when trying to grab a reference.
#
# Signed-off-by: David S. Miller [off-list ref]
#
# net/ipv4/devinet.c
# 2004/08/13 09:15:41-07:00 davem@nuts.davemloft.net +1 -0
# [IPV4]: Fix two bugs in inetdev RCU handling.
#
# include/linux/inetdevice.h
# 2004/08/13 09:15:41-07:00 davem@nuts.davemloft.net +7 -2
# [IPV4]: Fix two bugs in inetdev RCU handling.
#
diff -Nru a/include/linux/inetdevice.h b/include/linux/inetdevice.h
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-13 21:56:02
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.
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;
}
From: David S. Miller <hidden> Date: 2004-08-13 22:19:23
On Sat, 14 Aug 2004 07:56:02 +1000
Herbert Xu [off-list ref] wrote:
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.
Ok I see now. I've added your patch, it seems correct.
From: David S. Miller <hidden> Date: 2004-08-14 00:39:24
On Sat, 14 Aug 2004 10:34:28 +1000
Herbert Xu [off-list ref] wrote:
quoted
Ok I see now. I've added your patch, it seems correct.
Thanks. I presume you've reversed the previous patch as well?
Yes and I cured the one typo in it two, idev-->in_dev :-)
BTW, it looks like you can remove inetdev_lock altogether. All its
users already assume that they don't race against each other by
taking the rtnl lock.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-14 01:25:13
Hi Dave:
I gave your patch to the compiler and it doesn't like it :)
call_rcu needs three arguments, not two.
Watch out, this will probably conflict with your in_dev typo fix.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-14 05:08:48
Hi:
I'm just going through all the __in_dev_get() callers and the one
in ip_route_output_slow() looks fishy. It appears to be checking
whether the subsequent inet_select_addr() calls will succeed or not.
But this is not reliable since the addresses can always disappear
between the check and the actual call.
Do we really care about the zero return value of inet_select_addr()
here? What about the other calls to inet_select_addr()?
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-14 06:27:03
Hi:
I'm going through in_dev_get() callers. The call in arp_constructor()
looks racy. It takes in_dev->arp_parms and stores it in neigh->parms.
Is there any thing that prevents the following scenario from occuring?
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
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: David S. Miller <hidden> Date: 2004-08-16 02:08:23
On Sat, 14 Aug 2004 15:08:48 +1000
Herbert Xu [off-list ref] wrote:
I'm just going through all the __in_dev_get() callers and the one
in ip_route_output_slow() looks fishy. It appears to be checking
whether the subsequent inet_select_addr() calls will succeed or not.
But this is not reliable since the addresses can always disappear
between the check and the actual call.
Do we really care about the zero return value of inet_select_addr()
here? What about the other calls to inet_select_addr()?
It won't return zero, typically it will return loopback's IP
(with preference to any non-loopback addresses assigned to
the loopback device). This is being used for source address
selection.
Also, when device ipv4 addresses are deleted, NETDEV_DOWN messages
are broadcast to all the subsystems. One of the subsystems is FIB,
which will disable IP on that interface if this is the last ipv4
address and it will also flush the routing cache immediately.
However you are right that we may need to synchronize this more
tightly.
Hmmm....
From: David S. Miller <hidden> Date: 2004-08-16 02:14:50
On Sat, 14 Aug 2004 16:27:03 +1000
Herbert Xu [off-list ref] wrote:
Is there any thing that prevents the following scenario from occuring?
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.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-16 02:43:30
On Sun, Aug 15, 2004 at 07:08:23PM -0700, David S. Miller wrote:
quoted
Do we really care about the zero return value of inet_select_addr()
here? What about the other calls to inet_select_addr()?
It won't return zero, typically it will return loopback's IP
(with preference to any non-loopback addresses assigned to
the loopback device). This is being used for source address
selection.
From: David S. Miller <hidden> Date: 2004-08-16 02:58:20
On Sat, 14 Aug 2004 10:34:28 +1000
Herbert Xu [off-list ref] wrote:
BTW, it looks like you can remove inetdev_lock altogether. All its
users already assume that they don't race against each other by
taking the rtnl lock.
I think it's needed at least for the:
dev->ip_ptr = idev;
in_dev_get(idev);
thing, isn't it?
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-16 03:08:26
On Sun, Aug 15, 2004 at 07:58:20PM -0700, David S. Miller wrote:
On Sat, 14 Aug 2004 10:34:28 +1000
Herbert Xu [off-list ref] wrote:
quoted
BTW, it looks like you can remove inetdev_lock altogether. All its
users already assume that they don't race against each other by
taking the rtnl lock.
I think it's needed at least for the:
dev->ip_ptr = idev;
in_dev_get(idev);
You mean in_dev_hold?
thing, isn't it?
I don't think so. But there is a real bug here :)
The lock doesn't really do much since everyone who's holding
it (they're all in devinet.c) already hold the RTNL lock
(the rotten old lock perhaps :)
But the RCU change has created a real bug here. Imagine this:
CPU0 CPU1
inetdev_init
dev->ip_ptr = idev
in_dev_get
atomic_inc on refcnt
in_dev_put => frees idev
in_dev_hold(idev) => BUG
So we should reverse the two statements and add an smp_wmb().
Unfortunately the lock doesn't really help you at all.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: David S. Miller <hidden> Date: 2004-08-16 03:08:38
On Mon, 16 Aug 2004 12:43:30 +1000
Herbert Xu [off-list ref] wrote:
It will return zero if there is no in_dev at all. Perhaps what
we should do is get inet_select_addr() to get the address from
other devices in that case as well?
It does that now, look at how it iterates over dev_base's
list.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-16 03:14:56
On Sun, Aug 15, 2004 at 08:08:38PM -0700, David S. Miller wrote:
On Mon, 16 Aug 2004 12:43:30 +1000
Herbert Xu [off-list ref] wrote:
quoted
It will return zero if there is no in_dev at all. Perhaps what
we should do is get inet_select_addr() to get the address from
other devices in that case as well?
It does that now, look at how it iterates over dev_base's
list.
From: David S. Miller <hidden> Date: 2004-08-16 06:21:53
On Mon, 16 Aug 2004 13:08:26 +1000
Herbert Xu [off-list ref] wrote:
So we should reverse the two statements and add an smp_wmb().
I totally agree, and let's kill the lock too since it is
in fact useless now. I've done this as follows:
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2004/08/15 23:08:21-07:00 davem@nuts.davemloft.net
# [IPV4]: Kill inetdev_lock, no longer needed.
#
# It no longer protects anything, all users held RTNL
# semaphore to boot. Also, fix a potential race in the
# new RCU inetdev code, grab the reference on the idev
# before attaching it via dev->ip_ptr.
#
# Based upon discussions with Herbert Xu.
#
# Signed-off-by: David S. Miller [off-list ref]
#
# net/ipv4/devinet.c
# 2004/08/15 23:07:17-07:00 davem@nuts.davemloft.net +6 -14
# [IPV4]: Kill inetdev_lock, no longer needed.
#
diff -Nru a/net/ipv4/devinet.c b/net/ipv4/devinet.c
@@ -90,8 +90,6 @@/* Locks all the inet devices. */-staticspinlock_tinetdev_lock=SPIN_LOCK_UNLOCKED;-staticstructin_ifaddr*inet_alloc_ifa(void){structin_ifaddr*ifa=kmalloc(sizeof(*ifa),GFP_KERNEL);
@@ -158,11 +156,12 @@neigh_sysctl_register(dev,in_dev->arp_parms,NET_IPV4,NET_IPV4_NEIGH,"ipv4",NULL);#endif-spin_lock_bh(&inetdev_lock);-dev->ip_ptr=in_dev;+/* Account for reference dev->ip_ptr */in_dev_hold(in_dev);-spin_unlock_bh(&inetdev_lock);+smp_wmb();+dev->ip_ptr=in_dev;+#ifdef CONFIG_SYSCTLdevinet_sysctl_register(in_dev,&in_dev->cnf);#endif
@@ -201,10 +200,8 @@#ifdef CONFIG_SYSCTLdevinet_sysctl_unregister(&in_dev->cnf);#endif-spin_lock_bh(&inetdev_lock);+in_dev->dev->ip_ptr=NULL;-/* in_dev_put following below will kill the in_device */-spin_unlock_bh(&inetdev_lock);#ifdef CONFIG_SYSCTLneigh_sysctl_unregister(in_dev->arp_parms);
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-16 10:51:31
On Sun, Aug 15, 2004 at 07:14:50PM -0700, David S. Miller wrote:
On Sat, 14 Aug 2004 16:27:03 +1000
Herbert Xu [off-list ref] wrote:
quoted
Is there any thing that prevents the following scenario from occuring?
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.
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 :-)
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 :-)
From: "David S. Miller" <davem@davemloft.net> Date: 2004-08-31 06:08:20
On Sun, 29 Aug 2004 16:50:31 +1000
Herbert Xu [off-list ref] wrote:
quoted
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.
I think we can clear this by putting neigh_parms_release() into an
RCU handler. It can't be in in_dev_rcu_put.
I also now believe that these sorts of races you are mentioning percolate
out into ip_mc_destroy_dev().
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-08-31 10:41:39
On Mon, Aug 30, 2004 at 11:08:20PM -0700, David S. Miller wrote:
On Sun, 29 Aug 2004 16:50:31 +1000
Herbert Xu [off-list ref] wrote:
quoted
quoted
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.
I think we can clear this by putting neigh_parms_release() into an
RCU handler. It can't be in in_dev_rcu_put.
Yes that should go a long way in resolving this problem. However it
is still tricky because the neighbour table doesn't refer to idev
directly. So we'll need to go through contortions to make sure that
the corresponding idev is still alive when we add a neighbour with
its neigh_parms to the hash table. Or better yet we should ref count
the neigh_parms directly.
I also now believe that these sorts of races you are mentioning percolate
out into ip_mc_destroy_dev().
Well last time I looked at it I concluded that it was safe. It seems
that the references are either held by timers who are all taken down
at the very start of the destruction, or they're taken at places which
are under spin locks.
Did I miss something else?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: "David S. Miller" <davem@davemloft.net> Date: 2004-09-02 05:21:18
On Tue, 31 Aug 2004 20:41:39 +1000
Herbert Xu [off-list ref] wrote:
quoted
I think we can clear this by putting neigh_parms_release() into an
RCU handler. It can't be in in_dev_rcu_put.
Yes that should go a long way in resolving this problem.
So here's the first step. No rcu_read_lock()'s are needed
since the tbl->lock needs to be held as a write when
traversing these things anyways for other reasons.
Can you work on the next bit you mentioned, making
sure the corresponding idev is still alive when we add
a neighbour with its neigh_parms to the hash table?
Thanks.
===== include/net/neighbour.h 1.8 vs edited =====
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-09-02 13:06:05
On Wed, Sep 01, 2004 at 10:21:18PM -0700, David S. Miller wrote:
So here's the first step. No rcu_read_lock()'s are needed
since the tbl->lock needs to be held as a write when
traversing these things anyways for other reasons.
Thanks.
Can you work on the next bit you mentioned, making
sure the corresponding idev is still alive when we add
a neighbour with its neigh_parms to the hash table?
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-09-03 13:36:23
On Thu, Sep 02, 2004 at 11:06:05PM +1000, herbert wrote:
quoted
Can you work on the next bit you mentioned, making
sure the corresponding idev is still alive when we add
a neighbour with its neigh_parms to the hash table?
Sure. Actually I prefer to do it by ref counting neigh_parms directly.
I'll send you a patch soon.
Here is the patch.
I've added a refcnt on neigh_parms as well as a dead flag. The latter
is checked under the tbl_lock before adding a neigh entry to the hash
table.
The non-trivial bit of the patch is the first chunk of net/core/neighbour.c.
I removed that line because not doing so would mean that I have to drop
the reference to the parms right there. That would've lead to race
conditions since many places dereference neigh->parms without holding
locks. It's also unnecessary to reset n->parms since we're no longer
in a hurry to see it go due to the new ref counting.
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.
Incidentally a lot of these places were racy even before the RCU change.
For example, in the IPv6 case neigh->parms may be set to a value that's
just been released.
Finally in order to make sure that all stale entries are purged as
quickly as possible I've added neigh_ifdown/arp_ifdown calls after
every neigh_parms_release call. In many cases we now have multiple
calls to neigh_ifdown in the shutdown path. I didn't remove the
earlier calls because there may be hidden dependencies for them to
be there. Once the respective maintainers have looked at them we
can probably remove most of them.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
From: Stephen Hemminger <hidden> Date: 2004-09-03 16:00:53
On Fri, 3 Sep 2004 23:36:23 +1000
Herbert Xu [off-list ref] wrote:
On Thu, Sep 02, 2004 at 11:06:05PM +1000, herbert wrote:
quoted
quoted
Can you work on the next bit you mentioned, making
sure the corresponding idev is still alive when we add
a neighbour with its neigh_parms to the hash table?
Sure. Actually I prefer to do it by ref counting neigh_parms directly.
I'll send you a patch soon.
Here is the patch.
I've added a refcnt on neigh_parms as well as a dead flag. The latter
is checked under the tbl_lock before adding a neigh entry to the hash
table.
The non-trivial bit of the patch is the first chunk of net/core/neighbour.c.
I removed that line because not doing so would mean that I have to drop
the reference to the parms right there. That would've lead to race
conditions since many places dereference neigh->parms without holding
locks. It's also unnecessary to reset n->parms since we're no longer
in a hurry to see it go due to the new ref counting.
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.
From: "David S. Miller" <davem@davemloft.net> Date: 2004-09-03 16:18:17
On Fri, 3 Sep 2004 23:36:23 +1000
Herbert Xu [off-list ref] wrote:
Here is the patch.
Looks great. Yes, I see how the existing cases were racey
pre-RCU, it is similar to the sysctl stuff and that area was
truly horrible before Stephen and myself redid how generic
device destruction works.
Patch applied, thanks Herbert.
From: Herbert Xu <herbert@gondor.apana.org.au> Date: 2004-09-03 23:49:41
On Fri, Sep 03, 2004 at 09:00:53AM -0700, Stephen Hemminger wrote:
quoted
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@gondor.apana.org.au>
Thanks a lot,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt