Skip to contentSkip to conversation

bridge: Check return of dev_set_promiscuity

PATCH In review Wang Chen 3 participants 6 messages Jun 20, 2008 – Jun 21, 2008

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Wang Chen

author
#0 lore raw

PATCH 3/8 bridge: Check return of dev_set_promiscuity

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.

BTW, I have to add a function to handle cleanup for br_sysfs_addif(). I know Stephen has removed br_sysfs_removeif() to keep kobjects in use even if not using sysfs.
But here we have a new br_sysfs_removeif() which only cleanup the sysfs and doesn't touch kobjects.

Signed-off-by Wang Chen <[email protected]>
---
 net/bridge/br_if.c       |    7 ++++++-
 net/bridge/br_private.h  |    2 ++
 net/bridge/br_sysfs_if.c |   17 +++++++++++++++++
 3 files changed, 25 insertions(+), 1 deletions(-)
net/bridge/br_if.c +6 -1
index c2397f5..49dd433 100644
@@ -389,7 +389,10 @@ 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);
+	/* If promiscuity overflow, return error */
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto err3;
 
 	list_add_rcu(&p->list, &br->port_list);
 

@@ -409,6 +412,8 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
+err3:
+	br_sysfs_removeif(p);
 err2:
 	br_fdb_delete_by_port(br, p, 1);
 err1:
net/bridge/br_private.h +2 -0
index c11b554..c1bbfea 100644
@@ -250,6 +250,7 @@ extern void br_ifinfo_notify(int event, struct net_bridge_port *port);
 /* br_sysfs_if.c */
 extern struct sysfs_ops brport_sysfs_ops;
 extern int br_sysfs_addif(struct net_bridge_port *p);
+extern void br_sysfs_removeif(struct net_bridge_port *p);
 
 /* br_sysfs_br.c */
 extern int br_sysfs_addbr(struct net_device *dev);

@@ -258,6 +259,7 @@ extern void br_sysfs_delbr(struct net_device *dev);
 #else
 
 #define br_sysfs_addif(p)	(0)
+#define br_sysfs_removeif(p)	do { } while (0)
 #define br_sysfs_addbr(dev)	(0)
 #define br_sysfs_delbr(dev)	do { } while(0)
 #endif /* CONFIG_SYSFS */
net/bridge/br_sysfs_if.c +17 -0
index 02b2d50..7f80462 100644
@@ -233,3 +233,20 @@ int br_sysfs_addif(struct net_bridge_port *p)
 out2:
 	return err;
 }
+
+/*
+ * Remove sysfs entries of ethernet device added to a bridge.
+ * Revert all the things be done by br_sysfs_addif().
+ */
+void br_sysfs_removeif(struct net_bridge_port *p)
+{
+	struct net_bridge *br = p->br;
+	struct brport_attribute **a;
+
+	sysfs_remove_link(&p->kobj, p->dev->name);
+
+	for (a = brport_attrs; *a; ++a)
+		sysfs_remove_file(&p->kobj, &((*a)->attr));
+
+	sysfs_remove_link(&br->dev->dev.kobj, SYSFS_BRIDGE_PORT_LINK);
+}
Signature · 1 line
1.5.3.4
#1 lore raw
Quoted 6 lines · On Fri, 20 Jun 2008 08:55:00 +0800 Wang Chen <[email protected]> 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.

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

Quoted 7 lines

BTW, I have to add a function to handle cleanup for br_sysfs_addif(). I know Stephen has removed br_sysfs_removeif() to keep kobjects in use even if not using sysfs.
But here we have a new br_sysfs_removeif() which only cleanup the sysfs and doesn't touch kobjects.

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

Not that hard way please. Instead just rearrange the order of setup to avoid having to do the things that are harder to unwind. Sysfs calls seem to be more fragile/error prone than the simple counters.

Like this (untested)

net/bridge/br_if.c +6 -1
@@ -373,6 +373,10 @@ int br_add_if(struct net_bridge *br, str
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto put_kobj;
+
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
 	if (err)

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

@@ -416,6 +419,8 @@ err0:
 	kobject_put(&p->kobj);
 
 put_back:
+	dev_set_promiscuity(dev, -1);
+put_dev:
 	dev_put(dev);
 	return err;
 }
author
#2 lore raw

Stephen Hemminger said the following on 2008-6-20 9:18:

Quoted 15 lines
Quoted 12 lines · On Fri, 20 Jun 2008 08:55:00 +0800 Wang Chen <[email protected]> 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.

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

BTW, I have to add a function to handle cleanup for br_sysfs_addif(). I know Stephen has removed br_sysfs_removeif() to keep kobjects in use even if not using sysfs.
But here we have a new br_sysfs_removeif() which only cleanup the sysfs and doesn't touch kobjects.

Quoted 6 lines
Quoted 1 line
Signed-off-by Wang Chen <[email protected]>

Not that hard way please. Instead just rearrange the order of setup to avoid having to do the things that are harder to unwind. Sysfs calls seem to be more fragile/error prone than the simple counters.

Yes. Thank you for the good advice.
And, Can I add a SOF of you?

