* [Bridge] Re: BUG: when using 'brctl stp' [not found] ` <20070812012037.0195b610.akpm@linux-foundation.org> @ 2007-08-14 13:11 ` Stephen Hemminger 2007-08-14 13:18 ` Lennert Buytenhek 0 siblings, 1 reply; 5+ messages in thread From: Stephen Hemminger @ 2007-08-14 13:11 UTC (permalink / raw) To: Andrew Morton, David S. Miller; +Cc: netdev, bridge Bridge locking for /sys/class/net/br0/bridge/stp_enabled was wrong. Another bug in bridge utilities makes it such that this interface, meant it wasn't being used. The locking needs to be removed from set_stp_state(), the lock is already acquired down in br_stp_start()/br_stp_stop. Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org> --- a/net/bridge/br_sysfs_br.c 2007-07-16 14:24:18.000000000 +0100 +++ b/net/bridge/br_sysfs_br.c 2007-08-14 13:44:23.000000000 +0100 @@ -150,9 +150,7 @@ static ssize_t show_stp_state(struct dev static void set_stp_state(struct net_bridge *br, unsigned long val) { rtnl_lock(); - spin_unlock_bh(&br->lock); br_stp_set_enabled(br, val); - spin_lock_bh(&br->lock); rtnl_unlock(); } ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bridge] Re: BUG: when using 'brctl stp' 2007-08-14 13:11 ` [Bridge] Re: BUG: when using 'brctl stp' Stephen Hemminger @ 2007-08-14 13:18 ` Lennert Buytenhek 2007-08-14 13:50 ` [Bridge] [PATCH] bridge: sysfs locking fix Stephen Hemminger 0 siblings, 1 reply; 5+ messages in thread From: Lennert Buytenhek @ 2007-08-14 13:18 UTC (permalink / raw) To: Stephen Hemminger; +Cc: netdev, Andrew Morton, David S. Miller, bridge On Tue, Aug 14, 2007 at 02:11:05PM +0100, Stephen Hemminger wrote: > Bridge locking for /sys/class/net/br0/bridge/stp_enabled > was wrong. Another bug in bridge utilities makes it such that > this interface, meant it wasn't being used. The locking needs > to be removed from set_stp_state(), the lock is already acquired > down in br_stp_start()/br_stp_stop. The 'locking' in set_stp_state() is actually dropping the lock around the br_stp_set_enabled() invocation, not acquiring it: > @@ -150,9 +150,7 @@ static ssize_t show_stp_state(struct dev > static void set_stp_state(struct net_bridge *br, unsigned long val) > { > rtnl_lock(); > - spin_unlock_bh(&br->lock); > br_stp_set_enabled(br, val); > - spin_lock_bh(&br->lock); > rtnl_unlock(); > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bridge] [PATCH] bridge: sysfs locking fix. 2007-08-14 13:18 ` Lennert Buytenhek @ 2007-08-14 13:50 ` Stephen Hemminger 2007-08-14 20:21 ` [Bridge] " David Miller 2007-08-24 8:18 ` Daniel Lezcano 0 siblings, 2 replies; 5+ messages in thread From: Stephen Hemminger @ 2007-08-14 13:50 UTC (permalink / raw) To: Lennert Buytenhek; +Cc: netdev, Andrew Morton, David S. Miller, bridge Forget earlier patch, it is wrong... The stp change code generates "sleeping function called from invalid context" because rtnl_lock() called with BH disabled. This fixes it by not acquiring then dropping the bridge lock. Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org> --- a/net/bridge/br_sysfs_br.c 2007-08-06 09:26:48.000000000 +0100 +++ b/net/bridge/br_sysfs_br.c 2007-08-14 14:29:52.000000000 +0100 @@ -147,20 +147,26 @@ static ssize_t show_stp_state(struct dev return sprintf(buf, "%d\n", br->stp_enabled); } -static void set_stp_state(struct net_bridge *br, unsigned long val) -{ - rtnl_lock(); - spin_unlock_bh(&br->lock); - br_stp_set_enabled(br, val); - spin_lock_bh(&br->lock); - rtnl_unlock(); -} static ssize_t store_stp_state(struct device *d, struct device_attribute *attr, const char *buf, size_t len) { - return store_bridge_parm(d, buf, len, set_stp_state); + struct net_bridge *br = to_bridge(d); + char *endp; + unsigned long val; + + if (!capable(CAP_NET_ADMIN)) + return -EPERM; + + val = simple_strtoul(buf, &endp, 0); + if (endp == buf) + return -EINVAL; + + rtnl_lock(); + br_stp_set_enabled(br, val); + rtnl_unlock(); + } static DEVICE_ATTR(stp_state, S_IRUGO | S_IWUSR, show_stp_state, store_stp_state); ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bridge] Re: [PATCH] bridge: sysfs locking fix. 2007-08-14 13:50 ` [Bridge] [PATCH] bridge: sysfs locking fix Stephen Hemminger @ 2007-08-14 20:21 ` David Miller 2007-08-24 8:18 ` Daniel Lezcano 1 sibling, 0 replies; 5+ messages in thread From: David Miller @ 2007-08-14 20:21 UTC (permalink / raw) To: shemminger; +Cc: netdev, akpm, bridge, buytenh From: Stephen Hemminger <shemminger@linux-foundation.org> Date: Tue, 14 Aug 2007 14:50:52 +0100 > Forget earlier patch, it is wrong... > > The stp change code generates "sleeping function called from invalid context" > because rtnl_lock() called with BH disabled. This fixes it by not acquiring then > dropping the bridge lock. > > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org> Applied, thanks Stephen. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bridge] Re: [PATCH] bridge: sysfs locking fix. 2007-08-14 13:50 ` [Bridge] [PATCH] bridge: sysfs locking fix Stephen Hemminger 2007-08-14 20:21 ` [Bridge] " David Miller @ 2007-08-24 8:18 ` Daniel Lezcano 1 sibling, 0 replies; 5+ messages in thread From: Daniel Lezcano @ 2007-08-24 8:18 UTC (permalink / raw) To: Stephen Hemminger Cc: netdev, Andrew Morton, David S. Miller, Lennert Buytenhek, bridge Stephen Hemminger wrote: > Forget earlier patch, it is wrong... > > The stp change code generates "sleeping function called from invalid context" > because rtnl_lock() called with BH disabled. This fixes it by not acquiring then > dropping the bridge lock. > > Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org> > > --- a/net/bridge/br_sysfs_br.c 2007-08-06 09:26:48.000000000 +0100 > +++ b/net/bridge/br_sysfs_br.c 2007-08-14 14:29:52.000000000 +0100 > @@ -147,20 +147,26 @@ static ssize_t show_stp_state(struct dev > return sprintf(buf, "%d\n", br->stp_enabled); > } > > -static void set_stp_state(struct net_bridge *br, unsigned long val) > -{ > - rtnl_lock(); > - spin_unlock_bh(&br->lock); > - br_stp_set_enabled(br, val); > - spin_lock_bh(&br->lock); > - rtnl_unlock(); > -} > > static ssize_t store_stp_state(struct device *d, > struct device_attribute *attr, const char *buf, > size_t len) > { > - return store_bridge_parm(d, buf, len, set_stp_state); > + struct net_bridge *br = to_bridge(d); > + char *endp; > + unsigned long val; > + > + if (!capable(CAP_NET_ADMIN)) > + return -EPERM; > + > + val = simple_strtoul(buf, &endp, 0); > + if (endp == buf) > + return -EINVAL; > + > + rtnl_lock(); > + br_stp_set_enabled(br, val); > + rtnl_unlock(); > + Shouldn't len value be returned at the end of the function ? > } > static DEVICE_ATTR(stp_state, S_IRUGO | S_IWUSR, show_stp_state, > store_stp_state); > - ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2007-08-24 8:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <46BCE380.60508@cluded.net>
[not found] ` <20070812012037.0195b610.akpm@linux-foundation.org>
2007-08-14 13:11 ` [Bridge] Re: BUG: when using 'brctl stp' Stephen Hemminger
2007-08-14 13:18 ` Lennert Buytenhek
2007-08-14 13:50 ` [Bridge] [PATCH] bridge: sysfs locking fix Stephen Hemminger
2007-08-14 20:21 ` [Bridge] " David Miller
2007-08-24 8:18 ` Daniel Lezcano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox