Skip to contentSkip to conversation

: Some BUGs and deadlocks fixes and a bit of concomitant cleanups.

PATCH In review Pavel Emelyanov 3 participants 11 messages Apr 29, 2008 – May 3, 2008

Subsystems (3)
Networking Network drivers Bonding driver
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

cover : Some BUGs and deadlocks fixes and a bit of concomitant cleanups.

Hi Jay, Dave.

While looking at the bonding driver I found four bugs visible with the unaided eye. Here are the fixes, but I also cleaned up some places, I just couldn't come by. If you consider those cleanups to be for next merge window, just kick me and I'll resubmit this set without them.

Found BUGs summary:
1. Bad error path in bond_create - the registered device should not be released with free_netdev;
2. Specifying the 'bond%d' name when creating a new device results in a new device with literally 'bond%d' name, not bond<digit> like for other drivers;
3. Lost unlock of rtnl and bonding_rwsem in bonding_store_bonds;
4. Deadlock between bonding_store_bonds and bond_destroy_sysfs on module unload.

The clean ups are
1. Minor alloc_netdev consolidation after fix #2
2. Redundant 3rd argument for bond_create;
3. Unneeded _safe list iterations in some places;
4. Unneeded list_empty checks in some places.

Details in patches.

Signed-off-by Pavel Emelyanov <[email protected]>
author
#1 lore raw

PATCH 1/8 : Do not call free_netdev for already registered device.

If the call to bond_create_sysfs_entry in bond_create fails, the proper rollback is to call unregister_netdevice, not free_netdev. Otherwise - kernel BUG at net/core/dev.c:4057!

Checked with artificial failures injected into bond_create_sysfs_entry.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)
drivers/net/bonding/bond_main.c +9 -5
index 6e91b4b..36121b6 100644
@@ -4936,14 +4936,18 @@ int bond_create(char *name, struct bond_params *params, struct bonding **newbond
 	up_write(&bonding_rwsem);
 	rtnl_unlock(); /* allows sysfs registration of net device */
 	res = bond_create_sysfs_entry(bond_dev->priv);
-	if (res < 0) {
-		rtnl_lock();
-		down_write(&bonding_rwsem);
-		goto out_bond;
-	}
+	if (res < 0)
+		goto out_unreg;
 
 	return 0;
 
+out_unreg:
+	rtnl_lock();
+	down_write(&bonding_rwsem);
+	bond_deinit(bond_dev);
+	unregister_netdevice(bond_dev);
+	goto out_rtnl;
+
 out_bond:
 	bond_deinit(bond_dev);
 out_netdev:
Signature · 1 line
1.5.3.4
author
#2 lore raw

PATCH 2/8 : Don't create bondings with % in their names.

In reply to Pavel Emelyanov

Type

# echo '+bond%d' > /sys/class/net/bonding_masters
and get the 'bond%d' device in the ip l l output.

Other drivers do not play such jokes on user :)

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/net/bonding/bond_main.c +4 -0
index 36121b6..2072910 100644
@@ -4909,6 +4909,10 @@ int bond_create(char *name, struct bond_params *params, struct bonding **newbond
 		res = dev_alloc_name(bond_dev, "bond%d");
 		if (res < 0)
 			goto out_netdev;
+	} else if (strchr(name, '%')) {
+		res = dev_alloc_name(bond_dev, name);
+		if (res < 0)
+			goto out_netdev;
 	}
 
 	/* bond_init() must be called after dev_alloc_name() (for the
Signature · 1 line
1.5.3.4
author
#3 lore raw

PATCH 3/8 : Merge two calls to dev_alloc_name.

In reply to Pavel Emelyanov

Just reduce the amount of code after the previous patch.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)
drivers/net/bonding/bond_main.c +4 -5
index 2072910..0907f43 100644
@@ -4905,11 +4905,10 @@ int bond_create(char *name, struct bond_params *params, struct bonding **newbond
 		goto out_rtnl;
 	}
 
-	if (!name) {
-		res = dev_alloc_name(bond_dev, "bond%d");
-		if (res < 0)
-			goto out_netdev;
-	} else if (strchr(name, '%')) {
+	if (!name)
+		name = "bond%d";
+
+	if (strchr(name, '%')) {
 		res = dev_alloc_name(bond_dev, name);
 		if (res < 0)
 			goto out_netdev;
Signature · 1 line
1.5.3.4
author
#4 lore raw

PATCH 4/8 : Remove redundant argument from bond_create.

In reply to Pavel Emelyanov

While we're fixing the bond_create, I hope it's OK to polish it a bit after the fixes.

The third argument is NULL at the first caller and is ignored by the second one, so remove it.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c  |    7 ++-----
 drivers/net/bonding/bond_sysfs.c |    2 +-
 drivers/net/bonding/bonding.h    |    2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)
drivers/net/bonding/bond_main.c +2 -5
index 0907f43..c6661a9 100644
@@ -4874,7 +4874,7 @@ static struct lock_class_key bonding_netdev_xmit_lock_key;
  * Caller must NOT hold rtnl_lock; we need to release it here before we
  * set up our sysfs entries.
  */