---
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 Patrick McHardy <[email protected]>
---
 net/bridge/br_if.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
net/bridge/br_if.c +5 -1
index c2397f5..805dbb7 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);
 

@@ -418,6 +421,7 @@ err0:
 	kobject_put(&p->kobj);
 
 put_back:
+	dev_set_promiscuity(dev, -1);
 	dev_put(dev);
 	return err;
 }
Signature · 1 line
1.5.3.4
#3 lore raw

PATCH 1/1 bridge: Check return of dev_set_promiscuity (v2)

Alternate version of Wang Chen's patch to handle dev_set_promiscuity errors. I tried and make unwinds clearer here. The kobject_put can just be replace by immediate kfree since object never gets registered if going through that path.

Signed-off-by Stephen Hemminger <[email protected]>
net/bridge/br_if.c +11 -4
@@ -373,10 +373,15 @@ int br_add_if(struct net_bridge *br, str
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto err_free;
+
+
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
 	if (err)
-		goto err0;
+		goto err_unset_promiscuity;
 
 	err = br_fdb_insert(br, p, dev->dev_addr);
 	if (err)

@@ -388,7 +393,6 @@ int br_add_if(struct net_bridge *br, str
 
 	rcu_assign_pointer(dev->br_port, p);
 	dev_disable_lro(dev);
-	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);
 

@@ -408,14 +412,17 @@ int br_add_if(struct net_bridge *br, str
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
+
 err2:
 	br_fdb_delete_by_port(br, p, 1);
 err1:
 	kobject_del(&p->kobj);
 	goto put_back;
-err0:
-	kobject_put(&p->kobj);
 
+err_unset_promiscuity:
+	dev_set_promiscuity(dev, -1);
+err_free:
+	kfree(p);
 put_back:
 	dev_put(dev);
 	return err;
author
#4 lore raw

PATCH 1/1 bridge: Check return of dev_set_promiscuity (v3)

Stephen Hemminger said the following on 2008-6-21 11:48:

Quoted 7 lines

Alternate version of Wang Chen's patch to handle dev_set_promiscuity errors. I tried and make unwinds clearer here. The kobject_put can just be replace by immediate kfree since object never gets registered if going through that path.

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

oops, In my previous patch, I said I want to add your sof, but obviously I made a typo and add Patrick's sof instead of yours. Sorry for that.

Quoted 28 lines
net/bridge/br_if.c +6 -2
@@ -373,10 +373,15 @@ int br_add_if(struct net_bridge *br, str
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto err_free;
+
+
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
 	if (err)
-		goto err0;
+		goto err_unset_promiscuity;
 
 	err = br_fdb_insert(br, p, dev->dev_addr);
 	if (err)

@@ -388,7 +393,6 @@ int br_add_if(struct net_bridge *br, str
 
 	rcu_assign_pointer(dev->br_port, p);
 	dev_disable_lro(dev);
-	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);

net/bridge/br_if.c int br_add_if(struct net_bridge *br, str · line 420

Quoted 9 lines
net/bridge/br_if.c +1 -0
@@ -408,14 +412,17 @@ int br_add_if(struct net_bridge *br, str
 	kobject_uevent(&p->kobj, KOBJ_ADD);
 
 	return 0;
+
 err2:
 	br_fdb_delete_by_port(br, p, 1);
 err1:
 	kobject_del(&p->kobj);
 	goto put_back;

can't just goto put_back;
need to unset promiscuity.

net/bridge/br_if.c int br_add_if(struct net_bridge *br, str · line 421

Quoted 3 lines
net/bridge/br_if.c +0 -2
-err0:
-	kobject_put(&p->kobj);

Still needs kobject_put().
Because although kobject_init_and_add() failed,
part of it maybe done, such as init part.
And init of kobject will set kobj->ref to 1.

net/bridge/br_if.c int br_add_if(struct net_bridge *br, str · line 428

Quoted 8 lines
net/bridge/br_if.c +4 -0
+err_unset_promiscuity:
+	dev_set_promiscuity(dev, -1);
+err_free:
+	kfree(p);
 put_back:
 	dev_put(dev);
 	return err;

Here is v3. (Stephen, I put your SOF in, if you disagree, please let David know.)
- if kobject_init_and_add() failed, do kobject_put().
if error happen after kobject_init_and_add() being done, do kobjct_del and put.
- kfree after dev_put.

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;
 }
Signature · 1 line
1.5.3.4
maintainer
#5 lore raw
In reply to Wang Chen

From: Wang Chen <[email protected]>
Date: Fri, 20 Jun 2008 08:55:00 +0800

Sigh...

net/bridge/br_if.c int br_add_if(struct net_bridge *br, struct net_device *dev) · line 395

Quoted 9 lines
net/bridge/br_if.c +4 -1
@@ -389,7 +389,10 @@ 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);
+	/* If promiscuity overflow, return error */
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto err3;

Fixup this comment as I directed in my feedback for the previous patches. These just look like turds all over the tree that you are adding for every single dev_set_promiscuity() call site and it adds nothing to readability or understandability of the code.

Anyways, Stephen also wanted you to implement this a different way.