Hi Vitalli,
Thanks for the mstpd code, it looks good. We have started using the rstp/stp code and plan to use mstp in the future. We have identified some bugs, added support for some commands and have tested interoperability/conformance testing using IXIA ANVL. I have a list of changes below, please let me know which changes below you would like me to send patches for? I shall explain in detail about the fixes when I send out the patches.
Bug fixes:
When an operational UP root port is deleted from a bridge, port role selection for other ports is not done
When bridge down is done, the ports in the bridge are not brought down(mentioned in the discussion forum)
RSTP compliance IXIA ANVL fixes:
Set hello time transmitted to be equal to configured bridge hello time
The bridge and port identifier present in received BPDU is equal to the local port's values, discard the BPDU to identify possibly looped packets
If unknown port role is received in RSTP version, treat the RSTP BPDU as a STP Config BPDU instead and set the role to be designated role
Configuration commands:
Add support for setageing in bridge(current ticket in sourceforge) and bridge hello time
Additional functionality:
I have also added some functionaility which is not part of 802.1Q but is present in other vendors implementation which we would like to add. If you would like these changes too, please let me know.
BPDUGuard and Bridge assurance functionality
Thanks,
Satish
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hello Satish!
I'm glad to know that you've found mstpd useful, and I'm more than pleased to get feedback in the form of the patches. I'm happy to consider your changes, please don't hesitate to send them to me.
I have a slight doubt about IXIA ANVL compliance fix #2 though (the one about dropping looped BPDUs). Why do you think is it necessary? I hadn't implemented loop check because:
1) 802.1Q explicitly says that it is not necessary (see clause 14.4, NOTE 3 of the 802.1Q-2005);
2) mstpd puts the port in the Backup/Discarding state after receiving looped BPDU, even when forced to the "stp" protocol.
So, could you clarify why do you think this change is necessary? What bad behavior does it prevent?
Looking forward to you patches,
Vitalii
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Applied as revision [r39] with slight modifications (printf formatting, comments & range checking). Also added missed copy of the current Ageing_Time into the status struct when reading current bridge status.
Also, we need more work here. The Ageing_Time in the mstpd should be consistent with actual ageing time in the actual bridge device (Linux bridge and/or external bridge). I'm working on the patch and will send it for your revision soon.
Satish, please review the following patch and tell me if it breaks something for you. The patch translates Ageing time set in the mstpd to the Linux bridge and (possible) external bridging device.
It is based on revision [r39]
Index: mstp.c===================================================================--- mstp.c (revision 39)+++ mstp.c (working copy)@@ -278,6 +278,9 @@
*/
list_add_tail(&prt->br_list, &br->ports);
+ /* make ageing time consistent between mstpd and actual bridge device */+ MSTP_OUT_set_ageing_time(prt, br->Ageing_Time);+
prt_state_machines_begin(prt);
return true;
}
@@ -767,6 +770,14 @@
INFO_BRNAME(br, "bridge ageing_time new=%u, old=%u",
cfg->bridge_ageing_time, br->Ageing_Time);
assign(br->Ageing_Time, cfg->bridge_ageing_time);
+ /* make ageing time consistent between mstpd+ * and actual bridge device+ */+ FOREACH_PORT_IN_BRIDGE(prt, br)+ {+ if(0 == prt->rapidAgeingWhile)+ MSTP_OUT_set_ageing_time(prt, br->Ageing_Time);+ }
}
}
Hi Vitali,
Couple of thoughts regarding this:
1. Currently ageing_time in mstpd is really set in bridge driver only for STP version of the bridge, but with new changes, ageing_time will be set always, even for RSTP/MSTP version.
2. If admin configures ageing time in bridge driver using brctl etc, and does not configure the ageing time in mstpctl, mstpctl will end up overriding the ageing time incorrectly.
Thanks,
Satish
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Ok, I think you are right and it is better to not invent additional path of changing bridge parameters. So, I am not applying my patch. Instead, I described the current situation on the new wiki page: [ImplementationFeatures]
Thank you for sharing your consideration!
Issue:
When an operational UP root port is deleted from a bridge, port role selection for other ports is not done. This is because, delete_if() deletes the port from the bridge list before invoking MSTP_IN_delete_port(). Also, since reselect is not set, port role selection state machine does not do role selection for other ports in the bridge.
I don't think your approach is safe. As you mentioned, the prt is already deleted from the list, so your patch sets the "reselect" property of the already deleted port. It may work now, but can easily be broken in future.
I propose another solution: call MSTP_IN_delete_port() first, and only then (in the end of the function) delete port from the list. What would you say about the attached patch, does it fix the isuue?
Hi Vitalii,
I had explored this option and had tried this fix too, but this approach had a issue and the fix required adding another flag to indicate if port was already deleted.
Since bridge driver deletes the port from the bridge and then sends netlink message to mstpd and since mstpd does not have a flag to indicate if port was deleted, there are a couple of harmless errors in the logs in this case, when mstpd tries to flush fdb and set port_state, as seen below..
BTW, the functionality is correct with this fix, since port role selection goes through correctly. Due to this, I went with the above approach.
May 30 05:09:54 mstpd: error, MSTP_OUT_set_state: br1:swp3 Couldn't set kernel bridge state blocking
May 30 05:09:54 mstpd: Error in br_flush_port at bridge_track.c:431 verifying 0 <= fd. Couldn't open flush file /sys/class/net/swp3/brport/flush for write: No such file or directory
May 30 05:09:54 mstpd: error, MSTP_OUT_flush_all_fids: br1:swp3 Couldn't flush kernel bridge forwarding database
Thanks,
Satish
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I had explored this option and had tried this fix too, but this approach had a issue and the fix required adding another flag to indicate if port was already deleted.
Since bridge driver deletes the port from the bridge and then sends netlink message to mstpd and since mstpd does not have a flag to indicate if port was deleted, there are a couple of harmless errors in the logs in this case, when mstpd tries to flush fdb and set port_state, as seen below.
BTW, the functionality is correct with this fix, since port role selection goes through correctly. Due to this, I went with the above approach.
Although it works now, I still consider your approach as dangerous one, as it relies on the fact that ptp nodes of the deleted port are still accessible (they are not traversed when doing FOREACH_PORT_IN_BRIDGE/FOREACH_PTP_IN_PORT but they still can be accessed via FOREACH_TREE_IN_BRIDGE/FOREACH_PTP_IN_TREE). But internal implementation of the state machines can change in the future, for example, the order of the ptp traversal can change from per-tree to the per-port, and then suddenly your approach stops working.
So, as you mentioned, I'd rather prefer to improve my patch and implement additional flag ("deleted") which marks the port being deleted. Please, review the attached patch and tell me if it works for you.
Issue:
When "ifconfig br1 down" is done, the bridge ports are not brought down in mstpd and also the stp state needs to be set to disabled in this case.
Fix:
Add a check to see if bridge is not enabled when port is not enabled checks are present in the FSM
Issue:
When "ifconfig br1 down" is done, the bridge ports are not brought down in mstpd and also the stp state needs to be set to disabled in this case.
Fix:
Add a check to see if bridge is not enabled when port is not enabled checks are present in the FSM
I am not sure I understand what issue you are trying to resolve here. If bridge is down, bridgeEnabled is false, thus state machines are not run and mstpd does not send any BPDUs. So, please, could you explain in more details what exact issue do you have:
1) what is the sequence of commands;
2) what is the expected result;
3) what is the observed result, and why it is not satisfactory.
I guess, you want all ports to be in BR_STATE_DISABLED state when the bridge is down, don't you? Then we can achieve this goal immediately by directly setting port->portEnabled to false and calling MSTP_OUT_set_state(ptp, BR_STATE_DISABLED) in MSTP_IN_set_bridge_enable().
There is no BR_STATE_DISABLED in the state machines because 802.1Q does not define Disabled state for the MSTP - it is something external to the MSTP, the protocol knows only Blocking, Learning/Listening and Forwarding.
Also, I guess, setting portEnabled to false and calling MSTP_OUT_set_state(ptp, BR_STATE_DISABLED) isn't enough. Perhaps more cleaning should be done here, like setting all the internal states to the default values (see MSTP_IN_port_create_and_add_tail() and create_ptp()).
So, again, which problem are you facing? What erroneous behavior are you trying to improve?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Vitali,
Should have explained in detail earlier.
Issue:
The issue is that when bridge is brought down"ifconfig br1 down", since mstpd does not change the port state to disabled, the stp port state of the port is as was before bridge was down -i.e. - forwarding/blocking etc. Locally, mstpd will not send BPDU's, but the peer is sending BPDU's and MSTPD receives BPDU's every 2seconds and there is the below log every 2seconds. The only way MSTP will stop receiving BPDU's is when the port is in disabled state.
mstpd Received BPDU while bridge is disabled
Fix:
The fix was was to take action in mstpd when bridge is disabled and ensure that the ports are disabled in this case. I did not change the portEnabled to False instead, since that might make mstpd out of sync with kernel port state(also it might not be clear for admin in this case). It just seemed cleaner to look at bridge disabled state instead.
Thanks,
Satish
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hello Satish!
Ok, I see your intention: you want to put ports in disabled state when bridge is disabled. But there is more here, we should ensure that state machines start in consistent state when bridge is enabled again, so more clean-up must be done here. Also, putting port in disabled state is not in the 802.1Q FSM description, so the best place for this is outside the state machines; after all, it is logical to put it in clear and straightforward manner into the MSTP_IN_set_bridge_enable().
That said, here is the proposed patch. I believe it suites your needs (i.e. puts ports into the disabled state) and does all necessary clean up. Please tell me if it works for you.
Index: mstp.c===================================================================--- mstp.c (revision 41)+++ mstp.c (working copy)@@ -92,6 +92,64 @@
return MAX_PATH_COST;
}
+static void bridge_default_internal_vars(bridge_t *br)+{+ br->uptime = 0;+}++static void tree_default_internal_vars(tree_t *tree)+{+ /* 12.8.1.1.3.(b,c,d) */+ tree->time_since_topology_change = 0;+ tree->topology_change_count = 0;+ tree->topology_change = false; /* since all tcWhile are initialized to 0 */++ /* The following are initialized in BEGIN state:+ * - rootPortId, rootPriority, rootTimes: in Port Role Selection SM+ */+}++static void port_default_internal_vars(port_t *prt)+{+ prt->infoInternal = false;+ prt->rcvdInternal = false;+ prt->rcvdTcAck = false;+ prt->rcvdTcn = false;+ assign(prt->rapidAgeingWhile, 0u);++ /* The following are initialized in BEGIN state:+ * - mdelayWhile. mcheck, sendRSTP: in Port Protocol Migration SM+ * - helloWhen, newInfo, newInfoMsti, txCount: in Port Transmit SM+ * - edgeDelayWhile, rcvdBpdu, rcvdRSTP, rcvdSTP : in Port Receive SM+ * - operEdge: in Bridge Detection SM+ * - tcAck: in Topology Change SM+ */+}++static void ptp_default_internal_vars(per_tree_port_t *ptp)+{+ ptp->rcvdTc = false;+ ptp->tcProp = false;+ ptp->updtInfo = false;+ ptp->master = false; /* 13.24.5 */+ ptp->disputed = false;+ assign(ptp->rcvdInfo, (port_info_t)0);+ ptp->mastered = false;+ memset(&ptp->msgPriority, 0, sizeof(ptp->msgPriority));+ memset(&ptp->msgTimes, 0, sizeof(ptp->msgTimes));++ /* The following are initialized in BEGIN state:+ * - rcvdMsg: in Port Receive SM+ * - fdWhile, rrWhile, rbWhile, role, learn, forward,+ * sync, synced, reRoot: in Port Role Transitions SM+ * - tcWhile, fdbFlush: Topology Change SM+ * - rcvdInfoWhile, proposed, proposing, agree, agreed,+ * infoIs, reselect, selected: Port Information SM+ * - forwarding, learning: Port State Transition SM+ * - selectedRole, designatedPriority, designatedTimes: Port Role Selection SM+ */+}+
static tree_t * create_tree(bridge_t *br, __u8 *macaddr, __be16 MSTID)
{
/* Initialize all fields except anchor */
@@ -118,14 +176,8 @@
assign(tree->BridgeTimes.Message_Age, (__u8)0);
assign(tree->BridgeTimes.Hello_Time, br->Hello_Time);
- /* 12.8.1.1.3.(b,c,d) */- tree->time_since_topology_change = 0;- tree->topology_change_count = 0;- tree->topology_change = false; /* since all tcWhile are initialized to 0 */+ tree_default_internal_vars(tree);- /* The following are initialized in BEGIN state:- * - rootPortId, rootPriority, rootTimes: in Port Role Selection SM- */
return tree;
}
@@ -143,12 +195,8 @@
ptp->MSTID = tree->MSTID;
ptp->state = BR_STATE_DISABLED;
- ptp->rcvdTc = false;- ptp->tcProp = false;- ptp->updtInfo = false;
/* 0x80 = default port priority (17.14 of 802.1D) */
ptp->portId = __constant_cpu_to_be16(0x8000) | prt->port_number;
- ptp->master = false; /* 13.24.5 */
assign(ptp->AdminInternalPortPathCost, 0u);
assign(ptp->InternalPortPathCost, compute_pcost(GET_PORT_SPEED(prt)));
/* 802.1Q leaves portPriority and portTimes uninitialized */
@@ -157,24 +205,8 @@
ptp->calledFromFlushRoutine = false;
- /* The following are initialized in BEGIN state:- * - rcvdMsg: in Port Receive SM- * - fdWhile, rrWhile, rbWhile, role, learn, forward,- * sync, synced, reRoot: in Port Role Transitions SM- * - tcWhile, fdbFlush: Topology Change SM- * - rcvdInfoWhile, proposed, proposing, agree, agreed,- * infoIs, reselect, selected: Port Information SM- * - forwarding, learning: Port State Transition SM- * - selectedRole, designatedPriority, designatedTimes: Port Role Selection SM- */+ ptp_default_internal_vars(ptp);- /* The following are not initialized (set to zero thanks to calloc):- * - disputed- * - rcvdInfo- * - mastered- * - msgPriority- * - msgTimes- */
return ptp;
}
@@ -206,7 +238,7 @@
assign(br->Ageing_Time, 300u);/* 8.8.3 Table 8-3 */
assign(br->Hello_Time, (__u8)2); /* 17.14 of 802.1D */
- br->uptime = 0;+ bridge_default_internal_vars(br);
/* Create CIST */
if(!(cist = create_tree(br, macaddr, 0)))
@@ -234,25 +266,14 @@
prt->AdminP2P = p2pAuto;
prt->operPointToPointMAC = false;
prt->portEnabled = false;
- prt->infoInternal = false;- prt->rcvdInternal = false;- prt->rcvdTcAck = false;- prt->rcvdTcn = false;
prt->restrictedRole = false; /* 13.25.14 */
prt->restrictedTcn = false; /* 13.25.15 */
assign(prt->ExternalPortPathCost, MAX_PATH_COST); /* 13.37.1 */
prt->AdminEdgePort = false; /* 13.25 */
prt->AutoEdge = true; /* 13.25 */
- assign(prt->rapidAgeingWhile, 0u);
prt->deleted = false;
- /* The following are initialized in BEGIN state:- * - mdelayWhile. mcheck, sendRSTP: in Port Protocol Migration SM- * - helloWhen, newInfo, newInfoMsti, txCount: in Port Transmit SM- * - edgeDelayWhile, rcvdBpdu, rcvdRSTP, rcvdSTP : in Port Receive SM- * - operEdge: in Bridge Detection SM- * - tcAck: in Topology Change SM- */+ port_default_internal_vars(prt);
/* Create PerTreePort structures for all existing trees */
FOREACH_TREE_IN_BRIDGE(tree, br)
@@ -354,9 +375,41 @@
void MSTP_IN_set_bridge_enable(bridge_t *br, bool up)
{
+ port_t *prt;+ per_tree_port_t *ptp;+ tree_t *tree;+
if(br->bridgeEnabled == up)
return;
br->bridgeEnabled = up;
++ /* Reset all internal states and variables,+ * except those which are user-configurable */+ bridge_default_internal_vars(br);+ FOREACH_TREE_IN_BRIDGE(tree, br)+ {+ tree_default_internal_vars(tree);+ }+ FOREACH_PORT_IN_BRIDGE(prt, br)+ {+ /* NOTE: Don't check prt->deleted here, as it is imposible condition */+ /* NOTE: In the port_default_internal_vars() rapidAgeingWhile will be+ * reset, so we should stop rapid ageing procedure here.+ */+ if(prt->rapidAgeingWhile)+ {+ MSTP_OUT_set_ageing_time(prt, br->Ageing_Time);+ }+ port_default_internal_vars(prt);+ FOREACH_PTP_IN_PORT(ptp, prt)+ {+ if(BR_STATE_DISABLED != ptp->state)+ {+ MSTP_OUT_set_state(ptp, BR_STATE_DISABLED);+ }+ ptp_default_internal_vars(ptp);+ }+ }
br_state_machines_begin(br);
}
RSTP-1.2
Tue Apr 30 13:28:03 2013: TEST_DESCRIPTION
Quick test to verify that DUT transmits RST BPDU from each port.
The Unknown value of Port Role cannot be generated by a valid
implementation; however, this value is accepted on receipt.
(NOTE: The DUT will treat this RST-BPDU as a Configuration BPDU)
(Test to verify that the incoming RST BPDU with Unknown Port role
plays a role in the calculation of Active Spanning Tree)
Index: mstp.c===================================================================--- mstp.c (revision 37)+++ mstp.c (working copy)@@ -1588,6 +1588,25 @@ static port_info_t rcvInfo(per_tree_port_t *ptp)
case encodedRoleDesignated:
roleIsDesignated = true;
break;
+ case encodedRoleMaster:+ /* 802.1D-2004 S9.2.9 P61. The Unknown value of Port Role+ * cannot be generated by a valid implementation; however,+ * this value is accepted on receipt. roleMaster in MSTP is+ * roleUnknown in RSTP.+ * NOTE.If the Unknown value of the Port Role parameter is+ * received, the state machines will effectively treat the RST+ * BPDU as if it were a Configuration BPDU+ */+ if (protoRSTP == b->protocolVersion)+ {+ roleIsDesignated = true;+ break;+ }+ else+ {+ return OtherInfo;+ }+ break;
default:
return OtherInfo;
}
Issue:
The bridge and port identifier present in received BPDU is equal to the local port's values, discard the BPDU to identify possibly looped packets
Vitali, this is the loopback issue, for which you had a couple of questions. This is a negative testcase and this is specified in 802.1D as mentioned below. Just felt that processing every loopbacked packet would not be good and also it would be better to syslog this as an error, so that admin knows of the issue and takes immediate action.
Regarding clause 14.4, NOTE 3 of the 802.1Q-2005, not sure if it means that this validation was not specified in 802.1Q because it was already specified in 802.1D.
Sure, I think this is open to discussion..
Testcase 3.3 failure for RSTP suite
RSTP-3.3
Tue Apr 30 13:31:27 2013: TEST_DESCRIPTION
A Spanning Tree Protocol Entity shall process a received BPDU
if and only if the BPDU Type denotes a CONFIG BPDU and ..., and the
Bridge Identifier and Port Identifier parameters from the
received BPDU do not match the values that would be transmitted
in a BPDU from this Port
Tue Apr 30 13:31:27 2013: TEST_REFERENCE
NEGATIVE
IEEE Std 802.1D-2004 S9.3.4 P63 Validation of received BPDUs
From 802.1D SPEC
If the Bridge Identifier and Port Identifier both match the values that would be transmitted in a Configuration
BPDU, the BPDU is discarded to prevent processing of the Port’s own BPDUs; for example, if they are received by the
Port as a result of a loopback condition.
Failure:
! Did not receive expected RST BPDU from
! DUT Interface: 1
Index: mstp.c===================================================================--- mstp.c (revision 37)+++ mstp.c (working copy)@@ -474,6 +474,7 @@ void MSTP_IN_rx_bpdu(port_t *prt, bpdu_t *bpdu, in
{
int mstis_size;
bridge_t *br = prt->bridge;
+ per_tree_port_t *cist = GET_CIST_PTP_FROM_PORT(prt);
if(!br->bridgeEnabled)
{
@@ -487,6 +488,21 @@ void MSTP_IN_rx_bpdu(port_t *prt, bpdu_t *bpdu, in
return;
}
+ /* If the Bridge Identifier and Port Identifier both match the values that+ * would be transmitted in a Configuration BPDU, the BPDU is discarded to+ * prevent processing of the Port's own BPDUs; for example, if they are+ * received by the Port as a result of a loopback condition.+ * 802.1D Spec D9.3.4 Page63+ */+ if ((0 == _ncmp(cist->designatedPriority.RRootID, bpdu->cistRRootID))+ && (0 == _ncmp(cist->designatedPriority.DesignatedPortID,+ bpdu->cistPortID))) {+ ERROR_PRTNAME(br, prt, "rcvd local port_id - "PRT_ID_FMT", "\+ "bridge_id "BR_ID_FMT"", PRT_ID_ARGS(bpdu->cistPortID),+ BR_ID_ARGS(bpdu->cistRRootID));+ return;+ }+
/* 14.4 Validation */
if((TCN_BPDU_SIZE > size) || (0 != bpdu->protocolIdentifier))
{
Index: ctl_main.c===================================================================--- ctl_main.c (revision 37)+++ ctl_main.c (working copy)@@ -71,18 +71,6 @@ static inline int get_id(const char *str, const ch
return id;
}
-#define GET_NUM_FROM_PRIO(p) (__be16_to_cpu(p) & 0x0FFF)--#define BR_ID_FMT "%01hhX.%03hX.%02hhX:%02hhX:%02hhX:%02hhX:%02hhX:%02hhX"-#define BR_ID_ARGS(x) ((GET_PRIORITY_FROM_IDENTIFIER(x) >> 4) & 0x0F), \- GET_NUM_FROM_PRIO((x).s.priority), \- x.s.mac_address[0], x.s.mac_address[1], x.s.mac_address[2], \- x.s.mac_address[3], x.s.mac_address[4], x.s.mac_address[5]--#define PRT_ID_FMT "%01hhX.%03hX"-#define PRT_ID_ARGS(x) ((GET_PRIORITY_FROM_IDENTIFIER(x) >> 4) & 0x0F), \- GET_NUM_FROM_PRIO(x)-
#define BOOL_STR(x) ((x) ? "yes" : "no")
#define PROTO_VERS_STR(x) ((protoRSTP == (x)) ? "rstp" : \
((protoMSTP <= (x)) ? "mstp" : "stp"))
Index: mstp.h===================================================================--- mstp.h (revision 37)+++ mstp.h (working copy)@@ -81,6 +81,18 @@ typedef __be16 port_identifier_t;
*first_octet |= (pri) & 0xF0; \
}while(0)
+#define GET_NUM_FROM_PRIO(p) (__be16_to_cpu(p) & 0x0FFF)++#define BR_ID_FMT "%01hhX.%03hX.%02hhX:%02hhX:%02hhX:%02hhX:%02hhX:%02hhX"+#define BR_ID_ARGS(x) ((GET_PRIORITY_FROM_IDENTIFIER(x) >> 4) & 0x0F), \+ GET_NUM_FROM_PRIO((x).s.priority), \+ x.s.mac_address[0], x.s.mac_address[1], x.s.mac_address[2], \+ x.s.mac_address[3], x.s.mac_address[4], x.s.mac_address[5]++#define PRT_ID_FMT "%01hhX.%03hX"+#define PRT_ID_ARGS(x) ((GET_PRIORITY_FROM_IDENTIFIER(x) >> 4) & 0x0F), \+ GET_NUM_FROM_PRIO(x)+
#define CONFIGURATION_NAME_LEN 32
#define CONFIGURATION_DIGEST_LEN 16
typedef union
Issue:
The bridge and port identifier present in received BPDU is equal to the local port's values, discard the BPDU to identify possibly looped packets
Vitali, this is the loopback issue, for which you had a couple of questions. This is a negative testcase and this is specified in 802.1D as mentioned below. Just felt that processing every loopbacked packet would not be good and also it would be better to syslog this as an error, so that admin knows of the issue and takes immediate action.
After a lot of consideration I decided to NACK this patch. I wrote down the reasons in the wiki. Here is the citation:
Mstpd will not drop looped-back BPDUs, and there are several good reasons for it:
a) 802.1Q explicitly says that loopback check is not necessary (see clause 14.4, NOTE 3 of the 802.1Q-2005);
b) mstpd puts the port in the Backup/Discarding state after receiving looped BPDU, even when forced to the "stp" protocol, so there is no problem - mstpd takes reasonable action in this case;
c) if looped-back BPDUs were dropped, the port would remain in Forwarding state and bad things would happen, e.g. broadcast storms and undesired MAC learning on this port.
There are reports that such behavior breaks 802.1D compliance. That is expected - after all, mstpd is compliant with 802.1Q, not 802.1D.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Vitalli,
Thanks for the mstpd code, it looks good. We have started using the rstp/stp code and plan to use mstp in the future. We have identified some bugs, added support for some commands and have tested interoperability/conformance testing using IXIA ANVL. I have a list of changes below, please let me know which changes below you would like me to send patches for? I shall explain in detail about the fixes when I send out the patches.
Bug fixes:
RSTP compliance IXIA ANVL fixes:
Configuration commands:
Additional functionality:
I have also added some functionaility which is not part of 802.1Q but is present in other vendors implementation which we would like to add. If you would like these changes too, please let me know.
Thanks,
Satish
Hello Satish!
I'm glad to know that you've found mstpd useful, and I'm more than pleased to get feedback in the form of the patches. I'm happy to consider your changes, please don't hesitate to send them to me.
I have a slight doubt about IXIA ANVL compliance fix #2 though (the one about dropping looped BPDUs). Why do you think is it necessary? I hadn't implemented loop check because:
1) 802.1Q explicitly says that it is not necessary (see clause 14.4, NOTE 3 of the 802.1Q-2005);
2) mstpd puts the port in the Backup/Discarding state after receiving looped BPDU, even when forced to the "stp" protocol.
So, could you clarify why do you think this change is necessary? What bad behavior does it prevent?
Looking forward to you patches,
Vitalii
Configuration for bridge hello time patch
Last edit: Satish Ashok 2013-05-28
Applied as revision [r38] with slight modifications (printf formatting, comments & range checking).
Thanks, Satish!
Related
Commit: [r38]
Configuration for bridge set ageing patch
Last edit: Satish Ashok 2013-05-29
Applied as revision [r39] with slight modifications (printf formatting, comments & range checking). Also added missed copy of the current Ageing_Time into the status struct when reading current bridge status.
Also, we need more work here. The Ageing_Time in the mstpd should be consistent with actual ageing time in the actual bridge device (Linux bridge and/or external bridge). I'm working on the patch and will send it for your revision soon.
Related
Commit: [r39]
Satish, please review the following patch and tell me if it breaks something for you. The patch translates Ageing time set in the mstpd to the Linux bridge and (possible) external bridging device.
It is based on revision [r39]
Related
Commit: [r39]
Last edit: Vitalii Demianets 2013-05-29
Hi Vitali,
Couple of thoughts regarding this:
1. Currently ageing_time in mstpd is really set in bridge driver only for STP version of the bridge, but with new changes, ageing_time will be set always, even for RSTP/MSTP version.
2. If admin configures ageing time in bridge driver using brctl etc, and does not configure the ageing time in mstpctl, mstpctl will end up overriding the ageing time incorrectly.
Thanks,
Satish
Ok, I think you are right and it is better to not invent additional path of changing bridge parameters. So, I am not applying my patch. Instead, I described the current situation on the new wiki page: [ImplementationFeatures]
Thank you for sharing your consideration!
Related
Wiki: ImplementationFeatures
Issue:
When an operational UP root port is deleted from a bridge, port role selection for other ports is not done. This is because, delete_if() deletes the port from the bridge list before invoking MSTP_IN_delete_port(). Also, since reselect is not set, port role selection state machine does not do role selection for other ports in the bridge.
Bugfix:
Set reselect to true
Last edit: Satish Ashok 2013-05-28
I don't think your approach is safe. As you mentioned, the prt is already deleted from the list, so your patch sets the "reselect" property of the already deleted port. It may work now, but can easily be broken in future.
I propose another solution: call MSTP_IN_delete_port() first, and only then (in the end of the function) delete port from the list. What would you say about the attached patch, does it fix the isuue?
Hi Vitalii,
I had explored this option and had tried this fix too, but this approach had a issue and the fix required adding another flag to indicate if port was already deleted.
Since bridge driver deletes the port from the bridge and then sends netlink message to mstpd and since mstpd does not have a flag to indicate if port was deleted, there are a couple of harmless errors in the logs in this case, when mstpd tries to flush fdb and set port_state, as seen below..
BTW, the functionality is correct with this fix, since port role selection goes through correctly. Due to this, I went with the above approach.
May 30 05:09:54 mstpd: error, MSTP_OUT_set_state: br1:swp3 Couldn't set kernel bridge state blocking
May 30 05:09:54 mstpd: Error in br_flush_port at bridge_track.c:431 verifying 0 <= fd. Couldn't open flush file /sys/class/net/swp3/brport/flush for write: No such file or directory
May 30 05:09:54 mstpd: error, MSTP_OUT_flush_all_fids: br1:swp3 Couldn't flush kernel bridge forwarding database
Thanks,
Satish
Hi Satish!
Although it works now, I still consider your approach as dangerous one, as it relies on the fact that ptp nodes of the deleted port are still accessible (they are not traversed when doing FOREACH_PORT_IN_BRIDGE/FOREACH_PTP_IN_PORT but they still can be accessed via FOREACH_TREE_IN_BRIDGE/FOREACH_PTP_IN_TREE). But internal implementation of the state machines can change in the future, for example, the order of the ptp traversal can change from per-tree to the per-port, and then suddenly your approach stops working.
So, as you mentioned, I'd rather prefer to improve my patch and implement additional flag ("deleted") which marks the port being deleted. Please, review the attached patch and tell me if it works for you.
Thanks for the review!
Vitalii
Hi Vitalli,
I verified that this patch works without any issues. Looks good..
Thanks!!
Satish
Applied as revision [r41].
Satish, thank you for reporting the issue, reviewing and testing!
Related
Commit: [r41]
Issue:
When "ifconfig br1 down" is done, the bridge ports are not brought down in mstpd and also the stp state needs to be set to disabled in this case.
Fix:
Add a check to see if bridge is not enabled when port is not enabled checks are present in the FSM
I have added a patch file as attachment too..
Hello Satish!
I am not sure I understand what issue you are trying to resolve here. If bridge is down, bridgeEnabled is false, thus state machines are not run and mstpd does not send any BPDUs. So, please, could you explain in more details what exact issue do you have:
1) what is the sequence of commands;
2) what is the expected result;
3) what is the observed result, and why it is not satisfactory.
I guess, you want all ports to be in BR_STATE_DISABLED state when the bridge is down, don't you? Then we can achieve this goal immediately by directly setting port->portEnabled to false and calling MSTP_OUT_set_state(ptp, BR_STATE_DISABLED) in MSTP_IN_set_bridge_enable().
There is no BR_STATE_DISABLED in the state machines because 802.1Q does not define Disabled state for the MSTP - it is something external to the MSTP, the protocol knows only Blocking, Learning/Listening and Forwarding.
Also, I guess, setting portEnabled to false and calling MSTP_OUT_set_state(ptp, BR_STATE_DISABLED) isn't enough. Perhaps more cleaning should be done here, like setting all the internal states to the default values (see MSTP_IN_port_create_and_add_tail() and create_ptp()).
So, again, which problem are you facing? What erroneous behavior are you trying to improve?
Hi Vitali,
Should have explained in detail earlier.
Issue:
The issue is that when bridge is brought down"ifconfig br1 down", since mstpd does not change the port state to disabled, the stp port state of the port is as was before bridge was down -i.e. - forwarding/blocking etc. Locally, mstpd will not send BPDU's, but the peer is sending BPDU's and MSTPD receives BPDU's every 2seconds and there is the below log every 2seconds. The only way MSTP will stop receiving BPDU's is when the port is in disabled state.
mstpd Received BPDU while bridge is disabled
Fix:
The fix was was to take action in mstpd when bridge is disabled and ensure that the ports are disabled in this case. I did not change the portEnabled to False instead, since that might make mstpd out of sync with kernel port state(also it might not be clear for admin in this case). It just seemed cleaner to look at bridge disabled state instead.
Thanks,
Satish
Hello Satish!
Ok, I see your intention: you want to put ports in disabled state when bridge is disabled. But there is more here, we should ensure that state machines start in consistent state when bridge is enabled again, so more clean-up must be done here. Also, putting port in disabled state is not in the 802.1Q FSM description, so the best place for this is outside the state machines; after all, it is logical to put it in clear and straightforward manner into the MSTP_IN_set_bridge_enable().
That said, here is the proposed patch. I believe it suites your needs (i.e. puts ports into the disabled state) and does all necessary clean up. Please tell me if it works for you.
Hi Vitali,
I verified that the bridge down patch which you have above works fine..
Thanks,
Satish
Applied as revision [r48]. Thanks a lot!
Related
Commit: [r48]
Ixia RSTP ANVL Failure:
Tue Apr 30 13:28:03 2013: TEST_REFERENCE
NEGATIVE
IEEE Std 802.1D-2004 S9.2.9 Page 61
Verification:
Failure:
! Received RST BPDU doesn't contain expected
! Root Indentifier : 0 / 02:02:00:00:00:01Tue Apr 30 13:28:13 2013:
Applied as revision [r40]. Thanks for fixing that!
Related
Commit: [r40]
Issue:
The bridge and port identifier present in received BPDU is equal to the local port's values, discard the BPDU to identify possibly looped packets
Vitali, this is the loopback issue, for which you had a couple of questions. This is a negative testcase and this is specified in 802.1D as mentioned below. Just felt that processing every loopbacked packet would not be good and also it would be better to syslog this as an error, so that admin knows of the issue and takes immediate action.
Regarding clause 14.4, NOTE 3 of the 802.1Q-2005, not sure if it means that this validation was not specified in 802.1Q because it was already specified in 802.1D.
Sure, I think this is open to discussion..
Testcase 3.3 failure for RSTP suite
Tue Apr 30 13:31:27 2013: TEST_REFERENCE
NEGATIVE
IEEE Std 802.1D-2004 S9.3.4 P63 Validation of received BPDUs
From 802.1D SPEC
If the Bridge Identifier and Port Identifier both match the values that would be transmitted in a Configuration
BPDU, the BPDU is discarded to prevent processing of the Port’s own BPDUs; for example, if they are received by the
Port as a result of a loopback condition.
Failure:
! Did not receive expected RST BPDU from
! DUT Interface: 1
Last edit: Satish Ashok 2013-05-29
Satish!
After a lot of consideration I decided to NACK this patch. I wrote down the reasons in the wiki. Here is the citation:
Mstpd will not drop looped-back BPDUs, and there are several good reasons for it:
a) 802.1Q explicitly says that loopback check is not necessary (see clause 14.4, NOTE 3 of the 802.1Q-2005);
b) mstpd puts the port in the Backup/Discarding state after receiving looped BPDU, even when forced to the "stp" protocol, so there is no problem - mstpd takes reasonable action in this case;
c) if looped-back BPDUs were dropped, the port would remain in Forwarding state and bad things would happen, e.g. broadcast storms and undesired MAC learning on this port.
There are reports that such behavior breaks 802.1D compliance. That is expected - after all, mstpd is compliant with 802.1Q, not 802.1D.