All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity
@ 2008-06-20  0:55 Wang Chen
  2008-06-20  1:18 ` Stephen Hemminger
  2008-06-20  2:08 ` [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity David Miller
  0 siblings, 2 replies; 6+ messages in thread
From: Wang Chen @ 2008-06-20  0:55 UTC (permalink / raw)
  To: David S. Miller; +Cc: Stephen Hemminger, NETDEV, Patrick McHardy

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 <wangchen@cn.fujitsu.com>
---
 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(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c2397f5..49dd433 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -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:
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index c11b554..c1bbfea 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -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 */
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 02b2d50..7f80462 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -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);
+}
-- 
1.5.3.4





^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity
  2008-06-20  0:55 [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity Wang Chen
@ 2008-06-20  1:18 ` Stephen Hemminger
  2008-06-20  2:32   ` Wang Chen
  2008-06-20  2:08 ` [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity David Miller
  1 sibling, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2008-06-20  1:18 UTC (permalink / raw)
  To: Wang Chen; +Cc: David S. Miller, NETDEV, Patrick McHardy

On Fri, 20 Jun 2008 08:55:00 +0800
Wang Chen <wangchen@cn.fujitsu.com> 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.
> 
> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>

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)

--- a/net/bridge/br_if.c	2008-06-19 18:09:59.000000000 -0700
+++ b/net/bridge/br_if.c	2008-06-19 18:16:44.000000000 -0700
@@ -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;
 }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity
  2008-06-20  0:55 [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity Wang Chen
  2008-06-20  1:18 ` Stephen Hemminger
@ 2008-06-20  2:08 ` David Miller
  1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2008-06-20  2:08 UTC (permalink / raw)
  To: wangchen; +Cc: shemminger, netdev, kaber

From: Wang Chen <wangchen@cn.fujitsu.com>
Date: Fri, 20 Jun 2008 08:55:00 +0800

Sigh...

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity
  2008-06-20  1:18 ` Stephen Hemminger
@ 2008-06-20  2:32   ` Wang Chen
  2008-06-21  3:48     ` [PATCH] bridge: Check return of dev_set_promiscuity (v2) Stephen Hemminger
  0 siblings, 1 reply; 6+ messages in thread
From: Wang Chen @ 2008-06-20  2:32 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David S. Miller, NETDEV, Patrick McHardy

Stephen Hemminger said the following on 2008-6-20 9:18:
> On Fri, 20 Jun 2008 08:55:00 +0800
> Wang Chen <wangchen@cn.fujitsu.com> 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.
>>
>> Signed-off-by: Wang Chen <wangchen@cn.fujitsu.com>
> 
> 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 <wangchen@cn.fujitsu.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/bridge/br_if.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c2397f5..805dbb7 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);
 
@@ -418,6 +421,7 @@ err0:
 	kobject_put(&p->kobj);
 
 put_back:
+	dev_set_promiscuity(dev, -1);
 	dev_put(dev);
 	return err;
 }
-- 
1.5.3.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] bridge: Check return of dev_set_promiscuity (v2)
  2008-06-20  2:32   ` Wang Chen
@ 2008-06-21  3:48     ` Stephen Hemminger
  2008-06-21  5:45       ` [PATCH] bridge: Check return of dev_set_promiscuity (v3) Wang Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2008-06-21  3:48 UTC (permalink / raw)
  To: David S. Miller; +Cc: Wang Chen, NETDEV, Patrick McHardy

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 <shemminger@vyatta.com>

--- a/net/bridge/br_if.c	2008-06-20 15:57:51.000000000 -0700
+++ b/net/bridge/br_if.c	2008-06-20 16:15:47.000000000 -0700
@@ -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;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] bridge: Check return of dev_set_promiscuity (v3)
  2008-06-21  3:48     ` [PATCH] bridge: Check return of dev_set_promiscuity (v2) Stephen Hemminger
@ 2008-06-21  5:45       ` Wang Chen
  0 siblings, 0 replies; 6+ messages in thread
From: Wang Chen @ 2008-06-21  5:45 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: David S. Miller, NETDEV, Patrick McHardy

Stephen Hemminger said the following on 2008-6-21 11:48:
> 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 <shemminger@vyatta.com>
> 

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.

> --- a/net/bridge/br_if.c	2008-06-20 15:57:51.000000000 -0700
> +++ b/net/bridge/br_if.c	2008-06-20 16:15:47.000000000 -0700
> @@ -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;

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

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

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

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index c2397f5..58bbfb8 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -375,6 +375,10 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 	if (IS_ERR(p))
 		return PTR_ERR(p);
 
+	err = dev_set_promiscuity(dev, 1);
+	if (err)
+		goto put_back;
+
 	err = kobject_init_and_add(&p->kobj, &brport_ktype, &(dev->dev.kobj),
 				   SYSFS_BRIDGE_PORT_ATTR);
 	if (err)
@@ -389,7 +393,6 @@ int br_add_if(struct net_bridge *br, struct net_device *dev)
 		goto err2;
 
 	rcu_assign_pointer(dev->br_port, p);
-	dev_set_promiscuity(dev, 1);
 
 	list_add_rcu(&p->list, &br->port_list);
 
@@ -413,12 +416,12 @@ err2:
 	br_fdb_delete_by_port(br, p, 1);
 err1:
 	kobject_del(&p->kobj);
-	goto put_back;
 err0:
 	kobject_put(&p->kobj);
-
+	dev_set_promiscuity(dev, -1);
 put_back:
 	dev_put(dev);
+	kfree(p);
 	return err;
 }
 
-- 
1.5.3.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2008-06-21  5:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-06-20  0:55 [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity Wang Chen
2008-06-20  1:18 ` Stephen Hemminger
2008-06-20  2:32   ` Wang Chen
2008-06-21  3:48     ` [PATCH] bridge: Check return of dev_set_promiscuity (v2) Stephen Hemminger
2008-06-21  5:45       ` [PATCH] bridge: Check return of dev_set_promiscuity (v3) Wang Chen
2008-06-20  2:08 ` [PATCH net-next 3/8] bridge: Check return of dev_set_promiscuity David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.