[PATCH net-next 0/7] Check return of dev_set_promiscuity/allmulti

[PATCH net-next 0/7] Check return of dev_set_promiscuity/allmulti

[Wang Chen]

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.

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

[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.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
 net/packet/af_packet.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2cee87d..6370b9d 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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,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);
 
 done:
 	rtnl_unlock();
-- 1.5.3.4

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

[Wang Chen]

Wang Chen said the following on 2008-7-1 11:21:
> 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 :(

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

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Tue, 01 Jul 2008 11:31:14 +0800

> Wang Chen said the following on 2008-7-1 11:21:
> > 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.

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

[Patrick McHardy]

Wang Chen wrote:
> 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)
> @@ -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().

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-1 17:36:
> Wang Chen wrote:
>> 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)
>> @@ -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.

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

[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 <wangchen@cn.fujitsu.com>
---
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 2cee87d..7321825 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -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();

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

[Patrick McHardy]

Wang Chen wrote:
> 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 <wangchen@cn.fujitsu.com>

Acked-by: Patrick McHardy <kaber@trash.net>

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

[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 <wangchen@cn.fujitsu.com>
---
 drivers/net/bonding/bond_alb.c  |    6 ++++--
 drivers/net/bonding/bond_main.c |   39 +++++++++++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
index 5a67372..b211486 100644
--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -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;
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 50a40e4..fa3a06d 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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

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

[Wang Chen]

Wang Chen said the following on 2008-7-1 11:23:
> 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.
> 

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

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

[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 <wangchen@cn.fujitsu.com>
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
 net/bridge/br_if.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c2397f5..58bbfb8 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -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

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

[Wang Chen]

Wang Chen said the following on 2008-7-1 11:25:
> 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.

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

[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 <wangchen@cn.fujitsu.com>
---
 net/ipv6/ip6mr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 1479618..3e2d964 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -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

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

[Patrick McHardy]

Wang Chen wrote:
> 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 <wangchen@cn.fujitsu.com>
> ---
>  net/ipv6/ip6mr.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 1479618..3e2d964 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -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;


Missing error handling for the MIFF_REGISTER case.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-1 17:39:
> Wang Chen wrote:
>> 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 <wangchen@cn.fujitsu.com>
>> ---
>>  net/ipv6/ip6mr.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
>> index 1479618..3e2d964 100644
>> --- a/net/ipv6/ip6mr.c
>> +++ b/net/ipv6/ip6mr.c
>> @@ -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;
> 
> 
> Missing error handling for the MIFF_REGISTER case.
> 

will do, thank you.

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

[Wang Chen]

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 <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 1479618..ba763b8 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -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
 	 */

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

[Patrick McHardy]

Wang Chen wrote:
> v2->v3:
>   Do unwind job when error happen.
> 
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 1479618..ba763b8 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -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.

> +	free_netdev(dev);

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

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-2 20:54:
>> +    dev_close(dev);
>> +    unregister_netdevice(dev);
> 
> dev_close is already performed by unregister_netdevice.
> 
>> +    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 <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 1479618..6cd286d 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -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
 	 */

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

[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 <wangchen@cn.fujitsu.com>
---
 net/ipv4/ipmr.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 11700a4..ba0280f 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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 

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

[Patrick McHardy]

Wang Chen wrote:
> 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 <wangchen@cn.fujitsu.com>
> ---
>  net/ipv4/ipmr.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 11700a4..ba0280f 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -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;

Missing error handling for VIFF_REGISTER and VIFF_TUNNEL case.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-1 17:40:
> Wang Chen wrote:
>> 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 <wangchen@cn.fujitsu.com>
>> ---
>>  net/ipv4/ipmr.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
>> index 11700a4..ba0280f 100644
>> --- a/net/ipv4/ipmr.c
>> +++ b/net/ipv4/ipmr.c
>> @@ -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;
> 
> Missing error handling for VIFF_REGISTER and VIFF_TUNNEL case.
> 

will do.
seems 
>      if ((in_dev = __in_dev_get_rtnl(dev)) == NULL)
should do error handling too.

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

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-7-1 17:40:
>> Wang Chen wrote:
>>> -    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.

> seems 
>>      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.

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

[Wang Chen]

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 <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 11700a4..b1916d2 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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);
 
 	/*

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

[Patrick McHardy]

Wang Chen wrote:
> v2->v3
>   Do some unwind work when error happen.
> 
> +static void ipmr_unreg_vif(struct net_device *dev)
> +{
> +	dev_close(dev);
> +	unregister_netdevice(dev);
> +	free_netdev(dev);
> +}

Same problems as the IPv6 version.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-2 20:55:
> 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 <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 11700a4..05a2ab8 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -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);
 
 	/*

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

[David Miller]

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Thu, 03 Jul 2008 11:25:47 +0800

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

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

[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 <wangchen@cn.fujitsu.com>
Acked-by: Patrick McHardy <kaber@trash.net>
---
 drivers/net/macvlan.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index c36a03a..a857330 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -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 

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

[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 <wangchen@cn.fujitsu.com>
Acked-by: Patrick McHardy <kaber@trash.net>
---
 net/8021q/vlan_dev.c |   27 +++++++++++++++++++++------
 1 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5d055c2..083e0c8 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -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