v4 [PATCH 1/9] af_packet: Check return of dev_set_promiscuity/allmulti

PATCH In review Wang Chen 2 participants 23 messages Jul 15, 2008 – Jul 17, 2008 1 message not in the archive

Subsystems (6)

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

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Wang Chen

author

2008-07-15 03:04 UTC #1 lore raw

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

---

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

David Miller

maintainer Applied

2008-07-15 03:51 UTC #2 lore raw

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:09 +0800

Quoted 9 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.
Signed-off-by Wang Chen <[email protected]>
Acked-by Patrick McHardy <[email protected]>

Applied, thanks.

What are the exact locking rules for po->mclist btw? Obviously changes to the list are RTNL protected, but what about readers?

If readers lock differently, this error recovery where we unlink 'i' could cause an OOPS. It is ok to add new elements while allowing readers to traverse independantly but removal needs to interlock with such readers.

Wang Chen

author

2008-07-15 07:57 UTC #3 lore raw

PATCH 1/1 af_packet: po->mclist needs locker in reader side(WAS: [PATCH 1/9] af_packet: Check return of dev_set_promiscuity/allmulti)

David Miller said the following on 2008-7-15 11:51:

Quoted 15 lines

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:09 +0800
Quoted 9 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.
Signed-off-by Wang Chen <[email protected]>
Acked-by Patrick McHardy <[email protected]>
Applied, thanks.

Quoted 7 lines

What are the exact locking rules for po->mclist btw? Obviously changes to the list are RTNL protected, but what about readers?
If readers lock differently, this error recovery where we unlink 'i' could cause an OOPS. It is ok to add new elements while allowing readers to traverse independantly but removal needs to interlock with such readers.

As my understanding, packet_dev_mclist() is the only reader who doesn't use same lock as writers(actually it doesn't use any lock). My proposal is that use RTNL to prevent synchronous access to po->mclist. Because packet_dev_mclist() is only called when device be unregistered, the lock will not affect speed too much.

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

---

net/packet/af_packet.c +2 -0

index 2cee87d..9eb39f7 100644
@@ -1196,10 +1196,12 @@ static void packet_dev_mc(struct net_device *dev, struct packet_mclist *i, int w
 
 static void packet_dev_mclist(struct net_device *dev, struct packet_mclist *i, int what)
 {
+	rtnl_lock();
 	for ( ; i; i=i->next) {
 		if (i->ifindex == dev->ifindex)
 			packet_dev_mc(dev, i, what);
 	}
+	rtnl_unlock();
 }
 
 static int packet_mc_add(struct sock *sk, struct packet_mreq_max *mreq)

David Miller

maintainer

2008-07-15 09:11 UTC #4 lore raw

Re: [PATCH] af_packet: po->mclist needs locker in reader side

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 15:52:31 +0800

Quoted 7 lines

As my understanding, packet_dev_mclist() is the only reader who doesn't use same lock as writers(actually it doesn't use any lock). My proposal is that use RTNL to prevent synchronous access to po->mclist. Because packet_dev_mclist() is only called when device be unregistered, the lock will not affect speed too much.
Signed-off-by Wang Chen <[email protected]>

I took a look and it turns out that the RTNL lock is already held when that notifier runs, so it seems everything is fine here already.

So I'll ignore this patch :)

Wang Chen

author

2008-07-15 10:37 UTC #5 lore raw

Re: [PATCH] af_packet: po->mclist needs locker in reader side

David Miller said the following on 2008-7-15 17:11:

Quoted 9 lines

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 15:52:31 +0800
Quoted 6 lines
As my understanding, packet_dev_mclist() is the only reader who doesn't use same lock as writers(actually it doesn't use any lock). My proposal is that use RTNL to prevent synchronous access to po->mclist. Because packet_dev_mclist() is only called when device be unregistered, the lock will not affect speed too much.

Quoted 6 lines

