rstp: Fix deadlock with patch ports.

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: 6b90bc5 ("lib/rstp: Remove lock recursion.") Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2024-February/052925.html Reported-by: Huangzhidong <huang.zhidong@h3c.com> Acked-by: Simon Horman <horms@ovn.org> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
openvswitch · Feb 15, 2024 · 3e666ba · 3e666ba
1 parent 5df46a4
commit 3e666ba
Show file tree

Hide file tree

Showing 4 changed files with 123 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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");
     }