bonding: Check return of dev_set_promiscuity/allmulti

PATCH In review Wang Chen 5 participants 7 messages Jun 20, 2008 – Jun 23, 2008

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Wang Chen

author

2008-06-20 00:58 UTC #0 lore raw

PATCH 2/8 bonding: Check return of dev_set_promiscuity/allmulti

dev_set_promiscuity/allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened.

In bond_alb and bond_main, we check all positive increment for promiscuity and allmulti to get error return.
But there are still two problems left.
1. Some callers want quiet handle.
2. If there are multi slaves, it's hard to tell which slaves increment promisc/allmulti successfully and which failed.
So I left these problems to be FIXME.
Fortunately, the overflow is very rare case.

Signed-off-by Wang Chen <[email protected]>

---
 drivers/net/bonding/bond_alb.c  |    7 ++++-
 drivers/net/bonding/bond_main.c |   41 +++++++++++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 10 deletions(-)

drivers/net/bonding/bond_alb.c +5 -2

index 5a67372..26d49d8 100644
@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
+			bond->alb_info.primary_is_promisc = 1;
+		else
+			bond->alb_info.primary_is_promisc = 0;
 	}
 
 	bond->alb_info.rlb_promisc_timeout_counter = 0;

drivers/net/bonding/bond_main.c +33 -8

102 lines not on this page · raw message

Signature · 1 line

1.5.3.4

David Miller

maintainer

2008-06-20 02:07 UTC #1 lore raw

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:42 +0800

drivers/net/bonding/bond_alb.c static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) · line 423

Quoted 8 lines

drivers/net/bonding/bond_alb.c +2 -2

@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))

Like the first patch, please don't add such comments.

drivers/net/bonding/bond_main.c static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct · line 973

Quoted 10 lines

drivers/net/bonding/bond_main.c +3 -0

@@ -955,6 +965,9 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 
 	if (new_active) {
+		/* FIXME: promiscuity and allmulti might overflow,
+		 * but bond_mc_swap's caller likes quiet handle.
+		 */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}

Please reword this comment. The issue is that this code path has no mechanism to signal errors upstream. It isn't about a specific type of error condition in particular, it's about error handling capabilites in general.

drivers/net/bonding/bond_main.c static void bond_set_multicast_list(struct net_device *bond_dev) · line 3959

Quoted 11 lines

drivers/net/bonding/bond_main.c +4 -0

@@ -3933,6 +3950,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 	 * Do promisc before checking multicast_mode
 	 */
 	if ((bond_dev->flags & IFF_PROMISC) && !(bond->flags & IFF_PROMISC)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters promiscuity overflow.
+		 */
 		bond_set_promiscuity(bond, 1);
 	}

Remove specific reference to promiscuity overflow, these are generic error handling issues here regardless of the types of errors that the down calls could encounter.

drivers/net/bonding/bond_main.c static void bond_set_multicast_list(struct net_device *bond_dev) · line 3971

Quoted 10 lines

drivers/net/bonding/bond_main.c +4 -0

@@ -3942,6 +3963,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 
 	/* set allmulti flag to slaves */
 	if ((bond_dev->flags & IFF_ALLMULTI) && !(bond->flags & IFF_ALLMULTI)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters allmulti overflow.
+		 */
 		bond_set_allmulti(bond, 1);
 	}

Likewise.

Wang Chen

author

2008-06-20 03:00 UTC #2 lore raw

David Miller said the following on 2008-6-20 10:07:

Quoted 48 lines

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:42 +0800
Review static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) · line 423
Quoted 8 lines
(quoted diff) +2 -2
@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
Like the first patch, please don't add such comments.
Review static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct · line 973
Quoted 10 lines
(quoted diff) +3 -0
@@ -955,6 +965,9 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 
 	if (new_active) {
+		/* FIXME: promiscuity and allmulti might overflow,
+		 * but bond_mc_swap's caller likes quiet handle.
+		 */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}
Please reword this comment. The issue is that this code path has no mechanism to signal errors upstream. It isn't about a specific type of error condition in particular, it's about error handling capabilites in general.
Review static void bond_set_multicast_list(struct net_device *bond_dev) · line 3959
Quoted 11 lines
(quoted diff) +4 -0
@@ -3933,6 +3950,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 	 * Do promisc before checking multicast_mode
 	 */
 	if ((bond_dev->flags & IFF_PROMISC) && !(bond->flags & IFF_PROMISC)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters promiscuity overflow.
+		 */
 		bond_set_promiscuity(bond, 1);
 	}
 
