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

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

[Wang Chen]

Here is v3 of this series of patch.
This series of patch fix the places where calling dev_set_promiscuity/allmulti
but not do error handling according to the return of
dev_set_promiscuity/allmulti.

BTW, thanks Patrick who fixed a silly mistake of my patch
"netdevice: Fix promiscuity and allmulti overflow".

v2->v3
According to Patrick's review, do error handling when dev_set_promiscuity/allmulti
return negative.

v3 [PATCH net-next 1/7] 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>
Acked-by: Patrick McHardy <kaber@trash.net> 
---
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();

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

v3 [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;
 }
 

v3 [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>
---
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
 	 */

Re: v3 [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.
> 
> @@ -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;

The use of dev after putting it looks wrong, but thats already
present before your patch. So ACK for your patch, but we need
a fix on top.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-7 19:20:
> 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.
>>
>> @@ -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;
> 
> The use of dev after putting it looks wrong, but thats already
> present before your patch. So ACK for your patch, but we need
> a fix on top.
> 

Wait a moment :)
---
	case 0:
		dev = dev_get_by_index(&init_net, vifc->mif6c_pifi);
		if (!dev)
			return -EADDRNOTAVAIL;
		dev_put(dev);
---
dev_get_by_index() holds the dev, so I think dev_put() just for hold/put
balance.

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

[Patrick McHardy]

Wang Chen wrote:
> Patrick McHardy said the following on 2008-7-7 19:20:
>>> @@ -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;
>> The use of dev after putting it looks wrong, but thats already
>> present before your patch. So ACK for your patch, but we need
>> a fix on top.
>>
> 
> Wait a moment :)
> ---
> 	case 0:
> 		dev = dev_get_by_index(&init_net, vifc->mif6c_pifi);
> 		if (!dev)
> 			return -EADDRNOTAVAIL;
> 		dev_put(dev);
> ---
> dev_get_by_index() holds the dev, so I think dev_put() just for hold/put
> balance.

Sure, but its used after dropping the reference again.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-7 21:26:
> Wang Chen wrote:
>> Patrick McHardy said the following on 2008-7-7 19:20:
>>>> @@ -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;
>>> The use of dev after putting it looks wrong, but thats already
>>> present before your patch. So ACK for your patch, but we need
>>> a fix on top.
>>>
>>
>> Wait a moment :)
>> ---
>>     case 0:
>>         dev = dev_get_by_index(&init_net, vifc->mif6c_pifi);
>>         if (!dev)
>>             return -EADDRNOTAVAIL;
>>         dev_put(dev);
>> ---
>> dev_get_by_index() holds the dev, so I think dev_put() just for hold/put
>> balance.
> 
> Sure, but its used after dropping the reference again.

If so, my patch wrong.
Because, 

>               dev = ip6mr_reg_vif();
>               if (!dev)
>                       return -ENOBUFS;

here, needs dev_hold(dev) also.
so, I think I should fix it in this patch too, not a fix on top.
How do you think, Patrick?

> +               err = dev_set_allmulti(dev, 1);
> +               if (err) {
> +                       unregister_netdevice(dev);
> +                       return err;
> +               }
>               break;

v3 [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>
---
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: v3 [PATCH net-next 5/7] ipv4: Check return of dev_set_allmulti

[Patrick McHardy]

Wang Chen wrote:
>  	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;
> +		}

ipmr_new_tunnel creates a new tunnel device that you're not removing.

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

Also looks like a use after free, but again, one that is
already present without your patch.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-7 19:22:
> Wang Chen wrote:
>>      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;
>> +        }
> 
> ipmr_new_tunnel creates a new tunnel device that you're not removing.
> 

OK. Remove the created tunnel.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 11700a4..a55a23a 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -120,6 +120,31 @@ static struct timer_list ipmr_expire_timer;
 
 /* Service routines creating virtual interfaces: DVMRP tunnels and PIMREG */
 
