diff mbox series

[ovs-dev] rstp: Fix deadlock with patch ports.

Message ID 20240215115600.1780156-1-i.maximets@ovn.org
State Accepted
Commit 3e666ba000b5eff58da8abb4e8c694ac3f7b08d6
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] rstp: Fix deadlock with patch ports. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Ilya Maximets Feb. 15, 2024, 11:55 a.m. UTC 
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.zhidong@h3c.com>
Signed-off-by: Ilya Maximets <i.maximets@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(-)

Comments

Simon Horman Feb. 15, 2024, 1:36 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 12:55:59PM +0100, Ilya Maximets wrote:
> 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.zhidong@h3c.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>

Acked-by: Simon Horman <horms@ovn.org>
Ilya Maximets Feb. 15, 2024, 10:54 p.m. UTC | #2 
On 2/15/24 14:36, Simon Horman wrote:
> On Thu, Feb 15, 2024 at 12:55:59PM +0100, Ilya Maximets wrote:
>> 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.zhidong@h3c.com>
>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> 
> Acked-by: Simon Horman <horms@ovn.org>
> 

Thanks!  Applied and backported down to 2.17.

Best regards, Ilya Maximets.
diff mbox series

Patch

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");
     }