Check return of dev_set_promiscuity/allmulti

PATCH In review Wang Chen 3 participants 28 messages Jul 1, 2008 – Jul 3, 2008

Subsystems (6)

Networking Network drivers IPv4/IPv6 Ethernet bridge +2 more subsystems

lore

Share & tools

lore

Wang Chen

author

2008-07-01 03:24 UTC #0 lore raw

cover Check return of dev_set_promiscuity/allmulti

David, I've send this series of patch before, but discussion made the mail thread turbid.
I cleanup it again and make it easy for you to pick them up.

Wang Chen

author

2008-07-01 03:35 UTC #2 lore raw

Re: v2 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

Wang Chen said the following on 2008-7-1 11:21:

Quoted 7 lines

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 af_packet, we check all positive increment for promiscuity and allmulti to get error return.

oops, the subject should be 1/7.
sorry :(

David Miller

maintainer

2008-07-02 02:56 UTC #3 lore raw

Re: v2 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

From: Wang Chen <[email protected]>
Date: Tue, 01 Jul 2008 11:31:14 +0800

Quoted 11 lines

Wang Chen said the following on 2008-7-1 11:21:
Quoted 7 lines
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 af_packet, we check all positive increment for promiscuity and allmulti to get error return.
oops, the subject should be 1/7.
sorry :(

I understood this :)

Please make the fixes requested by Patrick and resubmit. The patches look otherwise fine to me.

Patrick McHardy

2008-07-01 09:36 UTC #4 lore raw

Re: v2 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

In reply to Wang Chen

Quoted 9 lines · Wang Chen

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 af_packet, we check all positive increment for promiscuity and allmulti to get error return.
static void packet_dev_mclist(struct net_device *dev, struct packet_mclist *i, int what)

net/packet/af_packet.c static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq) · line 1250

Quoted 5 lines

net/packet/af_packet.c +1 -1

@@ -1245,7 +1247,7 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
 	i->count = 1;
 	i->next = po->mclist;
 	po->mclist = i;
-	packet_dev_mc(dev, i, +1);
+	err = packet_dev_mc(dev, i, 1);

Missing error handling. The packet_mclist is already initialized and on the list here, which will result in deletion without addition in packet_flush_mclist().

Wang Chen

author

2008-07-01 09:47 UTC #5 lore raw

Re: v2 [PATCH net-next 1/8] af_packet: Check return of dev_set_promiscuity/allmulti

Patrick McHardy said the following on 2008-7-1 17:36:

Quoted 10 lines

Quoted 9 lines · Wang Chen
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 af_packet, we check all positive increment for promiscuity and allmulti
to get error return.

Quoted 14 lines

net/packet/af_packet.c static int packet_mc_add(struct sock *sk, struct · line 1251
Quoted 9 lines
static void packet_dev_mclist(struct net_device *dev, struct
packet_mclist *i, int what)
net/packet/af_packet.c +1 -1
@@ -1245,7 +1247,7 @@ static int packet_mc_add(struct sock *sk, struct
 packet_mreq_max *mreq)
     i->count = 1;
     i->next = po->mclist;
     po->mclist = i;
-    packet_dev_mc(dev, i, +1);
+    err = packet_dev_mc(dev, i, 1);
Missing error handling. The packet_mclist is already initialized and on the list here, which will result in deletion without addition in packet_flush_mclist().

will do.

Wang Chen

author

2008-07-02 08:17 UTC #6 lore raw

v3 [PATCH net-next 1/7] af_packet: Check return of dev_set_promiscuity/allmulti

In reply to Patrick McHardy

v2->v3:
Do unwinding when error happens.

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 af_packet, we check all positive increment for promiscuity and allmulti to get error return.

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

---

net/packet/af_packet.c +10 -4

index 2cee87d..7321825 100644
@@ -1175,7 +1175,8 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr,
 	return 0;
 }
 
-static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int what)
+static int packet_dev_mc(struct net_device *dev, struct packet_mclist *i,
+			 int what)
 {
 	switch (i->type) {
 	case PACKET_MR_MULTICAST:

@@ -1185,13 +1186,14 @@ static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int w
 			dev_mc_delete(dev, i->addr, i->alen, 0);
 		break;
 	case PACKET_MR_PROMISC:
-		dev_set_promiscuity(dev, what);
+		return dev_set_promiscuity(dev, what);
 		break;
 	case PACKET_MR_ALLMULTI:
-		dev_set_allmulti(dev, what);
+		return dev_set_allmulti(dev, what);
 		break;
 	default:;
 	}
+	return 0;
 }
 
 static void packet_dev_mclist(struct net_device *dev, struct packet_mclist *i, int what)

