From patchwork Thu Feb 15 11:55:59 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1899294 X-Patchwork-Delegate: i.maximets@samsung.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=patchwork.ozlabs.org) Received: from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4TbD5t0jNjz23hm for ; Thu, 15 Feb 2024 22:55:29 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 91895415E4; Thu, 15 Feb 2024 11:55:27 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id U_T0rj-8qCgx; Thu, 15 Feb 2024 11:55:26 +0000 (UTC) X-Comment: SPF check N/A for local connections - client-ip=140.211.9.56; helo=lists.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver= DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 3333441705 Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTPS id 3333441705; Thu, 15 Feb 2024 11:55:26 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 14A99C0072; Thu, 15 Feb 2024 11:55:26 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 05642C0037 for ; Thu, 15 Feb 2024 11:55:23 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id E39E381F01 for ; Thu, 15 Feb 2024 11:55:23 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id N5PFNpvDdlWU for ; Thu, 15 Feb 2024 11:55:22 +0000 (UTC) Received-SPF: Pass (mailfrom) identity=mailfrom; client-ip=217.70.183.200; helo=relay7-d.mail.gandi.net; envelope-from=i.maximets@ovn.org; receiver= DMARC-Filter: OpenDMARC Filter v1.4.2 smtp1.osuosl.org 52CFB81E8F Authentication-Results: smtp1.osuosl.org; dmarc=none (p=none dis=none) header.from=ovn.org DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 52CFB81E8F Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by smtp1.osuosl.org (Postfix) with ESMTPS id 52CFB81E8F for ; Thu, 15 Feb 2024 11:55:21 +0000 (UTC) Received: by mail.gandi.net (Postfix) with ESMTPSA id 8BBDC2000D; Thu, 15 Feb 2024 11:55:18 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Thu, 15 Feb 2024 12:55:59 +0100 Message-ID: <20240215115600.1780156-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.43.0 MIME-Version: 1.0 X-GND-Sasl: i.maximets@ovn.org Cc: Huangzhidong , Ilya Maximets Subject: [ovs-dev] [PATCH] rstp: Fix deadlock with patch ports. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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 Signed-off-by: Ilya Maximets Acked-by: Simon Horman --- 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"); }