Patchwork bridge: push blocking slaves to forwarding when turning stp off

login
register
mail settings
Submitter Vitalii Demianets
Date Dec. 13, 2011, 9:36 a.m.
Message ID <201112131136.25919.vitas@nppfactor.kiev.ua>
Download mbox | patch
Permalink /patch/131037/
State Deferred
Delegated to: David Miller
Headers show

Comments

Vitalii Demianets - Dec. 13, 2011, 9:36 a.m.
If there is a slave in blocking state when stp is turned off, that slave will 
remain in blocking state for indefinitely long time until interface state 
changed. We should push all blocking slaves into forwarding state after 
turning stp off.

Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>

---
 net/bridge/br_stp.c    |    5 ++++-
 net/bridge/br_stp_if.c |   10 +++++-----
 2 files changed, 9 insertions(+), 6 deletions(-)
stephen hemminger - Dec. 14, 2011, 12:16 a.m.
On Tue, 13 Dec 2011 11:36:25 +0200
Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:

> If there is a slave in blocking state when stp is turned off, that slave will 
> remain in blocking state for indefinitely long time until interface state 
> changed. We should push all blocking slaves into forwarding state after 
> turning stp off.
> 
> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>

Maybe. But if the port was in the blocking state then STP must have
decided there was a loop in the network if that port was used.
Therefore blindly putting the port into forwarding state could cause
disastrous network flood.


The user can force the port back out of blocking state (via sysfs).

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vitalii Demianets - Dec. 14, 2011, 9:32 a.m.
On Wednesday 14 December 2011 02:16:13 Stephen Hemminger wrote:
> On Tue, 13 Dec 2011 11:36:25 +0200
>
> Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:
> > If there is a slave in blocking state when stp is turned off, that slave
> > will remain in blocking state for indefinitely long time until interface
> > state changed. We should push all blocking slaves into forwarding state
> > after turning stp off.
> >
> > Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
>
> Maybe. But if the port was in the blocking state then STP must have
> decided there was a loop in the network if that port was used.
> Therefore blindly putting the port into forwarding state could cause
> disastrous network flood.
>
>
> The user can force the port back out of blocking state (via sysfs).
>

1) That blocking state in the absence of STP is not stable. It will eventually 
flip to forwarding sooner or later on the first call of 
br_port_state_selection(). For example, when user changes MAC address on 
another slave. Or even worse - when any other slave of the bridge changes its 
carrier state. Don't think user wants such unpredictable state changes.

2) There is also another drawback of not pushing ports into forwarding state 
after turning off USER_STP mode. Possible scenario is:
  a) bridge in USER_STP mode, all ports are in non-forwarding state (blocking, 
learning)
  b) user turns off STP. Without the patch ports are not advanced to the 
forwarding state and are left in the states they are (the timers do not work 
because of USER_STP mode)
  c) The bridge stays in no-carrier state until something happens (carrier 
state transition on one of the slaves, MAC address change etc)

You can say again that in the above two cases user can manually set the state 
of the slaves to forwarding.  But to account all possible cases one should 
always unconditionally do it for all the slaves each time when stp is being 
turned off. So why not to automate the task?

3) The initial intention of the code in br_stp_stop() was to get ports out of 
blocking state when stp is being turned off. It fails to achieve the goal, 
and patch just fixes it.

4) If user turns stp off he clearly indicates that she wants all ports to work 
in stateless mode and that he will deal with possible network loops on 
himself. Should we in that case guess network topology basing on loose 
assumptions and leave ports in unstable blocking state (and they will flip 
eventually to the forwarding state in unpredictable times as mentioned 
above)?
Vitalii Demianets - Dec. 20, 2011, 10:59 a.m.
Hello, Stephen!
I can not understand your silence.
There are issues fixed by the patch in question. For example, if the interface 
is left in blocking state after stp was turned off, that state is not 
stable - it can flip to forwarding state in unpredictable times, e.g. when 
_any other_ slave of the bridge goes up or down. Do you think user wants 
exactly that unpredictable state change?
Also, the code in question in function br_stp_stop(), namely 
br_port_state_selection(br) call, does exactly nothing except wasting cpu 
cycles. Isn't it worth fixing?
stephen hemminger - Dec. 20, 2011, 6:11 p.m.
On Tue, 20 Dec 2011 12:59:11 +0200
Vitalii Demianets <vitas@nppfactor.kiev.ua> wrote:

> Hello, Stephen!
> I can not understand your silence.
> There are issues fixed by the patch in question. For example, if the interface 
> is left in blocking state after stp was turned off, that state is not 
> stable - it can flip to forwarding state in unpredictable times, e.g. when 
> _any other_ slave of the bridge goes up or down. Do you think user wants 
> exactly that unpredictable state change?
> Also, the code in question in function br_stp_stop(), namely 
> br_port_state_selection(br) call, does exactly nothing except wasting cpu 
> cycles. Isn't it worth fixing?
> 

I had to go do real work last week.
Let me test and look at it more detail.
There is no urgency, the problem has existed for many years.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vitalii Demianets - Dec. 20, 2011, 6:27 p.m.
On Tuesday 20 December 2011 20:11:17 Stephen Hemminger wrote:
> I had to go do real work last week.
> Let me test and look at it more detail.
> There is no urgency, the problem has existed for many years.

Please excuse me. It was only my impatience and "deferred" status on the 
David's patchwork site that worried me.
I agree that problem is not urgent.

Patch

diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index dd147d7..aed7e21 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -415,7 +415,10 @@  void br_port_state_selection(struct net_bridge *br)
 			} else {
 				p->config_pending = 0;
 				p->topology_change_ack = 0;
-				br_make_blocking(p);
+				if(br->stp_enabled == BR_NO_STP)
+					br_make_forwarding(p);
+				else
+					br_make_blocking(p);
 			}
 		}
 
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 19308e3..38d8dd7 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -153,14 +153,14 @@  static void br_stp_stop(struct net_bridge *br)
 	if (br->stp_enabled == BR_USER_STP) {
 		r = call_usermodehelper(BR_STP_PROG, argv, envp, UMH_WAIT_PROC);
 		br_info(br, "userspace STP stopped, return code %d\n", r);
-
-		/* To start timers on any ports left in blocking */
-		spin_lock_bh(&br->lock);
-		br_port_state_selection(br);
-		spin_unlock_bh(&br->lock);
 	}
 
 	br->stp_enabled = BR_NO_STP;
+
+	/* To push in forwarding state any ports left in blocking */
+	spin_lock_bh(&br->lock);
+	br_port_state_selection(br);
+	spin_unlock_bh(&br->lock);
 }
 
 void br_stp_set_enabled(struct net_bridge *br, unsigned long val)