@@ -1245,7 +1247,11 @@ static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)
 	i->count = 1;
 	i->next = po->mclist;
 	po->mclist = i;
-	packet_dev_mc(dev, i, +1);
+	err = packet_dev_mc(dev, i, 1);
+	if (err) {
+		po->mclist = i->next;
+		kfree(i);
+	}
 
 done:
 	rtnl_unlock();

Patrick McHardy

Acked-by

2008-07-02 12:53 UTC #7 lore raw

Re: v3 [PATCH net-next 1/7] af_packet: Check return of dev_set_promiscuity/allmulti

Quoted 11 lines · Wang Chen

v2->v3:
Do unwinding when error happens.
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 af_packet, we check all positive increment for promiscuity and allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>

Acked-by Patrick McHardy <[email protected]>

Wang Chen

author

2008-07-01 03:27 UTC #8 lore raw

v2 [PATCH net-next 2/7] bonding: Check return of dev_set_promiscuity/allmulti

In reply to Wang Chen

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);
 	}

-- 1.5.3.4

Wang Chen

author

2008-07-02 08:19 UTC #9 lore raw

Re: v2 [PATCH net-next 2/7] bonding: Check return of dev_set_promiscuity/allmulti

Wang Chen said the following on 2008-7-1 11:23:

Quoted 4 lines

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.

Quoted 9 lines

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.

I've checked this one, all the unwind jobs, which can be done, were done.

Wang Chen

author

2008-07-01 03:29 UTC #10 lore raw

v2 [PATCH net-next 3/7] bridge: Check return of dev_set_promiscuity

In reply to Wang Chen

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.

Here, we check the positive increment for promiscuity to get error return.

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

Signed-off-by Stephen Hemminger <[email protected]>

---
 net/bridge/br_if.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

net/bridge/br_if.c +6 -3

index c2397f5..58bbfb8 100644
@@ -375,6 +375,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto put_back;
+
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
 	if (err)

@@ -389,7 +393,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 		goto err2;
 
 	rcu_assign_pointer(dev->br_port, p);
-	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);

@@ -413,12 +416,12 @@ err2:
 	br_fdb_delete_by_port(br, p, 1);
 err1:
 	kobject_del(&p->kobj);
-	goto put_back;
 err0:
 	kobject_put(&p->kobj);
-
+	dev_set_promiscuity(dev, -1);
 put_back:
 	dev_put(dev);
+	kfree(p);
 	return err;
 }

-- 1.5.3.4

Wang Chen

author

2008-07-02 08:20 UTC #11 lore raw

Re: v2 [PATCH net-next 3/7] bridge: Check return of dev_set_promiscuity

Wang Chen said the following on 2008-7-1 11:25:

Quoted 6 lines

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.
Here, we check the positive increment for promiscuity to get error return.

Of course, I did the unwind job.

Wang Chen

author

2008-07-01 03:31 UTC #12 lore raw

v2 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

In reply to Wang Chen

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

Here, we check the positive increment for allmulti to get error return.

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

---
 net/ipv6/ip6mr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

net/ipv6/ip6mr.c +4 -1

index 1479618..3e2d964 100644
@@ -603,6 +603,7 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 	int vifi = vifc->mif6c_mifi;
 	struct mif_device *v = &vif6_table[vifi];
 	struct net_device *dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (MIF_EXISTS(vifi))

@@ -632,7 +633,9 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		return -EINVAL;
 	}
 
