[PATCH net-next-2.6] veth: Fix veth_dellink method

[PATCH net-next-2.6] veth: Fix veth_dellink method

[Eric Dumazet]

In commit 23289a37e2b127dfc4de1313fba15bb4c9f0cd5b
(net: add a list_head parameter to dellink() method),
I forgot to actually use this parameter in veth_dellink.

I remember feeling a bit uncomfortable about veth_close(),
because it does :

netif_carrier_off(dev);
netif_carrier_off(priv->peer);


Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index ffb502d..9bed694 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -450,8 +450,8 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
 	priv = netdev_priv(dev);
 	peer = priv->peer;
 
-	unregister_netdevice(dev);
-	unregister_netdevice(peer);
+	unregister_netdevice_queue(dev, head);
+	unregister_netdevice_queue(peer, head);
 }
 
 static const struct nla_policy veth_policy[VETH_INFO_MAX + 1];

Re: [PATCH net-next-2.6] veth: Fix veth_dellink method

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 30 Oct 2009 08:00:31 +0100

> In commit 23289a37e2b127dfc4de1313fba15bb4c9f0cd5b
> (net: add a list_head parameter to dellink() method),
> I forgot to actually use this parameter in veth_dellink.
> 
> I remember feeling a bit uncomfortable about veth_close(),
> because it does :
> 
> netif_carrier_off(dev);
> netif_carrier_off(priv->peer);
> 
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks Eric.

Re: [PATCH net-next-2.6] veth: Fix veth_dellink method

[Eric W. Biederman]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> In commit 23289a37e2b127dfc4de1313fba15bb4c9f0cd5b
> (net: add a list_head parameter to dellink() method),
> I forgot to actually use this parameter in veth_dellink.
>
> I remember feeling a bit uncomfortable about veth_close(),
> because it does :
>
> netif_carrier_off(dev);
> netif_carrier_off(priv->peer);
>
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index ffb502d..9bed694 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -450,8 +450,8 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
>  	priv = netdev_priv(dev);
>  	peer = priv->peer;
>  
> -	unregister_netdevice(dev);
> -	unregister_netdevice(peer);
> +	unregister_netdevice_queue(dev, head);
> +	unregister_netdevice_queue(peer, head);

Unless I am mistaken you need to change the list_add_tail to
list_move_tail in unregister_netdevice_queue because we will be
adding each veth device twice.

Eric

[PATCH net-next-2.6] veth: Fix unregister_netdevice_queue for veth

[Eric W. Biederman]

I tested the recent unregister many changes and got a weird,
nasty and seemingly unrelasted kernel oops. Changing
unregister_netdevice_queue to use list_move_tail fixes
the problem for me.

ip link add type veth
rmmod veth

ls /sys/class/net/
showed one of the veth devices still present.

A subsequent ip link oopsed the box.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

---
diff --git a/net/core/dev.c b/net/core/dev.c
index 94f42a1..9c0b202 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5227,6 +5227,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	netdev_init_queues(dev);
 
 	INIT_LIST_HEAD(&dev->napi_list);
+	INIT_LIST_HEAD(&dev->unreg_list);
 	dev->priv_flags = IFF_XMIT_DST_RELEASE;
 	setup(dev);
 	strcpy(dev->name, name);
@@ -5308,7 +5309,7 @@ void unregister_netdevice_queue(struct net_device *dev, struct list_head *head)
 	ASSERT_RTNL();
 
 	if (head) {
-		list_add_tail(&dev->unreg_list, head);
+		list_move_tail(&dev->unreg_list, head);
 	} else {
 		rollback_registered(dev);
 		/* Finish processing unregister after unlock */

Re: [PATCH net-next-2.6] veth: Fix unregister_netdevice_queue for veth

[Eric Dumazet]

Eric W. Biederman a écrit :
> I tested the recent unregister many changes and got a weird,
> nasty and seemingly unrelasted kernel oops. Changing
> unregister_netdevice_queue to use list_move_tail fixes
> the problem for me.
> 
> ip link add type veth
> rmmod veth
> 
> ls /sys/class/net/
> showed one of the veth devices still present.
> 
> A subsequent ip link oopsed the box.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 

Yes, problem is __rtnl_kill_links() doesnt anymore restart
its loop and does :

        for_each_netdev(net, dev) {
                if (dev->rtnl_link_ops == ops)
                        ops->dellink(dev, &list_kill);
        }
        unregister_netdevice_many(&list_kill);

As veth wants in its dellink() method to unregister two netdevices,
we really want your fix or corrupt list_kill

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Thanks Eric

Re: [PATCH net-next-2.6] veth: Fix unregister_netdevice_queue for veth

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 31 Oct 2009 10:41:39 +0100

> Eric W. Biederman a écrit :
>> I tested the recent unregister many changes and got a weird,
>> nasty and seemingly unrelasted kernel oops. Changing
>> unregister_netdevice_queue to use list_move_tail fixes
>> the problem for me.
>> 
>> ip link add type veth
>> rmmod veth
>> 
>> ls /sys/class/net/
>> showed one of the veth devices still present.
>> 
>> A subsequent ip link oopsed the box.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
 ...
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.

Re: [PATCH net-next-2.6] veth: Fix veth_dellink method

[Eric Dumazet]

Eric W. Biederman a écrit :
> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index ffb502d..9bed694 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -450,8 +450,8 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
>>  	priv = netdev_priv(dev);
>>  	peer = priv->peer;
>>  
>> -	unregister_netdevice(dev);
>> -	unregister_netdevice(peer);
>> +	unregister_netdevice_queue(dev, head);
>> +	unregister_netdevice_queue(peer, head);
> 
> Unless I am mistaken you need to change the list_add_tail to
> list_move_tail in unregister_netdevice_queue because we will be
> adding each veth device twice.
> 

You are right, and your patch is the solution to this problem

Thanks