* [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 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
* 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
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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).