-	dev_set_allmulti(dev, 1);
+	err = dev_set_allmulti(dev, 1);
+	if (err)
+		return err;
 
 	/*
 	 *	Fill in the VIF structures

-- 1.5.3.4

Patrick McHardy

2008-07-01 09:39 UTC #13 lore raw

Re: v2 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

Quoted 24 lines · Wang Chen

allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened.
Here, we check the positive increment for allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>
---
 net/ipv6/ip6mr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/ipv6/ip6mr.c +1 -0
index 1479618..3e2d964 100644
@@ -603,6 +603,7 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 	int vifi = vifc->mif6c_mifi;
 	struct mif_device *v = &vif6_table[vifi];
 	struct net_device *dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (MIF_EXISTS(vifi))

net/ipv6/ip6mr.c static int mif6_add(struct mif6ctl *vifc, int mrtsock) · line 638

Quoted 7 lines

net/ipv6/ip6mr.c +3 -1

@@ -632,7 +633,9 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		return -EINVAL;
 	}
 
-	dev_set_allmulti(dev, 1);
+	err = dev_set_allmulti(dev, 1);
+	if (err)
+		return err;

Missing error handling for the MIFF_REGISTER case.

Wang Chen

author

2008-07-01 09:49 UTC #14 lore raw

Re: v2 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

Patrick McHardy said the following on 2008-7-1 17:39:

Quoted 31 lines

Quoted 30 lines · Wang Chen
allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes
dev_set_promiscuity/allmulti return error number if overflow happened.
Here, we check the positive increment for allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>
---
 net/ipv6/ip6mr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/ipv6/ip6mr.c +1 -0
index 1479618..3e2d964 100644
@@ -603,6 +603,7 @@ static int mif6_add(struct mif6ctl *vifc, int
 mrtsock)
     int vifi = vifc->mif6c_mifi;
     struct mif_device *v = &vif6_table[vifi];
     struct net_device *dev;
+    int err;
 
     /* Is vif busy ? */
if (MIF_EXISTS(vifi))
net/ipv6/ip6mr.c +0 -0
@@ -632,7 +633,9 @@ static int mif6_add(struct mif6ctl *vifc, int
 mrtsock)
         return -EINVAL;
     }
 

Quoted 8 lines

net/ipv6/ip6mr.c static int mif6_add(struct mif6ctl *vifc, int mrtsock) · line 638
Quoted 4 lines
net/ipv6/ip6mr.c +3 -1
-    dev_set_allmulti(dev, 1);
+    err = dev_set_allmulti(dev, 1);
+    if (err)
+        return err;
Missing error handling for the MIFF_REGISTER case.

will do, thank you.

Wang Chen

author

2008-07-02 08:22 UTC #15 lore raw

v3 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

In reply to Patrick McHardy

v2->v3:
Do unwind job when error happen.

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

Here, we check the positive increment for allmulti to get error return.

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

---

net/ipv6/ip6mr.c +16 -2

index 1479618..ba763b8 100644
@@ -461,6 +461,13 @@ failure:
 	unregister_netdevice(dev);
 	return NULL;
 }
+
+static void ip6mr_unreg_vif(struct net_device *dev)
+{
+	dev_close(dev);
+	unregister_netdevice(dev);
+	free_netdev(dev);
+}
 #endif
 
 /*

@@ -603,6 +610,7 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 	int vifi = vifc->mif6c_mifi;
 	struct mif_device *v = &vif6_table[vifi];
 	struct net_device *dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (MIF_EXISTS(vifi))

@@ -620,6 +628,11 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		dev = ip6mr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			ip6mr_unreg_vif(dev);
+			return err;
+		}
 		break;
 #endif
 	case 0:

@@ -627,13 +640,14 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		if (!dev)
 			return -EADDRNOTAVAIL;
 		dev_put(dev);
+		err = dev_set_allmulti(dev, 1);
+		if (err)
+			return err;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	dev_set_allmulti(dev, 1);
-
 	/*
 	 *	Fill in the VIF structures
 	 */

Patrick McHardy

2008-07-02 12:54 UTC #16 lore raw

Re: v3 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

Quoted 8 lines · Wang Chen

v2->v3:
Do unwind job when error happen.
net/ipv6/ip6mr.c +0 -0
index 1479618..ba763b8 100644

net/ipv6/ip6mr.c failure: · line 468

Quoted 8 lines

net/ipv6/ip6mr.c +5 -0

@@ -461,6 +461,13 @@ failure:
 	unregister_netdevice(dev);
 	return NULL;
 }