+static void ipmr_del_tunnel(struct net_device *dev, struct vifctl *v)
+{
+	dev_close(dev);
+
+	dev = __dev_get_by_name(&init_net, "tunl0");
+	if (dev) {
+		struct ifreq ifr;
+		mm_segment_t	oldfs;
+		struct ip_tunnel_parm p;
+
+		memset(&p, 0, sizeof(p));
+		p.iph.daddr = v->vifc_rmt_addr.s_addr;
+		p.iph.saddr = v->vifc_lcl_addr.s_addr;
+		p.iph.version = 4;
+		p.iph.ihl = 5;
+		p.iph.protocol = IPPROTO_IPIP;
+		sprintf(p.name, "dvmrp%d", v->vifc_vifi);
+		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
+
+		oldfs = get_fs(); set_fs(KERNEL_DS);
+		dev->do_ioctl(dev, &ifr, SIOCDELTUNNEL);
+		set_fs(oldfs);
+	}
+}
+
 static
 struct net_device *ipmr_new_tunnel(struct vifctl *v)
 {
@@ -398,6 +423,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 +441,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) {
+			ipmr_del_tunnel(dev, vifc);
+			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 +474,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

[Wang Chen]

Wang Chen said the following on 2008-7-8 17:34:
> Patrick McHardy said the following on 2008-7-7 19:22:
>> Wang Chen wrote:
>>>      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;
>>> +        }
>> ipmr_new_tunnel creates a new tunnel device that you're not removing.
>>
> 
> OK. Remove the created tunnel.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> ---
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 11700a4..a55a23a 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -120,6 +120,31 @@ static struct timer_list ipmr_expire_timer;
>  
>  /* Service routines creating virtual interfaces: DVMRP tunnels and PIMREG */
>  
> +static void ipmr_del_tunnel(struct net_device *dev, struct vifctl *v)
> +{
> +	dev_close(dev);
> +
> +	dev = __dev_get_by_name(&init_net, "tunl0");
> +	if (dev) {
> +		struct ifreq ifr;
> +		mm_segment_t	oldfs;
> +		struct ip_tunnel_parm p;
> +
> +		memset(&p, 0, sizeof(p));
> +		p.iph.daddr = v->vifc_rmt_addr.s_addr;
> +		p.iph.saddr = v->vifc_lcl_addr.s_addr;
> +		p.iph.version = 4;
> +		p.iph.ihl = 5;
> +		p.iph.protocol = IPPROTO_IPIP;
> +		sprintf(p.name, "dvmrp%d", v->vifc_vifi);
> +		ifr.ifr_ifru.ifru_data = (__force void __user *)&p;
> +
> +		oldfs = get_fs(); set_fs(KERNEL_DS);
> +		dev->do_ioctl(dev, &ifr, SIOCDELTUNNEL);
> +		set_fs(oldfs);
> +	}
> +}
> +
>  static
>  struct net_device *ipmr_new_tunnel(struct vifctl *v)
>  {
> @@ -398,6 +423,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 +441,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) {
> +			ipmr_del_tunnel(dev, vifc);
> +			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 +474,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, would you  please review this patch.
If you ack, I will resend whole series of patch to David again.

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

[Wang Chen]

Patrick McHardy said the following on 2008-7-7 19:22:
>>      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;
> 
> Also looks like a use after free, but again, one that is
> already present without your patch.
> 

Here is the patch for fixing use after free.
It fixes both ipv4 and ipv6 side and on top of my patches.
This patch will be the 6/8 of the series.

I will wait for Patrick's ack and resend the whole series again.

Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
---
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index a55a23a..23fa3f3 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -441,8 +441,10 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		dev = ipmr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		dev_hold(dev);
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
+			dev_put(dev);
 			unregister_netdevice(dev);
 			return err;
 		}
@@ -452,8 +454,10 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		dev = ipmr_new_tunnel(vifc);
 		if (!dev)
 			return -ENOBUFS;
+		dev_hold(dev);
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
+			dev_put(dev);
 			ipmr_del_tunnel(dev, vifc);
 			return err;
 		}
@@ -462,10 +466,11 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		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)
+		if (err) {
+			dev_put(dev);
 			return err;
+		}
 		break;
 	default:
 		return -EINVAL;
@@ -496,7 +501,6 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 
 	/* And finish update writing critical data */
 	write_lock_bh(&mrt_lock);
-	dev_hold(dev);
 	v->dev=dev;
 #ifdef CONFIG_IP_PIMSM
 	if (v->flags&VIFF_REGISTER)
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
index 6cd286d..a9bd74d 100644
--- a/net/ipv6/ip6mr.c
+++ b/net/ipv6/ip6mr.c
@@ -621,8 +621,10 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		dev = ip6mr_reg_vif();
 		if (!dev)
 			return -ENOBUFS;
+		dev_hold(dev);
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
+			dev_put(dev);
 			unregister_netdevice(dev);
 			return err;
 		}
@@ -632,10 +634,11 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		dev = dev_get_by_index(&init_net, vifc->mif6c_pifi);
 		if (!dev)
 			return -EADDRNOTAVAIL;
-		dev_put(dev);
 		err = dev_set_allmulti(dev, 1);
-		if (err)
+		if (err) {
+			dev_put(dev);
 			return err;
+		}
 		break;
 	default:
 		return -EINVAL;
@@ -659,7 +662,6 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 
 	/* And finish update writing critical data */
 	write_lock_bh(&mrt_lock);
-	dev_hold(dev);
 	v->dev = dev;
 #ifdef CONFIG_IPV6_PIMSM_V2
 	if (v->flags & MIFF_REGISTER)

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

[Wang Chen]

Wang Chen said the following on 2008-7-8 17:41:
> Patrick McHardy said the following on 2008-7-7 19:22:
>>>      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;
>> Also looks like a use after free, but again, one that is
>> already present without your patch.
>>
> 
> Here is the patch for fixing use after free.
> It fixes both ipv4 and ipv6 side and on top of my patches.
> This patch will be the 6/8 of the series.
> 

David, Patrick.
Please ignore this one.

v3 [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)

v3 [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)

[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

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

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>