-int bond_create(char *name, struct bond_params *params, struct bonding **newbond)
+int bond_create(char *name, struct bond_params *params)
 {
 	struct net_device *bond_dev;
 	struct bonding *bond, *nxt;

@@ -4931,9 +4931,6 @@ int bond_create(char *name, struct bond_params *params, struct bonding **newbond
 
 	lockdep_set_class(&bond_dev->_xmit_lock, &bonding_netdev_xmit_lock_key);
 
-	if (newbond)
-		*newbond = bond_dev->priv;
-
 	netif_carrier_off(bond_dev);
 
 	up_write(&bonding_rwsem);

@@ -4981,7 +4978,7 @@ static int __init bonding_init(void)
 	init_rwsem(&bonding_rwsem);
 
 	for (i = 0; i < max_bonds; i++) {
-		res = bond_create(NULL, &bonding_defaults, NULL);
+		res = bond_create(NULL, &bonding_defaults);
 		if (res)
 			goto err;
 	}
drivers/net/bonding/bond_sysfs.c +1 -1
index 979c2d0..88de27f 100644
@@ -122,7 +122,7 @@ static ssize_t bonding_store_bonds(struct class *cls, const char *buffer, size_t
 	if (command[0] == '+') {
 		printk(KERN_INFO DRV_NAME
 			": %s is being created...\n", ifname);
-		rv = bond_create(ifname, &bonding_defaults, &bond);
+		rv = bond_create(ifname, &bonding_defaults);
 		if (rv) {
 			printk(KERN_INFO DRV_NAME ": Bond creation failed.\n");
 			res = rv;
drivers/net/bonding/bonding.h +1 -1
index a3c74e2..0ce7f4a 100644
@@ -301,7 +301,7 @@ static inline void bond_unset_master_alb_flags(struct bonding *bond)
 
 struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev);
-int bond_create(char *name, struct bond_params *params, struct bonding **newbond);
+int bond_create(char *name, struct bond_params *params);
 void bond_destroy(struct bonding *bond);
 int  bond_release_and_destroy(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_create_sysfs(void);
Signature · 1 line
1.5.3.4
author
#5 lore raw

PATCH 5/8 : Lost semaphores unlock in one of bonding_store_bonds error paths.

In reply to Pavel Emelyanov

The out: label just makes return. Wipe things up.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_sysfs.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/net/bonding/bond_sysfs.c +5 -5
index 88de27f..752e43b 100644
@@ -146,12 +146,12 @@ static ssize_t bonding_store_bonds(struct class *cls, const char *buffer, size_t
 						": Unable remove bond %s due to open references.\n",
 						ifname);
 					res = -EPERM;
-					goto out;
+				} else {
+					printk(KERN_INFO DRV_NAME
+						": %s is being deleted...\n",
+						bond->dev->name);
+					bond_destroy(bond);
 				}
-				printk(KERN_INFO DRV_NAME
-					": %s is being deleted...\n",
-					bond->dev->name);
-				bond_destroy(bond);
 				up_write(&bonding_rwsem);
 				rtnl_unlock();
 				goto out;
Signature · 1 line
1.5.3.4
author
#6 lore raw

PATCH 6/8 : Relax unneeded _safe lists iterations.

In reply to Pavel Emelyanov

Many places either do not modify the list under the list_for_each_xxx, or break out of the loop as soon as the first element is removed.

Thus, this _safe iteration just occupies some unneeded .text space and requires an additional variable.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c  |   36 ++++++++++++++++--------------------
 drivers/net/bonding/bond_sysfs.c |    3 +--
 2 files changed, 17 insertions(+), 22 deletions(-)
drivers/net/bonding/bond_main.c +16 -20
index c6661a9..8623e47 100644
@@ -261,14 +261,14 @@ static int bond_add_vlan(struct bonding *bond, unsigned short vlan_id)
  */
 static int bond_del_vlan(struct bonding *bond, unsigned short vlan_id)
 {
-	struct vlan_entry *vlan, *next;
+	struct vlan_entry *vlan;
 	int res = -ENODEV;
 
 	dprintk("bond: %s, vlan id %d\n", bond->dev->name, vlan_id);
 
 	write_lock_bh(&bond->lock);
 
-	list_for_each_entry_safe(vlan, next, &bond->vlan_list, vlan_list) {
+	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		if (vlan->vlan_id == vlan_id) {
 			list_del(&vlan->vlan_list);
 

@@ -2428,7 +2428,7 @@ out:
 
 static int bond_has_ip(struct bonding *bond)
 {
-	struct vlan_entry *vlan, *vlan_next;
+	struct vlan_entry *vlan;
 
 	if (bond->master_ip)
 		return 1;

@@ -2436,8 +2436,7 @@ static int bond_has_ip(struct bonding *bond)
 	if (list_empty(&bond->vlan_list))
 		return 0;
 
-	list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
-				 vlan_list) {
+	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		if (vlan->vlan_ip)
 			return 1;
 	}

@@ -2447,7 +2446,7 @@ static int bond_has_ip(struct bonding *bond)
 
 static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 {
-	struct vlan_entry *vlan, *vlan_next;
+	struct vlan_entry *vlan;
 
 	if (ip == bond->master_ip)
 		return 1;

@@ -2455,8 +2454,7 @@ static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 	if (list_empty(&bond->vlan_list))
 		return 0;
 
-	list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
-				 vlan_list) {
+	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		if (ip == vlan->vlan_ip)
 			return 1;
 	}

@@ -2498,7 +2496,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 {
 	int i, vlan_id, rv;
 	__be32 *targets = bond->params.arp_targets;
-	struct vlan_entry *vlan, *vlan_next;
+	struct vlan_entry *vlan;
 	struct net_device *vlan_dev;
 	struct flowi fl;
 	struct rtable *rt;

@@ -2545,8 +2543,7 @@ static void bond_arp_send_all(struct bonding *bond, struct slave *slave)
 		}
 
 		vlan_id = 0;
-		list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
-					 vlan_list) {
+		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 			vlan_dev = vlan_group_get_device(bond->vlgrp, vlan->vlan_id);
 			if (vlan_dev == rt->u.dst.dev) {
 				vlan_id = vlan->vlan_id;

@@ -3503,13 +3500,13 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 {
 	struct in_ifaddr *ifa = ptr;
 	struct net_device *vlan_dev, *event_dev = ifa->ifa_dev->dev;
-	struct bonding *bond, *bond_next;
-	struct vlan_entry *vlan, *vlan_next;
+	struct bonding *bond;
+	struct vlan_entry *vlan;
 
 	if (dev_net(ifa->ifa_dev->dev) != &init_net)
 		return NOTIFY_DONE;
 
-	list_for_each_entry_safe(bond, bond_next, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bond_dev_list, bond_list) {
 		if (bond->dev == event_dev) {
 			switch (event) {
 			case NETDEV_UP:

@@ -3526,8 +3523,7 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 		if (list_empty(&bond->vlan_list))
 			continue;
 
-		list_for_each_entry_safe(vlan, vlan_next, &bond->vlan_list,
-					 vlan_list) {
+		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 			vlan_dev = vlan_group_get_device(bond->vlgrp, vlan->vlan_id);
 			if (vlan_dev == event_dev) {
 				switch (event) {

@@ -4877,7 +4873,7 @@ static struct lock_class_key bonding_netdev_xmit_lock_key;
 int bond_create(char *name, struct bond_params *params)
 {
 	struct net_device *bond_dev;
-	struct bonding *bond, *nxt;
+	struct bonding *bond;
 	int res;
 
 	rtnl_lock();

@@ -4885,7 +4881,7 @@ int bond_create(char *name, struct bond_params *params)
 
 	/* Check to see if the bond already exists. */
 	if (name) {
-		list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list)
+		list_for_each_entry(bond, &bond_dev_list, bond_list)
 			if (strnicmp(bond->dev->name, name, IFNAMSIZ) == 0) {
 				printk(KERN_ERR DRV_NAME
 			       ": cannot add bond %s; it already exists\n",

@@ -4962,7 +4958,7 @@ static int __init bonding_init(void)
 {
 	int i;
 	int res;
-	struct bonding *bond, *nxt;
+	struct bonding *bond;
 
 	printk(KERN_INFO "%s", version);
 

@@ -4992,7 +4988,7 @@ static int __init bonding_init(void)
 
 	goto out;
 err:
-	list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list) {
+	list_for_each_entry(bond, &bond_dev_list, bond_list) {
 		bond_work_cancel_all(bond);
 		destroy_workqueue(bond->wq);
 	}
drivers/net/bonding/bond_sysfs.c +1 -2
index 752e43b..8133855 100644
@@ -111,7 +111,6 @@ static ssize_t bonding_store_bonds(struct class *cls, const char *buffer, size_t
 	char *ifname;
 	int rv, res = count;
 	struct bonding *bond;
-	struct bonding *nxt;
 
 	sscanf(buffer, "%16s", command); /* IFNAMSIZ*/
 	ifname = command + 1;

@@ -134,7 +133,7 @@ static ssize_t bonding_store_bonds(struct class *cls, const char *buffer, size_t
 		rtnl_lock();
 		down_write(&bonding_rwsem);
 
-		list_for_each_entry_safe(bond, nxt, &bond_dev_list, bond_list)
+		list_for_each_entry(bond, &bond_dev_list, bond_list)
 			if (strnicmp(bond->dev->name, ifname, IFNAMSIZ) == 0) {
 				/* check the ref count on the bond's kobject.
 				 * If it's > expected, then there's a file open,
Signature · 1 line
1.5.3.4
author
#7 lore raw

PATCH 7/8 : Remove unneeded list_empty checks.

In reply to Pavel Emelyanov

Some places iterate over the checked list right after the check itself, so even if the list is empty, the list_for_each_xxx iterator will make everything right by himself.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)
drivers/net/bonding/bond_main.c +0 -9
index 8623e47..4d852ab 100644
@@ -2433,9 +2433,6 @@ static int bond_has_ip(struct bonding *bond)
 	if (bond->master_ip)
 		return 1;
 
-	if (list_empty(&bond->vlan_list))
-		return 0;
-
 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		if (vlan->vlan_ip)
 			return 1;

@@ -2451,9 +2448,6 @@ static int bond_has_this_ip(struct bonding *bond, __be32 ip)
 	if (ip == bond->master_ip)
 		return 1;
 
-	if (list_empty(&bond->vlan_list))
-		return 0;
-
 	list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 		if (ip == vlan->vlan_ip)
 			return 1;

@@ -3520,9 +3514,6 @@ static int bond_inetaddr_event(struct notifier_block *this, unsigned long event,
 			}
 		}
 
-		if (list_empty(&bond->vlan_list))
-			continue;
-
 		list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
 			vlan_dev = vlan_group_get_device(bond->vlgrp, vlan->vlan_id);
 			if (vlan_dev == event_dev) {
Signature · 1 line
1.5.3.4
author
#8 lore raw

PATCH 8/8 : Deadlock between bonding_store_bonds and bond_destroy_sysfs.

In reply to Pavel Emelyanov

The sysfs layer has an internal protection, that ensures, that all the process sitting inside ->sore/->show callback exits before the appropriate entry is unregistered (the calltraces are rather big, but I can provide them if required).

On the other hand, bonding takes rtnl_lock in
a) the bonding_store_bonds, i.e. in ->store callback,
b) module exit before calling the sysfs unregister routines.

Thus, the classical AB-BA deadlock may occur. To reproduce run

# while :; do modprobe bonding; rmmod bonding; done
and
# while :; do echo '+bond%d' > /sys/class/net/bonding_masters ; done
in parallel.

The fix is to move the bond_destroy_sysfs out of the rtnl_lock, but _before_ bond_free_all to make sure no bonding devices exist after module unload.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 drivers/net/bonding/bond_main.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/net/bonding/bond_main.c +4 -2
index 4d852ab..7e7b0b8 100644
@@ -4984,9 +4984,10 @@ err:
 		destroy_workqueue(bond->wq);
 	}
 
+	bond_destroy_sysfs();
+
 	rtnl_lock();
 	bond_free_all();
-	bond_destroy_sysfs();
 	rtnl_unlock();
 out:
 	return res;

@@ -4998,9 +4999,10 @@ static void __exit bonding_exit(void)
 	unregister_netdevice_notifier(&bond_netdev_notifier);
 	unregister_inetaddr_notifier(&bond_inetaddr_notifier);
 
+	bond_destroy_sysfs();
+
 	rtnl_lock();
 	bond_free_all();
-	bond_destroy_sysfs();
 	rtnl_unlock();
 }
Signature · 1 line
1.5.3.4
maintainer
#9 lore raw
In reply to Pavel Emelyanov

From: Pavel Emelyanov <[email protected]>
Date: Tue, 29 Apr 2008 18:51:32 +0400

Quoted 5 lines

While looking at the bonding driver I found four bugs visible with the unaided eye. Here are the fixes, but I also cleaned up some places, I just couldn't come by. If you consider those cleanups to be for next merge window, just kick me and I'll resubmit this set without them.

I think all of these patches are excellent.

Jay, et al., can you make sure these go to Jeff and get integrated?

Thanks.

#10 lore raw
Quoted 9 lines · David Miller

From: Pavel Emelyanov <[email protected]>
Date: Tue, 29 Apr 2008 18:51:32 +0400

Quoted 5 lines

While looking at the bonding driver I found four bugs visible with the unaided eye. Here are the fixes, but I also cleaned up some places, I just couldn't come by. If you consider those cleanups to be for next merge window, just kick me and I'll resubmit this set without them.

Quoted 4 lines

I think all of these patches are excellent.

Jay, et al., can you make sure these go to Jeff and get integrated?

Yep, I played with them today, and am passing on the bug fixes
for 2.6.26 and the cleanups for -next.

	-J

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]