+
+static void ip6mr_unreg_vif(struct net_device *dev)
+{
+	dev_close(dev);
+	unregister_netdevice(dev);

dev_close is already performed by unregister_netdevice.

net/ipv6/ip6mr.c failure: · line 469

Quoted 1 line

net/ipv6/ip6mr.c +1 -0
+	free_netdev(dev);

This is a double free, the device uses free_netdev as destructor.

Wang Chen

author

2008-07-03 03:28 UTC #17 lore raw

v4 [PATCH net-next 4/7] ipv6: Check return of dev_set_allmulti

Patrick McHardy said the following on 2008-7-2 20:54:

Quoted 9 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+    dev_close(dev);
+    unregister_netdevice(dev);
dev_close is already performed by unregister_netdevice.
Quoted 1 line
+ free_netdev(dev);
This is a double free, the device uses free_netdev as destructor.

Patrick, thanks for your time.
I am so shame for being unware such idiot mistake.

v3->v4
As Patrick said, unregister_netdevice() can do all unwind job for ip6mr_reg_vif().

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

Here, we check the positive increment for allmulti to get error return.

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

---

net/ipv6/ip6mr.c +9 -2

index 1479618..6cd286d 100644
@@ -603,6 +603,7 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 	int vifi = vifc->mif6c_mifi;
 	struct mif_device *v = &vif6_table[vifi];
 	struct net_device *dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (MIF_EXISTS(vifi))

@@ -620,6 +621,11 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		dev = ip6mr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			unregister_netdevice(dev);
+			return err;
+		}
 		break;
 #endif
 	case 0:

@@ -627,13 +633,14 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		if (!dev)
 			return -EADDRNOTAVAIL;
 		dev_put(dev);
+		err = dev_set_allmulti(dev, 1);
+		if (err)
+			return err;
 		break;
 	default:
 		return -EINVAL;
 	}
 
-	dev_set_allmulti(dev, 1);
-
 	/*
 	 *	Fill in the VIF structures
 	 */

Wang Chen

author

2008-07-01 03:32 UTC #18 lore raw

v2 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

In reply to Wang Chen

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

Here, we check the positive increment for allmulti to get error return.

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

---
 net/ipv4/ipmr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

net/ipv4/ipmr.c +4 -1

index 11700a4..ba0280f 100644
@@ -398,6 +398,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	struct vif_device *v = &vif_table[vifi];
 	struct net_device *dev;
 	struct in_device *in_dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (VIF_EXISTS(vifi))

@@ -435,7 +436,9 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
 		return -EADDRNOTAVAIL;
 	IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
-	dev_set_allmulti(dev, +1);
+	err = dev_set_allmulti(dev, 1);
+	if (err)
+		return err;
 	ip_rt_multicast_event(in_dev);
 
 	/*

-- 1.5.3.4

Patrick McHardy

2008-07-01 09:40 UTC #19 lore raw

Re: v2 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

Quoted 24 lines · Wang Chen

allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes dev_set_promiscuity/allmulti return error number if overflow happened.
Here, we check the positive increment for allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>
---
 net/ipv4/ipmr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/ipv4/ipmr.c +1 -0
index 11700a4..ba0280f 100644
@@ -398,6 +398,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	struct vif_device *v = &vif_table[vifi];
 	struct net_device *dev;
 	struct in_device *in_dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (VIF_EXISTS(vifi))

net/ipv4/ipmr.c static int vif_add(struct vifctl *vifc, int mrtsock) · line 441

Quoted 7 lines

net/ipv4/ipmr.c +3 -1

@@ -435,7 +436,9 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
 		return -EADDRNOTAVAIL;
 	IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
-	dev_set_allmulti(dev, +1);
+	err = dev_set_allmulti(dev, 1);
+	if (err)
+		return err;

Missing error handling for VIFF_REGISTER and VIFF_TUNNEL case.

Wang Chen

author

2008-07-01 09:54 UTC #20 lore raw

Re: v2 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

Patrick McHardy said the following on 2008-7-1 17:40:

Quoted 23 lines

Quoted 22 lines · Wang Chen
allmulti might overflow.
Commit: "netdevice: Fix promiscuity and allmulti overflow" in net-next makes
dev_set_promiscuity/allmulti return error number if overflow happened.
Here, we check the positive increment for allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>
---
 net/ipv4/ipmr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/ipv4/ipmr.c +1 -0
index 11700a4..ba0280f 100644
@@ -398,6 +398,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
     struct vif_device *v = &vif_table[vifi];
     struct net_device *dev;
     struct in_device *in_dev;
+    int err;
 

Quoted 13 lines

net/ipv4/ipmr.c static int vif_add(struct vifctl *vifc, int mrtsock) · line 441

Quoted 10 lines

net/ipv4/ipmr.c +3 -1

     /* Is vif busy ? */
     if (VIF_EXISTS(vifi))

