[PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Gao feng]

in dst_dev_event,we get the still referenced dst entries
from dst_garbage list,and call dst_ifdown to change these
dst entries' device to the net namesapce's lo device.

when we moving a net device(A) to another net namespace,
because free_fib_info_rcu is called after a grace period,
we may call dst_dev_event before free_fib_info_rcu putting
dst_entry into the dst_garbage list.

so in dst_dev_event, we can't see these dst entries through
dst_garbage list, and without changing their device to the
old net namespace's lo device. after a grace period, these
dst entries which dst->dev is device A will in the dst_garbage
list, and the device A will belong to the new net namespcae.

then we exit from this new net namespace, the dst_dev_event
is called again,it will get these dst entries from dst_garbage
list,and call dst_ifdown to hold the new net namespace's lo
device incorrectly and put the device A.

so it will tigger the emg message in netdev_wait_allrefs like
below.
unregister_netdevice: waiting for lo to become free. Usage count = 1

fix this problem by adding rcu_barrier() in dst_dev_event
when event is NETDEV_UNREGISTER.
with this,dst_ifdown will be called after the dst_garbage list
beeing updated.

Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
---
 net/core/dst.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/dst.c b/net/core/dst.c
index 56d6361..38c2199 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -375,6 +375,7 @@ static int dst_dev_event(struct notifier_block *this, unsigned long event,
 
 	switch (event) {
 	case NETDEV_UNREGISTER:
+		rcu_barrier();
 	case NETDEV_DOWN:
 		mutex_lock(&dst_gc_mutex);
 		for (dst = dst_busy_list; dst; dst = dst->next) {
-- 
1.7.7.6

Re: [PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Eric Dumazet]

On Wed, 2012-08-22 at 16:31 +0800, Gao feng wrote:
> in dst_dev_event,we get the still referenced dst entries
> from dst_garbage list,and call dst_ifdown to change these
> dst entries' device to the net namesapce's lo device.
> 
> when we moving a net device(A) to another net namespace,
> because free_fib_info_rcu is called after a grace period,
> we may call dst_dev_event before free_fib_info_rcu putting
> dst_entry into the dst_garbage list.
> 
> so in dst_dev_event, we can't see these dst entries through
> dst_garbage list, and without changing their device to the
> old net namespace's lo device. after a grace period, these
> dst entries which dst->dev is device A will in the dst_garbage
> list, and the device A will belong to the new net namespcae.
> 
> then we exit from this new net namespace, the dst_dev_event
> is called again,it will get these dst entries from dst_garbage
> list,and call dst_ifdown to hold the new net namespace's lo
> device incorrectly and put the device A.
> 
> so it will tigger the emg message in netdev_wait_allrefs like
> below.
> unregister_netdevice: waiting for lo to become free. Usage count = 1
> 
> fix this problem by adding rcu_barrier() in dst_dev_event
> when event is NETDEV_UNREGISTER.
> with this,dst_ifdown will be called after the dst_garbage list
> beeing updated.
> 
> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
> ---
>  net/core/dst.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 56d6361..38c2199 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -375,6 +375,7 @@ static int dst_dev_event(struct notifier_block *this, unsigned long event,
>  
>  	switch (event) {
>  	case NETDEV_UNREGISTER:
> +		rcu_barrier();
>  	case NETDEV_DOWN:
>  		mutex_lock(&dst_gc_mutex);
>  		for (dst = dst_busy_list; dst; dst = dst->next) {


Did you miss http://patchwork.ozlabs.org/patch/176517/  or is this patch
an alternative ?

rcu_barrier() at this place will kill some workloads.

Re: [PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Gao feng]

On Wed, 2012-08-22 at 16:39 +0800, Eric Dumazet wrote:
> On Wed, 2012-08-22 at 16:31 +0800, Gao feng wrote:
>> in dst_dev_event,we get the still referenced dst entries
>> from dst_garbage list,and call dst_ifdown to change these
>> dst entries' device to the net namesapce's lo device.
>>
>> when we moving a net device(A) to another net namespace,
>> because free_fib_info_rcu is called after a grace period,
>> we may call dst_dev_event before free_fib_info_rcu putting
>> dst_entry into the dst_garbage list.
>>
>> so in dst_dev_event, we can't see these dst entries through
>> dst_garbage list, and without changing their device to the
>> old net namespace's lo device. after a grace period, these
>> dst entries which dst->dev is device A will in the dst_garbage
>> list, and the device A will belong to the new net namespcae.
>>
>> then we exit from this new net namespace, the dst_dev_event
>> is called again,it will get these dst entries from dst_garbage
>> list,and call dst_ifdown to hold the new net namespace's lo
>> device incorrectly and put the device A.
>>
>> so it will tigger the emg message in netdev_wait_allrefs like
>> below.
>> unregister_netdevice: waiting for lo to become free. Usage count = 1
>>
>> fix this problem by adding rcu_barrier() in dst_dev_event
>> when event is NETDEV_UNREGISTER.
>> with this,dst_ifdown will be called after the dst_garbage list
>> beeing updated.
>>
>> Signed-off-by: Gao feng <gaofeng@cn.fujitsu.com>
>> ---
>>  net/core/dst.c |    1 +
>>  1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/core/dst.c b/net/core/dst.c
>> index 56d6361..38c2199 100644
>> --- a/net/core/dst.c
>> +++ b/net/core/dst.c
>> @@ -375,6 +375,7 @@ static int dst_dev_event(struct notifier_block *this, unsigned long event,
>>  
>>  	switch (event) {
>>  	case NETDEV_UNREGISTER:
>> +		rcu_barrier();
>>  	case NETDEV_DOWN:
>>  		mutex_lock(&dst_gc_mutex);
>>  		for (dst = dst_busy_list; dst; dst = dst->next) {
> 
> 
> Did you miss http://patchwork.ozlabs.org/patch/176517/  or is this patch
> an alternative ?
> 

Hi Eric

I saw your patch and think this patch is clear and doesn't change too much logic.

I test your patch, it not fix this problem.

In my test case,when moving a net device to another net namespace,
Because you patch delete NETDEV_UNREGISTER event from dst_dev_event,
we will just put dst entries into the dst garbage list in event
NETDEV_DOWN,without call dst_ifdown to change these dst entries' device
to the lo device,and now this net device belongs to the new net namespace.

After the net device beeing moved to another net namespace, I rmmod this
net device's driver,this will trigger the new added event NETDEV_UNREGISTER_FINISH,
so in dst_dev_event,we will change these dst entries's device to the new net
namespace's lo device,and this will make the referenct count of the new net namespace's
lo device incorrect. when we exit the new net namespace,this emg message is still exist.

Message from syslogd@Donkey at Aug 22 18:50:13 ...
 kernel:[ 1161.979036] unregister_netdevice: waiting for lo to become free. Usage count = 1

And because net_mutex is locked here,so we can't create new net namespace.

> rcu_barrier() at this place will kill some workloads.
> 

I think this will only add some workloads when unregister a net device.
Do I miss something?

Re: [PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Eric Dumazet]

On Wed, 2012-08-22 at 19:00 +0800, Gao feng wrote:

> Hi Eric
> 
> I saw your patch and think this patch is clear and doesn't change too much logic.
> 
> I test your patch, it not fix this problem.
> 
> In my test case,when moving a net device to another net namespace,
> Because you patch delete NETDEV_UNREGISTER event from dst_dev_event,
> we will just put dst entries into the dst garbage list in event
> NETDEV_DOWN,without call dst_ifdown to change these dst entries' device
> to the lo device,and now this net device belongs to the new net namespace.
> 

Then fix the "moving a net device to another net namespace", instead
of slowing down other common operations.

dev_change_net_namespace() is probably a better place to put your patch

> After the net device beeing moved to another net namespace, I rmmod this
> net device's driver,this will trigger the new added event NETDEV_UNREGISTER_FINISH,
> so in dst_dev_event,we will change these dst entries's device to the new net
> namespace's lo device,and this will make the referenct count of the new net namespace's
> lo device incorrect. when we exit the new net namespace,this emg message is still exist.
> 
> Message from syslogd@Donkey at Aug 22 18:50:13 ...
>  kernel:[ 1161.979036] unregister_netdevice: waiting for lo to become free. Usage count = 1
> 
> And because net_mutex is locked here,so we can't create new net namespace.
> 
> > rcu_barrier() at this place will kill some workloads.
> > 
> 
> I think this will only add some workloads when unregister a net device.
> Do I miss something?

Yes, rcu_barrier() at this point is killing performance, because we hold
RTNL.

We worked hard to batch things, your patch is a huge step backward.

Re: [PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Gao feng]

于 2012年08月22日 19:24, Eric Dumazet 写道:
> On Wed, 2012-08-22 at 19:00 +0800, Gao feng wrote:
> 
>> Hi Eric
>>
>> I saw your patch and think this patch is clear and doesn't change too much logic.
>>
>> I test your patch, it not fix this problem.
>>
>> In my test case,when moving a net device to another net namespace,
>> Because you patch delete NETDEV_UNREGISTER event from dst_dev_event,
>> we will just put dst entries into the dst garbage list in event
>> NETDEV_DOWN,without call dst_ifdown to change these dst entries' device
>> to the lo device,and now this net device belongs to the new net namespace.
>>
> 
> Then fix the "moving a net device to another net namespace", instead
> of slowing down other common operations.
> 

okay, I will send a patch to fix this problem after your patch beeing applied.

> dev_change_net_namespace() is probably a better place to put your patch
> 
>> After the net device beeing moved to another net namespace, I rmmod this
>> net device's driver,this will trigger the new added event NETDEV_UNREGISTER_FINISH,
>> so in dst_dev_event,we will change these dst entries's device to the new net
>> namespace's lo device,and this will make the referenct count of the new net namespace's
>> lo device incorrect. when we exit the new net namespace,this emg message is still exist.
>>
>> Message from syslogd@Donkey at Aug 22 18:50:13 ...
>>  kernel:[ 1161.979036] unregister_netdevice: waiting for lo to become free. Usage count = 1
>>
>> And because net_mutex is locked here,so we can't create new net namespace.
>>
>>> rcu_barrier() at this place will kill some workloads.
>>>
>>
>> I think this will only add some workloads when unregister a net device.
>> Do I miss something?
> 
> Yes, rcu_barrier() at this point is killing performance, because we hold
> RTNL.
> 
> We worked hard to batch things, your patch is a huge step backward.
> 

Get it,thanks for your explanation.

Re: [PATCH] net: dev: fix the incorrect hold of net namespace's lo device

[Eric Dumazet]

On Wed, 2012-08-22 at 10:39 +0200, Eric Dumazet wrote:

> rcu_barrier() at this place will kill some workloads.

For example, I tested the following one :

modprobe dummy numdummies=1000
time rmmod dummy

-> 16.5 seconds instead of 0.4 second