The cited commit removed direct call to RSTP module from a callback,
but we can still enter the module after going through a patch port
to a different bridge via ofproto_dpif_send_packet().
Partially revert the change going back to a recursive mutex.
Adding the same test for both RSTP and STP. While STP unit tests
do catch the same problem for STP (if STP mutex changed to be
non-recursive), they are not actually using the same callback function
as ovs-vswitchd, so it makes sense to test the implementation in
ovs-vswitchd itself as well.
Fixes: 6b90bc57e7a2 ("lib/rstp: Remove lock recursion.")
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052925.html
Reported-by: Huangzhidong <huang.zhid...@h3c.com>
Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
---
lib/rstp.c | 6 ++++-
tests/rstp.at | 57 +++++++++++++++++++++++++++++++++++++++++++++
tests/stp.at | 59 +++++++++++++++++++++++++++++++++++++++++++++++
tests/test-rstp.c | 2 ++
4 files changed, 123 insertions(+), 1 deletion(-)
diff --git a/lib/rstp.c b/lib/rstp.c
index 2f01966f7..90e809459 100644
--- a/lib/rstp.c
+++ b/lib/rstp.c
@@ -50,7 +50,7 @@
VLOG_DEFINE_THIS_MODULE(rstp);
-struct ovs_mutex rstp_mutex = OVS_MUTEX_INITIALIZER;
+struct ovs_mutex rstp_mutex;
static struct ovs_list all_rstps__ = OVS_LIST_INITIALIZER(&all_rstps__);
static struct ovs_list *const all_rstps OVS_GUARDED_BY(rstp_mutex) =
&all_rstps__;
@@ -248,6 +248,10 @@ void
rstp_init(void)
OVS_EXCLUDED(rstp_mutex)
{
+ /* We need a recursive mutex because rstp_send_bpdu() could loop back
+ * into the rstp module through a patch port. */
+ ovs_mutex_init_recursive(&rstp_mutex);
+
unixctl_command_register("rstp/tcn", "[bridge]", 0, 1, rstp_unixctl_tcn,
NULL);
unixctl_command_register("rstp/show", "[bridge]", 0, 1, rstp_unixctl_show,
diff --git a/tests/rstp.at b/tests/rstp.at
index 600e85dab..e0d4bed4f 100644
--- a/tests/rstp.at
+++ b/tests/rstp.at
@@ -253,3 +253,60 @@ AT_CHECK([ovs-vsctl del-port br0 p1])
OVS_VSWITCHD_STOP
AT_CLEANUP
+
+AT_SETUP([RSTP - patch ports])
+# Create br0 with interfaces p1 and p7
+# and br1 with interfaces p2 and p8
+# with p1 and p2 being connected patch ports.
+OVS_VSWITCHD_START(
+ [set port br0 other_config:rstp-enable=false -- \
+ set bridge br0 rstp-enable=true
+])
+
+AT_CHECK([add_of_br 1 \
+ set port br1 other_config:rstp-enable=false -- \
+ set bridge br1 rstp-enable=true])
+
+ovs-appctl time/stop
+
+AT_CHECK([ovs-vsctl \
+ add-port br0 p1 -- \
+ set interface p1 type=patch options:peer=p2 ofport_request=1 -- \
+ set port p1 other_config:rstp-enable=true -- \
+ add-port br1 p2 -- \
+ set interface p2 type=patch options:peer=p1 ofport_request=2 -- \
+ set port p2 other_config:rstp-enable=true -- \
+])
+
+AT_CHECK([ovs-vsctl \
+ add-port br0 p7 -- \
+ set interface p7 ofport_request=7 type=dummy -- \
+ set port p7 other_config:rstp-enable=false -- \
+ add-port br1 p8 -- \
+ set interface p8 ofport_request=8 type=dummy -- \
+ set port p8 other_config:rstp-enable=false -- \
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"])
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 icmp actions=7"])
+AT_CHECK([ovs-ofctl add-flow br1 "in_port=8 icmp actions=2"])
+AT_CHECK([ovs-ofctl add-flow br1 "in_port=2 icmp actions=8"])
+
+# Give time for RSTP to synchronize.
+ovs-appctl time/warp 5000 500
+
+OVS_WAIT_UNTIL_EQUAL([cat ovs-vswitchd.log | FILTER_STP_TOPOLOGY], [dnl
+port p1: RSTP state changed from Disabled to Discarding
+port p2: RSTP state changed from Disabled to Discarding
+port p2: RSTP state changed from Discarding to Forwarding
+port p1: RSTP state changed from Discarding to Forwarding])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
| grep Datapath], [0], [dnl
+Datapath actions: 8
+])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'in_port(8),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3,dst=10.0.0.4,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
| grep Datapath], [0], [dnl
+Datapath actions: 7
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
diff --git a/tests/stp.at b/tests/stp.at
index e7bf3958a..75abe8e5c 100644
--- a/tests/stp.at
+++ b/tests/stp.at
@@ -464,6 +464,65 @@ Datapath actions: 2
AT_CLEANUP
+AT_SETUP([STP - patch ports])
+# Create br0 with interfaces p1 and p7
+# and br1 with interfaces p2 and p8
+# with p1 and p2 being connected patch ports.
+OVS_VSWITCHD_START(
+ [set port br0 other_config:stp-enable=false -- \
+ set bridge br0 stp-enable=true
+])
+
+AT_CHECK([add_of_br 1 \
+ set port br1 other_config:stp-enable=false -- \
+ set bridge br1 stp-enable=true])
+
+ovs-appctl time/stop
+
+AT_CHECK([ovs-vsctl \
+ add-port br0 p1 -- \
+ set interface p1 type=patch options:peer=p2 ofport_request=1 -- \
+ set port p1 other_config:stp-enable=true -- \
+ add-port br1 p2 -- \
+ set interface p2 type=patch options:peer=p1 ofport_request=2 -- \
+ set port p2 other_config:stp-enable=true -- \
+])
+
+AT_CHECK([ovs-vsctl \
+ add-port br0 p7 -- \
+ set interface p7 ofport_request=7 type=dummy -- \
+ set port p7 other_config:stp-enable=false -- \
+ add-port br1 p8 -- \
+ set interface p8 ofport_request=8 type=dummy -- \
+ set port p8 other_config:stp-enable=false -- \
+])
+
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=7 icmp actions=1"])
+AT_CHECK([ovs-ofctl add-flow br0 "in_port=1 icmp actions=7"])
+AT_CHECK([ovs-ofctl add-flow br1 "in_port=8 icmp actions=2"])
+AT_CHECK([ovs-ofctl add-flow br1 "in_port=2 icmp actions=8"])
+
+# Give time for STP to synchronize.
+ovs-appctl time/warp 30000 3000
+
+OVS_WAIT_UNTIL_EQUAL([cat ovs-vswitchd.log | FILTER_STP_TOPOLOGY], [dnl
+port <>: STP state changed from disabled to listening
+port <>: STP state changed from disabled to listening
+port <>: STP state changed from listening to learning
+port <>: STP state changed from listening to learning
+port <>: STP state changed from learning to forwarding
+port <>: STP state changed from learning to forwarding])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'in_port(7),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
| grep Datapath], [0], [dnl
+Datapath actions: 8
+])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
'in_port(8),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.3,dst=10.0.0.4,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'
| grep Datapath], [0], [dnl
+Datapath actions: 7
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([STP - flush the fdb and mdb when topology changed])
OVS_VSWITCHD_START([])
diff --git a/tests/test-rstp.c b/tests/test-rstp.c
index 9c1026ec1..707ee3a6c 100644
--- a/tests/test-rstp.c
+++ b/tests/test-rstp.c
@@ -469,6 +469,8 @@ test_rstp_main(int argc, char *argv[])
vlog_set_pattern(VLF_CONSOLE, "%c|%p|%m");
vlog_set_levels(NULL, VLF_SYSLOG, VLL_OFF);
+ rstp_init();
+
if (argc != 2) {
ovs_fatal(0, "usage: test-rstp INPUT.RSTP");
}
--
2.43.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