Remove specific reference to promiscuity overflow, these are generic error handling issues here regardless of the types of errors that the down calls could encounter.
Quoted 2 lines
(quoted diff) +0 -0
@@ -3942,6 +3963,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 

Quoted 11 lines

Review static void bond_set_multicast_list(struct net_device *bond_dev) · line 3971

Quoted 8 lines

(quoted diff) +4 -0

 	/* set allmulti flag to slaves */
 	if ((bond_dev->flags & IFF_ALLMULTI) && !(bond->flags & IFF_ALLMULTI)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters allmulti overflow.
+		 */
 		bond_set_allmulti(bond, 1);
 	}

Likewise.

dev_set_promiscuity/allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened.

In bond_alb and bond_main, we check all positive increment for promiscuity and allmulti to get error return.
But there are still two problems left.
1. Some code path has no mechanism to signal errors upstream.
2. If there are multi slaves, it's hard to tell which slaves increment promisc/allmulti successfully and which failed.
So I left these problems to be FIXME.
Fortunately, the overflow is very rare case.

Signed-off-by Wang Chen <[email protected]>

---
 drivers/net/bonding/bond_alb.c  |    6 ++++--
 drivers/net/bonding/bond_main.c |   39 +++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 10 deletions(-)

drivers/net/bonding/bond_alb.c +4 -2

index 5a67372..b211486 100644
@@ -419,8 +419,10 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
+			bond->alb_info.primary_is_promisc = 1;
+		else
+			bond->alb_info.primary_is_promisc = 0;
 	}
 
 	bond->alb_info.rlb_promisc_timeout_counter = 0;

drivers/net/bonding/bond_main.c +31 -8

index 50a40e4..fa3a06d 100644
@@ -762,39 +762,49 @@ static struct dev_mc_list *bond_mc_list_find_dmi(struct dev_mc_list *dmi, struct
 /*
  * Push the promiscuity flag down to appropriate slaves
  */
-static void bond_set_promiscuity(struct bonding *bond, int inc)
+static int bond_set_promiscuity(struct bonding *bond, int inc)
 {
+	int err = 0;
 	if (USES_PRIMARY(bond->params.mode)) {
 		/* write lock already acquired */
 		if (bond->curr_active_slave) {
-			dev_set_promiscuity(bond->curr_active_slave->dev, inc);
+			err = dev_set_promiscuity(bond->curr_active_slave->dev,
+						  inc);
 		}
 	} else {
 		struct slave *slave;
 		int i;
 		bond_for_each_slave(bond, slave, i) {
-			dev_set_promiscuity(slave->dev, inc);
+			err = dev_set_promiscuity(slave->dev, inc);
+			if (err)
+				return err;
 		}
 	}
+	return err;
 }
 
 /*
  * Push the allmulti flag down to all slaves
  */
-static void bond_set_allmulti(struct bonding *bond, int inc)
+static int bond_set_allmulti(struct bonding *bond, int inc)
 {
+	int err = 0;
 	if (USES_PRIMARY(bond->params.mode)) {
 		/* write lock already acquired */
 		if (bond->curr_active_slave) {
-			dev_set_allmulti(bond->curr_active_slave->dev, inc);
+			err = dev_set_allmulti(bond->curr_active_slave->dev,
+					       inc);
 		}
 	} else {
 		struct slave *slave;
 		int i;
 		bond_for_each_slave(bond, slave, i) {
-			dev_set_allmulti(slave->dev, inc);
+			err = dev_set_allmulti(slave->dev, inc);
+			if (err)
+				return err;
 		}
 	}
+	return err;
 }
 
 /*

@@ -955,6 +965,7 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 
 	if (new_active) {
+		/* FIXME: Signal errors upstream. */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}