@@ -435,7 +436,9 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
     if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
         return -EADDRNOTAVAIL;
     IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
-    dev_set_allmulti(dev, +1);
+    err = dev_set_allmulti(dev, 1);
+    if (err)
+        return err;

Missing error handling for VIFF_REGISTER and VIFF_TUNNEL case.

will do.
seems

Quoted 1 line

     if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)

should do error handling too.

Patrick McHardy

2008-07-01 09:57 UTC #21 lore raw

Re: v2 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

Quoted 10 lines · Wang Chen

Patrick McHardy said the following on 2008-7-1 17:40:
Quoted 7 lines
net/ipv4/ipmr.c static int vif_add(struct vifctl *vifc, int mrtsock) · line 441
Quoted 4 lines · Wang Chen
net/ipv4/ipmr.c +3 -1
-    dev_set_allmulti(dev, +1);
+    err = dev_set_allmulti(dev, 1);
+    if (err)
+        return err;
Missing error handling for VIFF_REGISTER and VIFF_TUNNEL case.
will do.

Thanks. I didn't check the other patches (except those I ACKed). Please also verify wether they all do error handling properly.

Quoted 3 lines

seems
Quoted 1 line
     if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
should do error handling too.

Doesn't seem necessary, it doesn't take a reference or has any other sideeffects.

Wang Chen

author

2008-07-02 08:29 UTC #22 lore raw

v3 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

In reply to Patrick McHardy

v2->v3
Do some unwind work when error happen.

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

Here, we check the positive increment for allmulti to get error return.

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

---

net/ipv4/ipmr.c +21 -1

index 11700a4..b1916d2 100644
@@ -244,6 +244,13 @@ failure:
 	unregister_netdevice(dev);
 	return NULL;
 }
+
+static void ipmr_unreg_vif(struct net_device *dev)
+{
+	dev_close(dev);
+	unregister_netdevice(dev);
+	free_netdev(dev);
+}
 #endif
 
 /*

@@ -398,6 +405,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	struct vif_device *v = &vif_table[vifi];
 	struct net_device *dev;
 	struct in_device *in_dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (VIF_EXISTS(vifi))

@@ -415,18 +423,31 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		dev = ipmr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			ipmr_unreg_vif(dev);
+			return err;
+		}
 		break;
 #endif
 	case VIFF_TUNNEL:
 		dev = ipmr_new_tunnel(vifc);
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			dev_close(dev);
+			return err;
+		}
 		break;
 	case 0:
 		dev = ip_dev_find(&init_net, vifc->vifc_lcl_addr.s_addr);
 		if (!dev)
 			return -EADDRNOTAVAIL;
 		dev_put(dev);
+		err = dev_set_allmulti(dev, 1);
+		if (err)
+			return err;
 		break;
 	default:
 		return -EINVAL;

@@ -435,7 +456,6 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
 		return -EADDRNOTAVAIL;
 	IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
-	dev_set_allmulti(dev, +1);
 	ip_rt_multicast_event(in_dev);
 
 	/*

Patrick McHardy

2008-07-02 12:55 UTC #23 lore raw

Re: v3 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

net/ipv4/ipmr.c failure: · line 253

Quoted 9 lines · Wang Chen

v2->v3
Do some unwind work when error happen.

net/ipv4/ipmr.c +6 -0

+static void ipmr_unreg_vif(struct net_device *dev)
+{
+	dev_close(dev);
+	unregister_netdevice(dev);
+	free_netdev(dev);
+}

Same problems as the IPv6 version.

Wang Chen

author

2008-07-03 03:30 UTC #24 lore raw

v4 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

Patrick McHardy said the following on 2008-7-2 20:55:

Quoted 2 lines

Same problems as the IPv6 version.

v3->v4
As Patrick said, unregister_netdevice() can do all unwind job for ipmr_reg_vif().

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

Here, we check the positive increment for allmulti to get error return.

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

---

net/ipv4/ipmr.c +14 -1

index 11700a4..05a2ab8 100644
@@ -398,6 +398,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	struct vif_device *v = &vif_table[vifi];
 	struct net_device *dev;
 	struct in_device *in_dev;
+	int err;
 
 	/* Is vif busy ? */
 	if (VIF_EXISTS(vifi))

@@ -415,18 +416,31 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		dev = ipmr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			unregister_netdevice(dev);
+			return err;
+		}
 		break;
 #endif
 	case VIFF_TUNNEL:
 		dev = ipmr_new_tunnel(vifc);
 		if (!dev)
 			return -ENOBUFS;
