Skip to contentSkip to conversation

8021q: Check return of dev_set_promiscuity/allmulti

PATCH In review Wang Chen 2 participants 6 messages Jun 20, 2008 – Jun 24, 2008

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 8/8 8021q: Check return of dev_set_promiscuity/allmulti

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

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

Signed-off-by Wang Chen <[email protected]>
---
 net/8021q/vlan_dev.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)
net/8021q/vlan_dev.c +15 -4
index 5d055c2..1f2669b 100644
@@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev)
 	}
 	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 out;
+	}
+	if (dev->flags & IFF_PROMISC) {
+		err = dev_set_promiscuity(real_dev, 1);
+		if (err < 0)
+			goto out;
+	}
 
 	return 0;
+
+out:
+	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+		dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+	return err;
 }
 
 static int vlan_dev_stop(struct net_device *dev)
Signature · 1 line
1.5.3.4
#1 lore raw
Quoted 19 lines · Wang Chen
net/8021q/vlan_dev.c +7 -4
index 5d055c2..1f2669b 100644
@@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev)
 	}
 	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 out;
+	}
+	if (dev->flags & IFF_PROMISC) {
+		err = dev_set_promiscuity(real_dev, 1);

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 566

Quoted 10 lines
net/8021q/vlan_dev.c +8 -0
+		if (err < 0)
+			goto out;
+	}
 
 	return 0;
+
+out:
+	if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+		dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+	return err;

This doesn't undo the operations that already succeeded.

author
#2 lore raw

Patrick McHardy said the following on 2008-6-20 17:47:

Quoted 24 lines
Quoted 23 lines · Wang Chen
net/8021q/vlan_dev.c +10 -4
index 5d055c2..1f2669b 100644
@@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev)
     }
     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 out;
+    }
+    if (dev->flags & IFF_PROMISC) {
+        err = dev_set_promiscuity(real_dev, 1);
+        if (err < 0)
+            goto out;
+    }
Quoted 9 lines

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 566

Quoted 6 lines
net/8021q/vlan_dev.c +5 -0
     return 0;
+
+out:
+    if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+        dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+    return err;

This doesn't undo the operations that already succeeded.

Why?
Should I do

 	dev_mc_unsync(real_dev, dev);
	dev_unicast_unsync(real_dev, dev);

before dev_unicast_delete()?

#3 lore raw
Quoted 34 lines · Wang Chen

Patrick McHardy said the following on 2008-6-20 17:47:

Quoted 24 lines
Quoted 23 lines · Wang Chen
net/8021q/vlan_dev.c +10 -4
index 5d055c2..1f2669b 100644
@@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev)
     }
     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 out;
+    }
+    if (dev->flags & IFF_PROMISC) {
+        err = dev_set_promiscuity(real_dev, 1);
+        if (err < 0)
+            goto out;
+    }
Quoted 8 lines

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 566

Quoted 6 lines
net/8021q/vlan_dev.c +5 -0
     return 0;
+
+out:
+    if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+        dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+    return err;

This doesn't undo the operations that already succeeded.

Quoted 5 lines

Why?
Should I do

 	dev_mc_unsync(real_dev, dev);
	dev_unicast_unsync(real_dev, dev);

before dev_unicast_delete()?

When returning an error, the state shouldn't change at all, so you need to:

- remove unicast address if changed
- restore old address in vlan->real_dev_addr
- undo dev_set_allmulti

author
#4 lore raw

Patrick McHardy said the following on 2008-6-20 18:08:

Quoted 41 lines
Quoted 34 lines · Wang Chen

Patrick McHardy said the following on 2008-6-20 17:47:

Quoted 24 lines
Quoted 23 lines · Wang Chen
net/8021q/vlan_dev.c +10 -4
index 5d055c2..1f2669b 100644
@@ -547,12 +547,23 @@ static int vlan_dev_open(struct net_device *dev)
     }
     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 out;
+    }
+    if (dev->flags & IFF_PROMISC) {
+        err = dev_set_promiscuity(real_dev, 1);
+        if (err < 0)
+            goto out;
+    }
Quoted 8 lines

net/8021q/vlan_dev.c static int vlan_dev_open(struct net_device *dev) · line 566

Quoted 6 lines
net/8021q/vlan_dev.c +5 -0
     return 0;
+
+out:
+    if (compare_ether_addr(dev->dev_addr, real_dev->dev_addr))
+        dev_unicast_delete(real_dev, dev->dev_addr, dev->addr_len);
+    return err;

This doesn't undo the operations that already succeeded.

Quoted 5 lines

Why?
Should I do

     dev_mc_unsync(real_dev, dev);
    dev_unicast_unsync(real_dev, dev);

before dev_unicast_delete()?

Quoted 7 lines

When returning an error, the state shouldn't change at all, so you need to:

- remove unicast address if changed
- restore old address in vlan->real_dev_addr
- undo dev_set_allmulti

Too much cleanup job!
Let me think, how about move dev_set_* ahead of memcpy(). Because unwinding dev_set_* is simpler than restoring old real_dev_addr.

---
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]>
---
 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)
Signature · 1 line
1.5.3.4
Acked-by
#5 lore raw
Quoted 8 lines · Wang Chen

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

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