@@ -1456,12 +1467,16 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	if (!USES_PRIMARY(bond->params.mode)) {
 		/* set promiscuity level to new slave */
 		if (bond_dev->flags & IFF_PROMISC) {
-			dev_set_promiscuity(slave_dev, 1);
+			res = dev_set_promiscuity(slave_dev, 1);
+			if (res)
+				goto err_close;
 		}
 
 		/* set allmulti level to new slave */
 		if (bond_dev->flags & IFF_ALLMULTI) {
-			dev_set_allmulti(slave_dev, 1);
+			res = dev_set_allmulti(slave_dev, 1);
+			if (res)
+				goto err_close;
 		}
 
 		netif_tx_lock_bh(bond_dev);

@@ -3933,6 +3948,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 	 * Do promisc before checking multicast_mode
 	 */
 	if ((bond_dev->flags & IFF_PROMISC) && !(bond->flags & IFF_PROMISC)) {
+		/*
+		 * FIXME: Need to handle the error when one of the multi-slaves
+		 * encounters error.
+		 */
 		bond_set_promiscuity(bond, 1);
 	}

@@ -3942,6 +3961,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 
 	/* set allmulti flag to slaves */
 	if ((bond_dev->flags & IFF_ALLMULTI) && !(bond->flags & IFF_ALLMULTI)) {
+		/*
+		 * FIXME: Need to handle the error when one of the multi-slaves
+		 * encounters error.
+		 */
 		bond_set_allmulti(bond, 1);
 	}

Signature · 1 line

1.5.3.4

Joe Eykholt

2008-06-21 18:19 UTC #3 lore raw

In reply to David Miller

Quoted 17 lines · David Miller

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:42 +0800
drivers/net/bonding/bond_alb.c static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) · line 423
Quoted 8 lines
drivers/net/bonding/bond_alb.c +2 -2
@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
Like the first patch, please don't add such comments.
Quoted 3 lines
(quoted diff) +0 -0
@@ -955,6 +965,9 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 

Quoted 12 lines

drivers/net/bonding/bond_main.c static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct · line 973
Quoted 7 lines
drivers/net/bonding/bond_main.c +3 -0
 	if (new_active) {
+		/* FIXME: promiscuity and allmulti might overflow,
+		 * but bond_mc_swap's caller likes quiet handle.
+		 */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}
Please reword this comment. The issue is that this code path has no mechanism to signal errors upstream. It isn't about a specific type of error condition in particular, it's about error handling capabilites in general.

If netdev->promiscuity overflows, shouldn't there be a WARN_ON or BUG_ON that catches that? Either someone forgot to clean up, or much less likely, the counter needs to be widened. It isn't necessarily the current caller's fault, but some indication of the problem is better than nothing.

Quoted 32 lines

drivers/net/bonding/bond_main.c static void bond_set_multicast_list(struct net_device *bond_dev) · line 3959
Quoted 11 lines
drivers/net/bonding/bond_main.c +4 -0
@@ -3933,6 +3950,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 	 * Do promisc before checking multicast_mode
 	 */
 	if ((bond_dev->flags & IFF_PROMISC) && !(bond->flags & IFF_PROMISC)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters promiscuity overflow.
+		 */
 		bond_set_promiscuity(bond, 1);
 	}
 
Remove specific reference to promiscuity overflow, these are generic error handling issues here regardless of the types of errors that the down calls could encounter.
drivers/net/bonding/bond_main.c static void bond_set_multicast_list(struct net_device *bond_dev) · line 3971
Quoted 10 lines
drivers/net/bonding/bond_main.c +4 -0
@@ -3942,6 +3963,10 @@ static void bond_set_multicast_list(struct net_device *bond_dev)
 
 	/* set allmulti flag to slaves */
 	if ((bond_dev->flags & IFF_ALLMULTI) && !(bond->flags & IFF_ALLMULTI)) {
+		/*
+		 * FIXME: If bond has multi slaves, how to handle the error
+		 * when one of the slaves encounters allmulti overflow.
+		 */
 		bond_set_allmulti(bond, 1);
 	}
Likewise.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Wang Chen

author

2008-06-23 01:40 UTC #4 lore raw

Joe Eykholt said the following on 2008-6-22 2:19:

Quoted 29 lines