+		err = dev_set_allmulti(dev, 1);
+		if (err) {
+			dev_close(dev);
+			return err;
+		}
 		break;
 	case 0:
 		dev = ip_dev_find(&init_net, vifc->vifc_lcl_addr.s_addr);
 		if (!dev)
 			return -EADDRNOTAVAIL;
 		dev_put(dev);
+		err = dev_set_allmulti(dev, 1);
+		if (err)
+			return err;
 		break;
 	default:
 		return -EINVAL;

@@ -435,7 +449,6 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 	if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
 		return -EADDRNOTAVAIL;
 	IPV4_DEVCONF(in_dev->cnf, MC_FORWARDING)++;
-	dev_set_allmulti(dev, +1);
 	ip_rt_multicast_event(in_dev);
 
 	/*

David Miller

maintainer

2008-07-03 04:13 UTC #25 lore raw

Re: v4 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

From: Wang Chen <[email protected]>
Date: Thu, 03 Jul 2008 11:25:47 +0800

Quoted 4 lines

v3->v4
As Patrick said, unregister_netdevice() can do all unwind job for ipmr_reg_vif().

Please repost the entire series for me once you've worked out all of the bugs ;-)

Wang Chen

author

2008-07-01 03:33 UTC #26 lore raw

v2 [PATCH net-next 6/7] macvlan: Check return of dev_set_allmulti

In reply to Wang Chen

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

Here, we check the positive increment for allmulti to get error return.

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

Acked-by Patrick McHardy <[email protected]>

---
 drivers/net/macvlan.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

drivers/net/macvlan.c +11 -3

index c36a03a..a857330 100644
@@ -189,12 +189,20 @@ static int macvlan_open(struct net_device *dev)
 
 	err = dev_unicast_add(lowerdev, dev->dev_addr, ETH_ALEN);
 	if (err < 0)
-		return err;
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(lowerdev, 1);
+		goto out;
+	if (dev->flags & IFF_ALLMULTI) {
+		err = dev_set_allmulti(lowerdev, 1);
+		if (err < 0)
+			goto del_unicast;
+	}
 
 	hlist_add_head_rcu(&vlan->hlist, &port->vlan_hash[dev->dev_addr[5]]);
 	return 0;
+
+del_unicast:
+	dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
+out:
+	return err;
 }
 
 static int macvlan_stop(struct net_device *dev)

-- 1.5.3.4

Wang Chen

author

2008-07-01 03:34 UTC #27 lore raw

v2 [PATCH net-next 7/7] 8021q: Check return of dev_set_promiscuity/allmulti

In reply to Wang Chen

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.

Here, we check all positive increment for promiscuity and allmulti to get error return.

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

Acked-by Patrick McHardy <[email protected]>

---
 net/8021q/vlan_dev.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

net/8021q/vlan_dev.c +21 -6

index 5d055c2..083e0c8 100644
@@ -543,16 +543,31 @@ static int vlan_dev_open(struct net_device *dev)
 	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr)) {
 		err = dev_unicast_add(real_dev, dev->dev_addr, ETH_ALEN);
 		if (err < 0)
-			return err;
+			goto out;
 	}
-	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
 
-	if (dev->flags & IFF_ALLMULTI)
-		dev_set_allmulti(real_dev, 1);
-	if (dev->flags & IFF_PROMISC)
-		dev_set_promiscuity(real_dev, 1);
+	if (dev->flags & IFF_ALLMULTI) {
+		err = dev_set_allmulti(real_dev, 1);
+		if (err < 0)
+			goto del_unicast;
+	}
+	if (dev->flags & IFF_PROMISC) {
+		err = dev_set_promiscuity(real_dev, 1);
+		if (err < 0)
+			goto clear_allmulti;
+	}
 
+	memcpy(vlan->real_dev_addr, real_dev->dev_addr, ETH_ALEN);
 	return 0;
+
+clear_allmulti:
+	if (dev->flags & IFF_ALLMULTI)
+		dev_set_allmulti(real_dev, -1);
+del_unicast:
+	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+		dev_unicast_delete(real_dev, dev->dev_addr, ETH_ALEN);
+out:
+	return err;
 }
 
 static int vlan_dev_stop(struct net_device *dev)

-- 1.5.3.4