Quoted 1 line
Signed-off-by Wang Chen <[email protected]>
I took a look and it turns out that the RTNL lock is already held when that notifier runs, so it seems everything is fine here already.
So I'll ignore this patch :)

Yes. register_netdevice() holds the lock ;)

Wang Chen

author

2008-07-15 03:04 UTC #6 lore raw

v4 [PATCH 2/9] bonding: Check return of dev_set_promiscuity/allmulti

In reply to a message not in the archive

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

David Miller

maintainer Applied

2008-07-15 03:51 UTC #7 lore raw

Re: v4 [PATCH 2/9] bonding: Check return of dev_set_promiscuity/allmulti

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:16 +0800

Quoted 13 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 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.

Quoted 1 line

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

Applied.

Wang Chen

author

2008-07-15 03:04 UTC #8 lore raw

v4 [PATCH 3/9] bridge: Check return of dev_set_promiscuity

In reply to a message not in the archive

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

David Miller

maintainer Applied

2008-07-15 03:54 UTC #9 lore raw

Re: v4 [PATCH 3/9] bridge: Check return of dev_set_promiscuity

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:20 +0800

Quoted 8 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.
Signed-off-by Wang Chen <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>

Applied.

Although I had to apply this by hand since this wasn't generated against the current tree which has the new dev_disable_lro() calls in br.f

Wang Chen

author

2008-07-15 03:04 UTC #10 lore raw

v4 [PATCH 4/9] ipv6: Check return of dev_set_allmulti

In reply to a message not in the archive

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

---

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

David Miller

maintainer Applied

2008-07-15 03:54 UTC #11 lore raw

Re: v4 [PATCH 4/9] ipv6: Check return of dev_set_allmulti

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:25 +0800

Quoted 8 lines

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

Applied.

Wang Chen

author

2008-07-15 03:04 UTC #12 lore raw

v4 [PATCH 5/9] ipv6: Fix using after dev_put()

In reply to a message not in the archive

Patrick McHardy pointed out it.

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

---

net/ipv6/ip6mr.c +5 -3

index 6cd286d..faf6650 100644
@@ -451,6 +451,7 @@ static struct net_device *ip6mr_reg_vif(void)
 	if (dev_open(dev))
 		goto failure;
 
+	dev_hold(dev);
 	return dev;
 
 failure:

@@ -624,6 +625,7 @@ static int mif6_add(struct mif6ctl *vifc, int mrtsock)
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
 			unregister_netdevice(dev);
+			dev_put(dev);
 			return err;
 		}
 		break;

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

David Miller

maintainer Applied

2008-07-15 03:55 UTC #13 lore raw

Re: v4 [PATCH 5/9] ipv6: Fix using after dev_put()

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:29 +0800

Quoted 3 lines

Patrick McHardy pointed out it.
Signed-off-by Wang Chen <[email protected]>

Applied, thanks.

Wang Chen

author

2008-07-15 03:04 UTC #14 lore raw

v4 [PATCH 6/9] ipv4: Check return of dev_set_allmulti

In reply to a message not in the archive

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.

PS: For unwinding tunnel creating, we let ipip->ioctl() to handle it.

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

---

net/ipv4/ipmr.c +39 -1

