Ivan Vecera
2017-May-19 16:25 UTC
[Bridge] [PATCH net] bridge: fix hello and hold timers starting/stopping
Current bridge code incorrectly handles starting/stopping of hello and
hold timers during STP enable/disable.
1. Timers are stopped in br_stp_start() during NO_STP->USER_STP
transition. This is not correct as the timers are stopped in NO_STP
case.
2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition.
This is not also correct as the timers should be stopped in NO_STP
state.
3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP
transition. They should be stopped as they are running in KERNEL_STP
state and should not run in NO_STP case.
The patch is a follow-up for "bridge: start hello_timer when enabling
KERNEL_STP in br_stp_start" patch from Xin Long.
Cc: davem at davemloft.net
Cc: sashok at cumulusnetworks.com
Cc: stephen at networkplumber.org
Cc: bridge at lists.linux-foundation.org
Cc: lucien.xin at gmail.com
Cc: nikolay at cumulusnetworks.com
Signed-off-by: Ivan Vecera <cera at cera.cz>
---
net/bridge/br_stp_if.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 0db8102995a5..f137ebf27755 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -150,7 +150,6 @@ static int br_stp_call_user(struct net_bridge *br, char
*arg)
static void br_stp_start(struct net_bridge *br)
{
- struct net_bridge_port *p;
int err = -ENOENT;
if (net_eq(dev_net(br->dev), &init_net))
@@ -169,11 +168,6 @@ static void br_stp_start(struct net_bridge *br)
if (!err) {
br->stp_enabled = BR_USER_STP;
br_debug(br, "userspace STP started\n");
-
- /* Stop hello and hold timers */
- del_timer(&br->hello_timer);
- list_for_each_entry(p, &br->port_list, list)
- del_timer(&p->hold_timer);
} else {
br->stp_enabled = BR_KERNEL_STP;
br_debug(br, "using kernel STP\n");
@@ -197,13 +191,14 @@ static void br_stp_stop(struct net_bridge *br)
br_err(br, "failed to stop userspace STP (%d)\n", err);
/* To start timers on any ports left in blocking */
- mod_timer(&br->hello_timer, jiffies + br->hello_time);
- list_for_each_entry(p, &br->port_list, list)
- mod_timer(&p->hold_timer,
- round_jiffies(jiffies + BR_HOLD_TIME));
spin_lock_bh(&br->lock);
br_port_state_selection(br);
spin_unlock_bh(&br->lock);
+ } else {
+ /* BR_KERNEL_STP - stop hello and hold timers */
+ del_timer(&br->hello_timer);
+ list_for_each_entry(p, &br->port_list, list)
+ del_timer(&p->hold_timer);
}
br->stp_enabled = BR_NO_STP;
--
2.13.0
Stephen Hemminger
2017-May-19 16:38 UTC
[Bridge] [PATCH net] bridge: fix hello and hold timers starting/stopping
On Fri, 19 May 2017 18:25:43 +0200 Ivan Vecera <cera at cera.cz> wrote:> Current bridge code incorrectly handles starting/stopping of hello and > hold timers during STP enable/disable. > > 1. Timers are stopped in br_stp_start() during NO_STP->USER_STP > transition. This is not correct as the timers are stopped in NO_STP > case. > > 2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition. > This is not also correct as the timers should be stopped in NO_STP > state. > > 3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP > transition. They should be stopped as they are running in KERNEL_STP > state and should not run in NO_STP case. > > The patch is a follow-up for "bridge: start hello_timer when enabling > KERNEL_STP in br_stp_start" patch from Xin Long. > > Cc: davem at davemloft.net > Cc: sashok at cumulusnetworks.com > Cc: stephen at networkplumber.org > Cc: bridge at lists.linux-foundation.org > Cc: lucien.xin at gmail.com > Cc: nikolay at cumulusnetworks.com > Signed-off-by: Ivan Vecera <cera at cera.cz>Overall, this looks correct but the wording of commit message is too terse. It would be better to add a more complete description of the impact of this from a user's point of view. I am concerned that this might have other side effects. For example, what is the sequence of commands to validated this. What is the impact, should this go to stable?
Xin Long
2017-May-19 16:51 UTC
[Bridge] [PATCH net] bridge: fix hello and hold timers starting/stopping
On Sat, May 20, 2017 at 12:25 AM, Ivan Vecera <cera at cera.cz> wrote:> Current bridge code incorrectly handles starting/stopping of hello and > hold timers during STP enable/disable. > > 1. Timers are stopped in br_stp_start() during NO_STP->USER_STP > transition. This is not correct as the timers are stopped in NO_STP > case. > > 2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition. > This is not also correct as the timers should be stopped in NO_STP > state. > > 3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP > transition. They should be stopped as they are running in KERNEL_STP > state and should not run in NO_STP case. > > The patch is a follow-up for "bridge: start hello_timer when enabling > KERNEL_STP in br_stp_start" patch from Xin Long. > > Cc: davem at davemloft.net > Cc: sashok at cumulusnetworks.com > Cc: stephen at networkplumber.org > Cc: bridge at lists.linux-foundation.org > Cc: lucien.xin at gmail.com > Cc: nikolay at cumulusnetworks.com > Signed-off-by: Ivan Vecera <cera at cera.cz> > --- > net/bridge/br_stp_if.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c > index 0db8102995a5..f137ebf27755 100644 > --- a/net/bridge/br_stp_if.c > +++ b/net/bridge/br_stp_if.c > @@ -150,7 +150,6 @@ static int br_stp_call_user(struct net_bridge *br, char *arg) > > static void br_stp_start(struct net_bridge *br) > { > - struct net_bridge_port *p; > int err = -ENOENT; > > if (net_eq(dev_net(br->dev), &init_net)) > @@ -169,11 +168,6 @@ static void br_stp_start(struct net_bridge *br) > if (!err) { > br->stp_enabled = BR_USER_STP; > br_debug(br, "userspace STP started\n"); > - > - /* Stop hello and hold timers */ > - del_timer(&br->hello_timer); > - list_for_each_entry(p, &br->port_list, list) > - del_timer(&p->hold_timer); > } else { > br->stp_enabled = BR_KERNEL_STP; > br_debug(br, "using kernel STP\n"); > @@ -197,13 +191,14 @@ static void br_stp_stop(struct net_bridge *br) > br_err(br, "failed to stop userspace STP (%d)\n", err); > > /* To start timers on any ports left in blocking */ > - mod_timer(&br->hello_timer, jiffies + br->hello_time); > - list_for_each_entry(p, &br->port_list, list) > - mod_timer(&p->hold_timer, > - round_jiffies(jiffies + BR_HOLD_TIME)); > spin_lock_bh(&br->lock); > br_port_state_selection(br); > spin_unlock_bh(&br->lock); > + } else { > + /* BR_KERNEL_STP - stop hello and hold timers */ > + del_timer(&br->hello_timer); > + list_for_each_entry(p, &br->port_list, list) > + del_timer(&p->hold_timer);I'm thinking, what if the timers are running when deleting them ? del_timer may not be going to delete it, and still have to stop itself next time when br->stp_enabled = BR_NO_STP. So do you think it's better to do nothing here and just leave it to be stopped by itself when checking br->stp_enabled in br_hello_timer_expired ?> } > > br->stp_enabled = BR_NO_STP; > -- > 2.13.0 >
Nikolay Aleksandrov
2017-May-19 16:55 UTC
[Bridge] [PATCH net] bridge: fix hello and hold timers starting/stopping
On 5/19/17 7:25 PM, Ivan Vecera wrote:> Current bridge code incorrectly handles starting/stopping of hello and > hold timers during STP enable/disable. > > 1. Timers are stopped in br_stp_start() during NO_STP->USER_STP > transition. This is not correct as the timers are stopped in NO_STP > case.This really is a noop, but ok.> > 2. Timers are started in br_stp_stop() during USER_STP->NO_STP transition. > This is not also correct as the timers should be stopped in NO_STP > state.Indeed, but the actual end result is almost as them being stopped because in the timers there are specific checks if the STP == KERNEL_STP (see br_transmit_config()) and the hold_timers will simply expire and not rearm in any other mode. The only real problem is the hello_timer which continues to rearm itself, but with Xin's earlier patch that is taken care of too.> > 3. Timers are NOT stopped in br_stp_stop() during KERNEL_STP->NO_STP > transition. They should be stopped as they are running in KERNEL_STP > state and should not run in NO_STP case.Same comment as for point 2.> > The patch is a follow-up for "bridge: start hello_timer when enabling > KERNEL_STP in br_stp_start" patch from Xin Long. >I'd say this is more of a cleanup/improvement after Xin's patch and thus would suggest targeting net-next. The only real issue is fixed by his patch.> Cc: davem at davemloft.net > Cc: sashok at cumulusnetworks.com > Cc: stephen at networkplumber.org > Cc: bridge at lists.linux-foundation.org > Cc: lucien.xin at gmail.com > Cc: nikolay at cumulusnetworks.com > Signed-off-by: Ivan Vecera <cera at cera.cz> > --- > net/bridge/br_stp_if.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c > index 0db8102995a5..f137ebf27755 100644 > --- a/net/bridge/br_stp_if.c > +++ b/net/bridge/br_stp_if.c > @@ -150,7 +150,6 @@ static int br_stp_call_user(struct net_bridge *br, char *arg) > > static void br_stp_start(struct net_bridge *br) > { > - struct net_bridge_port *p; > int err = -ENOENT; > > if (net_eq(dev_net(br->dev), &init_net)) > @@ -169,11 +168,6 @@ static void br_stp_start(struct net_bridge *br) > if (!err) { > br->stp_enabled = BR_USER_STP; > br_debug(br, "userspace STP started\n"); > - > - /* Stop hello and hold timers */ > - del_timer(&br->hello_timer); > - list_for_each_entry(p, &br->port_list, list) > - del_timer(&p->hold_timer); > } else { > br->stp_enabled = BR_KERNEL_STP; > br_debug(br, "using kernel STP\n"); > @@ -197,13 +191,14 @@ static void br_stp_stop(struct net_bridge *br) > br_err(br, "failed to stop userspace STP (%d)\n", err); > > /* To start timers on any ports left in blocking */ > - mod_timer(&br->hello_timer, jiffies + br->hello_time); > - list_for_each_entry(p, &br->port_list, list) > - mod_timer(&p->hold_timer, > - round_jiffies(jiffies + BR_HOLD_TIME)); > spin_lock_bh(&br->lock); > br_port_state_selection(br); > spin_unlock_bh(&br->lock); > + } else { > + /* BR_KERNEL_STP - stop hello and hold timers */ > + del_timer(&br->hello_timer); > + list_for_each_entry(p, &br->port_list, list) > + del_timer(&p->hold_timer); > } > > br->stp_enabled = BR_NO_STP; >
Xin Long
2017-May-19 17:26 UTC
[Bridge] [PATCH net] bridge: fix hello and hold timers starting/stopping
On Sat, May 20, 2017 at 12:25 AM, Ivan Vecera <cera at cera.cz> wrote: [...]> @@ -197,13 +191,14 @@ static void br_stp_stop(struct net_bridge *br) > br_err(br, "failed to stop userspace STP (%d)\n", err); > > /* To start timers on any ports left in blocking */ > - mod_timer(&br->hello_timer, jiffies + br->hello_time); > - list_for_each_entry(p, &br->port_list, list) > - mod_timer(&p->hold_timer, > - round_jiffies(jiffies + BR_HOLD_TIME)); > spin_lock_bh(&br->lock); > br_port_state_selection(br); > spin_unlock_bh(&br->lock); > + } else { > + /* BR_KERNEL_STP - stop hello and hold timers */ > + del_timer(&br->hello_timer); > + list_for_each_entry(p, &br->port_list, list) > + del_timer(&p->hold_timer); > } > > br->stp_enabled = BR_NO_STP;I have a question here, br->stp_enabled is not atomic, and it is being changed without holding br->lock here, while it may be checked in br_hello_timer_expired, is it safe ? (sorry if I misunderstood or overthought about it)> -- > 2.13.0 >