Skip to contentSkip to conversation

veth: Fix veth_dellink method

PATCH Applied Eric Dumazet 3 participants 7 messages Oct 30, 2009 – Nov 2, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 veth: Fix veth_dellink method

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 <[email protected]>

---

drivers/net/veth.c +2 -2
index ffb502d..9bed694 100644
@@ -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];
maintainer Applied
#1 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 30 Oct 2009 08:00:31 +0100

Quoted 4 lines

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

Quoted 8 lines

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 <[email protected]>

Applied, thanks Eric.

#2 lore raw
In reply to Eric Dumazet
Quoted 19 lines · 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 <[email protected]>

---

drivers/net/veth.c +0 -0
index ffb502d..9bed694 100644

drivers/net/veth.c static void veth_dellink(struct net_device *dev, struct list_head *head) · line 454

Quoted 7 lines
drivers/net/veth.c +2 -2
@@ -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

#3 lore raw

PATCH 1/1 veth: Fix unregister_netdevice_queue for veth

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" <[email protected]>

---

net/core/dev.c +2 -1
index 94f42a1..9c0b202 100644
@@ -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 */
author Acked-by on v1 1/1
#4 lore raw

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

Quoted 8 lines · 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

Quoted 7 lines

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" <[email protected]>

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 <[email protected]>

Thanks Eric

maintainer Applied
#5 lore raw

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

From: Eric Dumazet <[email protected]>
Date: Sat, 31 Oct 2009 10:41:39 +0100

Quoted 15 lines
Quoted 14 lines · 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" <[email protected]>

...

Quoted 1 line
Acked-by Eric Dumazet <[email protected]>

Applied, thanks everyone.

author
#6 lore raw
In reply to Eric W. Biederman
Quoted 10 lines · Eric W. Biederman
Quoted 9 lines · Eric Dumazet
drivers/net/veth.c +0 -0
index ffb502d..9bed694 100644
@@ -450,8 +450,8 @@ static void veth_dellink(struct net_device *dev, struct list_head *head)
 	priv = netdev_priv(dev);
 	peer = priv->peer;
Quoted 9 lines

drivers/net/veth.c static void veth_dellink(struct net_device *dev, struct list_head *head) · line 454

Quoted 4 lines
drivers/net/veth.c +2 -2
-	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