index 11700a4..a55a23a 100644
@@ -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);
 
 	/*

David Miller

maintainer Applied

2008-07-15 03:55 UTC #15 lore raw

Re: v4 [PATCH 6/9] ipv4: Check return of dev_set_allmulti

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:35 +0800

Quoted 9 lines

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.
PS: For unwinding tunnel creating, we let ipip->ioctl() to handle it.
Signed-off-by Wang Chen <[email protected]>

Unwinding is ugly, but I can't suggest a better way right now :)

Applied, thanks.

Wang Chen

author

2008-07-15 03:04 UTC #16 lore raw

v4 [PATCH 7/9] ipv4: Fix ipmr unregister device oops

In reply to a message not in the archive

When I test the patch 6/9, oops happened during device unregister.

The following oops happened when I add two tunnels, which use a same device, and then delete one tunnel.
Obviously deleting tunnel "A" causes device unregister, which send a notification, and after receiving notification, ipmr do unregister again for tunnel "B" which also use same device. That is wrong.
After receiving notification, ipmr only needs to decrease reference count and don't do duplicated unregister.
Fortunately, IPv6 side doesn't add tunnel in ip6mr, so it's clean.

This patch fixs:
- unregister device oops
- using after dev_put()

Here is the oops:
===

Jul 11 15:39:29 wangchen kernel: ------------[ cut here ]------------
Jul 11 15:39:29 wangchen kernel: kernel BUG at net/core/dev.c:3651!
Jul 11 15:39:29 wangchen kernel: invalid opcode: 0000 [#1] 
Jul 11 15:39:29 wangchen kernel: Modules linked in: ipip tunnel4 nfsd lockd nfs_acl auth_rpcgss sunrpc exportfs ipv6 snd_pcm_oss snd_mixer_oss snd_seq snd_seq_device af_packet binfmt_misc button battery ac loop dm_mod usbhid ff_memless pcmcia firmware_class ohci1394 8139too mii ieee1394 yenta_socket rsrc_nonstatic pcmcia_core ide_cd_mod cdrom snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm i2c_i801 snd_timer snd i2c_core soundcore snd_page_alloc rng_core shpchp ehci_hcd uhci_hcd pci_hotplug intel_agp agpgart usbcore ext3 jbd ata_piix ahci libata dock edd fan thermal processor thermal_sys piix sd_mod scsi_mod ide_disk ide_core [last unloaded: freq_table]
Jul 11 15:39:29 wangchen kernel: 
Jul 11 15:39:29 wangchen kernel: Pid: 4102, comm: mroute Not tainted (2.6.26-rc9-default #69)
Jul 11 15:39:29 wangchen kernel: EIP: 0060:[<c024636b>] EFLAGS: 00010202 CPU: 0
Jul 11 15:39:29 wangchen kernel: EIP is at rollback_registered+0x61/0xe3
Jul 11 15:39:29 wangchen kernel: EAX: 00000001 EBX: ecba6000 ECX: 00000000 EDX: ffffffff
Jul 11 15:39:29 wangchen kernel: ESI: 00000001 EDI: ecba6000 EBP: c03de2e8 ESP: ed8e7c3c
Jul 11 15:39:29 wangchen kernel:  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
Jul 11 15:39:29 wangchen kernel: Process mroute (pid: 4102, ti=ed8e6000 task=ed41e830 task.ti=ed8e6000)
Jul 11 15:39:29 wangchen kernel: Stack: ecba6000 c024641c 00000028 c0284e1a 00000001 c03de2e8 ecba6000 eecff360 
Jul 11 15:39:29 wangchen kernel:        c0284e4c c03536f4 fffffff8 00000000 c029a819 ecba6000 00000006 ecba6000 
Jul 11 15:39:29 wangchen kernel:        00000000 ecba6000 c03de2c0 c012841b ffffffff 00000000 c024639f ecba6000 
Jul 11 15:39:29 wangchen kernel: Call Trace:
Jul 11 15:39:29 wangchen kernel:  [<c024641c>] unregister_netdevice+0x2f/0x51
Jul 11 15:39:29 wangchen kernel:  [<c0284e1a>] vif_delete+0xaf/0xc3
Jul 11 15:39:29 wangchen kernel:  [<c0284e4c>] ipmr_device_event+0x1e/0x30
Jul 11 15:39:29 wangchen kernel:  [<c029a819>] notifier_call_chain+0x2a/0x47
Jul 11 15:39:29 wangchen kernel:  [<c012841b>] raw_notifier_call_chain+0x9/0xc
Jul 11 15:39:29 wangchen kernel:  [<c024639f>] rollback_registered+0x95/0xe3
Jul 11 15:39:29 wangchen kernel:  [<c024641c>] unregister_netdevice+0x2f/0x51
Jul 11 15:39:29 wangchen kernel:  [<c0284e1a>] vif_delete+0xaf/0xc3
Jul 11 15:39:29 wangchen kernel:  [<c0285eee>] ip_mroute_setsockopt+0x47a/0x801
Jul 11 15:39:29 wangchen kernel:  [<eea5a70c>] do_get_write_access+0x2df/0x313 [jbd]
Jul 11 15:39:29 wangchen kernel:  [<c01727c4>] __find_get_block_slow+0xda/0xe4
Jul 11 15:39:29 wangchen kernel:  [<c0172a7f>] __find_get_block+0xf8/0x122
Jul 11 15:39:29 wangchen kernel:  [<c0172a7f>] __find_get_block+0xf8/0x122
Jul 11 15:39:29 wangchen kernel:  [<eea5d563>] journal_cancel_revoke+0xda/0x110 [jbd]
Jul 11 15:39:29 wangchen kernel:  [<c0263501>] ip_setsockopt+0xa9/0x9ee
Jul 11 15:39:29 wangchen kernel:  [<eea5d563>] journal_cancel_revoke+0xda/0x110 [jbd]
Jul 11 15:39:29 wangchen kernel:  [<eea5a70c>] do_get_write_access+0x2df/0x313 [jbd]
Jul 11 15:39:29 wangchen kernel:  [<eea69287>] __ext3_get_inode_loc+0xcf/0x271 [ext3]
Jul 11 15:39:29 wangchen kernel:  [<eea743c7>] __ext3_journal_dirty_metadata+0x13/0x32 [ext3]
Jul 11 15:39:29 wangchen kernel:  [<c0116434>] __wake_up+0xf/0x15
Jul 11 15:39:29 wangchen kernel:  [<eea5a424>] journal_stop+0x1bd/0x1c6 [jbd]
Jul 11 15:39:29 wangchen kernel:  [<eea703a7>] __ext3_journal_stop+0x19/0x34 [ext3]
Jul 11 15:39:29 wangchen kernel:  [<c014291e>] get_page_from_freelist+0x94/0x369
Jul 11 15:39:29 wangchen kernel:  [<c01408f2>] filemap_fault+0x1ac/0x2fe
Jul 11 15:39:29 wangchen kernel:  [<c01a605e>] security_sk_alloc+0xd/0xf
Jul 11 15:39:29 wangchen kernel:  [<c023edea>] sk_prot_alloc+0x36/0x78
Jul 11 15:39:29 wangchen kernel:  [<c0240037>] sk_alloc+0x3a/0x40
Jul 11 15:39:29 wangchen kernel:  [<c0276062>] raw_hash_sk+0x46/0x4e
Jul 11 15:39:29 wangchen kernel:  [<c0166aff>] d_alloc+0x1b/0x157
Jul 11 15:39:29 wangchen kernel:  [<c023e4d1>] sock_common_setsockopt+0x12/0x16
Jul 11 15:39:29 wangchen kernel:  [<c023cb1e>] sys_setsockopt+0x6f/0x8e
Jul 11 15:39:29 wangchen kernel:  [<c023e105>] sys_socketcall+0x15c/0x19e
Jul 11 15:39:29 wangchen kernel:  [<c0103611>] sysenter_past_esp+0x6a/0x99
Jul 11 15:39:29 wangchen kernel:  [<c0290000>] unix_poll+0x69/0x78
Jul 11 15:39:29 wangchen kernel:  =======================
Jul 11 15:39:29 wangchen kernel: Code: 83 e0 01 00 00 85 c0 75 1f 53 53 68 12 81 31 c0 e8 3c 30 ed ff ba 3f 0e 00 00 b8 b9 7f 31 c0 83 c4 0c 5b e9 f5 26 ed ff 48 74 04 <0f> 0b eb fe 89 d8 e8 21 ff ff ff 89 d8 e8 62 ea ff ff c7 83 e0 
Jul 11 15:39:29 wangchen kernel: EIP: [<c024636b>] rollback_registered+0x61/0xe3 SS:ESP 0068:ed8e7c3c
Jul 11 15:39:29 wangchen kernel: ---[ end trace c311acf85d169786 ]---

===

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

---

net/ipv4/ipmr.c +14 -8

index a55a23a..9b35566 100644
@@ -186,6 +186,7 @@ struct net_device *ipmr_new_tunnel(struct vifctl *v)
 
 			if (dev_open(dev))
 				goto failure;
+			dev_hold(dev);
 		}
 	}
 	return dev;

@@ -259,6 +260,8 @@ static struct net_device *ipmr_reg_vif(void)
 	if (dev_open(dev))
 		goto failure;
 
+	dev_hold(dev);
+
 	return dev;
 
 failure:

@@ -273,9 +276,10 @@ failure:
 
 /*
  *	Delete a VIF entry
+ *	@notify: Set to 1, if the caller is a notifier_call
  */
 