Quoted 16 lines · David Miller
From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:42 +0800
drivers/net/bonding/bond_alb.c static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) · line 423
Quoted 8 lines
drivers/net/bonding/bond_alb.c +2 -2
@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
Like the first patch, please don't add such comments.
Quoted 3 lines
(quoted diff) +0 -0
@@ -955,6 +965,9 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 
Quoted 11 lines
drivers/net/bonding/bond_main.c static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct · line 973
Quoted 7 lines
drivers/net/bonding/bond_main.c +3 -0
 	if (new_active) {
+		/* FIXME: promiscuity and allmulti might overflow,
+		 * but bond_mc_swap's caller likes quiet handle.
+		 */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}
Please reword this comment. The issue is that this code path has no mechanism to signal errors upstream. It isn't about a specific type of error condition in particular, it's about error handling capabilites in general.

Quoted 5 lines

If netdev->promiscuity overflows, shouldn't there be a WARN_ON or BUG_ON that catches that? Either someone forgot to clean up, or much less likely, the counter needs to be widened. It isn't necessarily the current caller's fault, but some indication of the problem is better than nothing.

If promiscuity overflows, dev_set_promiscuity will printk(KERN_WARNING) now. Compare to that, WARN_ON has more information about modules info and dump stack. But I think printk has enough information to indicate the problem and we don't need WARN_ON.
How do you think?

Joe Eykholt

2008-06-23 04:10 UTC #5 lore raw

Quoted 35 lines · Wang Chen

Joe Eykholt said the following on 2008-6-22 2:19:
Quoted 28 lines
Quoted 16 lines · David Miller
From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:54:42 +0800
drivers/net/bonding/bond_alb.c static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[]) · line 423
Quoted 8 lines
drivers/net/bonding/bond_alb.c +2 -2
@@ -419,8 +419,11 @@ static void rlb_teach_disabled_mac_on_primary(struct bonding *bond, u8 addr[])
 	}
 
 	if (!bond->alb_info.primary_is_promisc) {
-		bond->alb_info.primary_is_promisc = 1;
-		dev_set_promiscuity(bond->curr_active_slave->dev, 1);
+		/* dev_set_promiscuity might overflow, check it here */
+		if (!dev_set_promiscuity(bond->curr_active_slave->dev, 1))
Like the first patch, please don't add such comments.
Quoted 3 lines
(quoted diff) +0 -0
@@ -955,6 +965,9 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct
 	}
 
Quoted 11 lines
drivers/net/bonding/bond_main.c static void bond_mc_swap(struct bonding *bond, struct slave *new_active, struct · line 973
Quoted 7 lines
drivers/net/bonding/bond_main.c +3 -0
 	if (new_active) {
+		/* FIXME: promiscuity and allmulti might overflow,
+		 * but bond_mc_swap's caller likes quiet handle.
+		 */
 		if (bond->dev->flags & IFF_PROMISC) {
 			dev_set_promiscuity(new_active->dev, 1);
 		}
Please reword this comment. The issue is that this code path has no mechanism to signal errors upstream. It isn't about a specific type of error condition in particular, it's about error handling capabilites in general.
Quoted 5 lines
If netdev->promiscuity overflows, shouldn't there be a WARN_ON or BUG_ON that catches that? Either someone forgot to clean up, or much less likely, the counter needs to be widened. It isn't necessarily the current caller's fault, but some indication of the problem is better than nothing.

Quoted 5 lines

If promiscuity overflows, dev_set_promiscuity will printk(KERN_WARNING) now. Compare to that, WARN_ON has more information about modules info and dump stack. But I think printk has enough information to indicate the problem and we don't need WARN_ON.
How do you think?

I agree, printk is enough. If it is reproducible, the cause can be determined. I didn't check to see there was a printk there.

Thanks,
Joe

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Patrick McHardy

2008-06-23 11:13 UTC #6 lore raw

In reply to Wang Chen

Quoted 7 lines · Wang Chen

Joe Eykholt said the following on 2008-6-22 2:19:
Quoted 5 lines
If netdev->promiscuity overflows, shouldn't there be a WARN_ON or BUG_ON that catches that? Either someone forgot to clean up, or much less likely, the counter needs to be widened. It isn't necessarily the current caller's fault, but some indication of the problem is better than nothing.

Quoted 5 lines

If promiscuity overflows, dev_set_promiscuity will printk(KERN_WARNING) now. Compare to that, WARN_ON has more information about modules info and dump stack. But I think printk has enough information to indicate the problem and we don't need WARN_ON.
How do you think?

WARN_ON doesn't provide any useful information here, if the counter overflowed, its because *something else* increased it by too much.