-static int vif_delete(int vifi)
+static int vif_delete(int vifi, int notify)
 {
 	struct vif_device *v;
 	struct net_device *dev;

@@ -318,7 +322,7 @@ static int vif_delete(int vifi)
 		ip_rt_multicast_event(in_dev);
 	}
 
-	if (v->flags&(VIFF_TUNNEL|VIFF_REGISTER))
+	if (v->flags&(VIFF_TUNNEL|VIFF_REGISTER) && !notify)
 		unregister_netdevice(dev);
 
 	dev_put(dev);

@@ -444,6 +448,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
 			unregister_netdevice(dev);
+			dev_put(dev);
 			return err;
 		}
 		break;

@@ -455,6 +460,7 @@ static int vif_add(struct vifctl *vifc, int mrtsock)
 		err = dev_set_allmulti(dev, 1);
 		if (err) {
 			ipmr_del_tunnel(dev, vifc);
+			dev_put(dev);
 			return err;
 		}
 		break;

@@ -462,10 +468,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 +503,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)

@@ -843,7 +849,7 @@ static void mroute_clean_tables(struct sock *sk)
 	 */
 	for (i=0; i<maxvif; i++) {
 		if (!(vif_table[i].flags&VIFF_STATIC))
-			vif_delete(i);
+			vif_delete(i, 0);
 	}
 
 	/*

@@ -956,7 +962,7 @@ int ip_mroute_setsockopt(struct sock *sk,int optname,char __user *optval,int opt
 		if (optname==MRT_ADD_VIF) {
 			ret = vif_add(&vif, sk==mroute_socket);
 		} else {
-			ret = vif_delete(vif.vifc_vifi);
+			ret = vif_delete(vif.vifc_vifi, 0);
 		}
 		rtnl_unlock();
 		return ret;

@@ -1135,7 +1141,7 @@ static int ipmr_device_event(struct notifier_block *this, unsigned long event, v
 	v=&vif_table[0];
 	for (ct=0;ct<maxvif;ct++,v++) {
 		if (v->dev==dev)
-			vif_delete(ct);
+			vif_delete(ct, 1);
 	}
 	return NOTIFY_DONE;
 }

David Miller

maintainer Applied

2008-07-15 03:56 UTC #17 lore raw

Re: v4 [PATCH 7/9] ipv4: Fix ipmr unregister device oops

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:40 +0800

Quoted 2 lines

When I test the patch 6/9, oops happened during device unregister.

...

Quoted 1 line

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

Applied, thanks.

Wang Chen

author

2008-07-15 03:04 UTC #18 lore raw

v4 [PATCH 8/9] macvlan: Check return of dev_set_allmulti

In reply to a message not in the archive

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)

David Miller

maintainer Applied

2008-07-15 03:57 UTC #19 lore raw

Re: v4 [PATCH 8/9] macvlan: Check return of dev_set_allmulti

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:45 +0800

Quoted 8 lines

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

Applied, thanks.

Wang Chen

author

2008-07-15 03:04 UTC #20 lore raw

v4 [PATCH 9/9] 8021q: Check return of dev_set_promiscuity/allmulti

In reply to a message not in the archive

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)

David Miller

maintainer Applied

2008-07-15 04:00 UTC #21 lore raw

Re: v4 [PATCH 9/9] 8021q: Check return of dev_set_promiscuity/allmulti

From: Wang Chen <[email protected]>
Date: Tue, 15 Jul 2008 10:59:49 +0800

Quoted 9 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 all positive increment for promiscuity and allmulti to get error return.
Signed-off-by Wang Chen <[email protected]>
Acked-by Patrick McHardy <[email protected]>

Applied.

But I had to apply this one also by hand, as it conflicted with Patrick's GVRP changes already in the tree.

When you are maintaining a patch set like this, you have to update your tree and regenerate your patches in order to make sure no new conflicts exist before submitting your patches.

That part isn't my job :)

Thanks.

Wang Chen

author

2008-07-15 06:09 UTC #22 lore raw

Re: v4 [PATCH 9/9] 8021q: Check return of dev_set_promiscuity/allmulti

David Miller said the following on 2008-7-15 12:00:

Quoted 3 lines

But I had to apply this one also by hand, as it conflicted with Patrick's GVRP changes already in the tree.

Sorry*2 for making you typing my patch twice. ;)

Quoted 7 lines

When you are maintaining a patch set like this, you have to update your tree and regenerate your patches in order to make sure no new conflicts exist before submitting your patches.
That part isn't my job :)
Thanks.

OK.
But I need some help from you, otherwise the latest tree to me is Linus's mainline tree.
Actually, long time ago, I want to git-clone your net tree but failed. But I can git-clone Linus's.
The reason is that in my working place I can only use http protocol when git clone. But your tree seems not work for http git-clone.
I asked Herbert before, he said, maybe git-update-server-info needs to be executed.
I am very appreciated if you can figure out why net tree can't be cloned by http. And maybe there are some other people whose employer's firewall filtering git port.
Thank you for your understanding in advance :)

Wang Chen

author

2008-07-17 05:46 UTC #23 lore raw

git clone net-next-2.6

Wang Chen said the following on 2008-7-15 14:04:

Quoted 4 lines

But I need some help from you, otherwise the latest tree to me is Linus's mainline tree.
Actually, long time ago, I want to git-clone your net tree but failed. But I can git-clone Linus's.

Quoted 9 lines

The reason is that in my working place I can only use http protocol when git clone. But your tree seems not work for http git-clone.
I asked Herbert before, he said, maybe git-update-server-info needs to be executed.
I am very appreciated if you can figure out why net tree can't be cloned by http. And maybe there are some other people whose employer's firewall filtering git port.
Thank you for your understanding in advance :)

David, git-clone your net-next-2.6 tree by http can work now. But, it failed half way.
BTW, I am not urgent :)

---

# git-clone http://www.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
Initialized empty Git repository in /pub/git/wcn/davem/net-next-2.6/.git/
Getting alternates list for http://www.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
Also look at http://www.kernel.org/home/ftp/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/
Getting pack list for http://www.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
Getting index for pack b4c5447ecf8fe5dd18e4403f6628e52579ba93aa
Getting pack b4c5447ecf8fe5dd18e4403f6628e52579ba93aa
 which contains 79d16385c7f287a33ea771c4dbe60ae43f791b49
walk 79d16385c7f287a33ea771c4dbe60ae43f791b49
walk b19fa1fa91845234961c64dbd564671aa7c0fd27
Getting pack list for http://www.kernel.org/home/ftp/pub/scm/linux/kernel/git/torvalds/linux-2.6.git/
error: Unable to find 869e1a3b64b6bf969eeced820691e955e03e3068 under http://www.kernel.org/pub/scm/linux/kernel/git/davem/net-next-2.6.git
Cannot obtain needed blob 869e1a3b64b6bf969eeced820691e955e03e3068
while processing commit b19fa1fa91845234961c64dbd564671aa7c0fd27.