From patchwork Wed Feb 23 18:48:12 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gaetan Rivet X-Patchwork-Id: 1596853 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: bilbo.ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=u256.net header.i=@u256.net header.a=rsa-sha256 header.s=fm1 header.b=DhmiTCca; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=messagingengine.com header.i=@messagingengine.com header.a=rsa-sha256 header.s=fm2 header.b=X+mAt7qO; dkim-atps=neutral Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp1.osuosl.org (smtp1.osuosl.org [140.211.166.138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by bilbo.ozlabs.org (Postfix) with ESMTPS id 4K3lRc758Lz9sFq for ; Thu, 24 Feb 2022 05:48:28 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 2A2F4829B1; Wed, 23 Feb 2022 18:48:26 +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 ZY_wOvYJeDAH; Wed, 23 Feb 2022 18:48:25 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp1.osuosl.org (Postfix) with ESMTPS id 2A529828B5; Wed, 23 Feb 2022 18:48:24 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D6F46C0021; Wed, 23 Feb 2022 18:48:23 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 374F3C0011 for ; Wed, 23 Feb 2022 18:48:22 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 17CB4401E7 for ; Wed, 23 Feb 2022 18:48:22 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp4.osuosl.org (amavisd-new); dkim=pass (2048-bit key) header.d=u256.net header.b="DhmiTCca"; dkim=pass (2048-bit key) header.d=messagingengine.com header.b="X+mAt7qO" 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 W9jWqUP_PmAO for ; Wed, 23 Feb 2022 18:48:21 +0000 (UTC) X-Greylist: from auto-whitelisted by SQLgrey-1.8.0 Received: from wout1-smtp.messagingengine.com (wout1-smtp.messagingengine.com [64.147.123.24]) by smtp4.osuosl.org (Postfix) with ESMTPS id 10077401D9 for ; Wed, 23 Feb 2022 18:48:20 +0000 (UTC) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 9E0363201FEF; Wed, 23 Feb 2022 13:48:18 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Wed, 23 Feb 2022 13:48:18 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=u256.net; h=cc :cc:content-transfer-encoding:date:date:from:from:in-reply-to :message-id:mime-version:reply-to:sender:subject:subject:to:to; s=fm1; bh=WcwQcYk8k4x3GXSkII35nITkQbH2f3XlJkIYJ+8sy7Y=; b=DhmiT CcaLdlPcfbPUGp4OTCj6+3ff5d3EAb3c9aawI2tCjCy/S1PGlEefkfy4mqpsl7GB jvAZfBcMO6DMiDwh0U6gljXaBk9vKPoFlaCVaBUqVsg18mODRWoRmzfabkcwyEKo PhHvEB7deWuGGg3Uyd7JiGKxAe9Ylcb7AMYFWPYMW8lFk+lF6DYbFKoz8ube1eV2 4eFgNT1xHAEnj5/kHYXcfgD/QYMr5Y5Iu83v7Z1Ed0DqzAGn5RYy9IxDcSNn5JvV 3TmABtwurX64LtDj76XJnK1M4v+rNLy+odDEMO06tpyyhCLJokwPpROM8LEfC6u1 NQZq/1V/joysBijZw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding:date:date :from:from:in-reply-to:message-id:mime-version:reply-to:sender :subject:subject:to:to:x-me-proxy:x-me-proxy:x-me-sender :x-me-sender:x-sasl-enc; s=fm2; bh=WcwQcYk8k4x3GXSkII35nITkQbH2f 3XlJkIYJ+8sy7Y=; b=X+mAt7qO/lvrba0xBZsZfpmIc4WE/wJNLuDpg2nb0bEhb VwUzqzwJ1Ip7HPsDzi9blcezu87JFSB5AlOTn6vbSRHBN4D+xK9IILyIZdoFyu5a jXeQlcDheAgBrZom+4T4SfkhwXC7PQdvrja88WA6qMFBX/+rSCBlhYy8frtAieID ZqPF13KzqX6DN8dNa4shOz3pOG9niPxQ9xwaGR6cVGR7ORX8oN+R32cVdt20XkAo jm0ZKkGBPUrIEs/kVYgahsiQ0i5nS0M4SrL+KW87/M8V/nr5quyu3CpfdJ6VA97C sJgh1wMHeA0wH0YPC0Bla7ImLRXyz+lpE3H7A/LlQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvvddrledtgdduudehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkffoggfgsedtkeertdertddtnecuhfhrohhmpefirggvthgrnhcu tfhivhgvthcuoehgrhhivhgvsehuvdehiedrnhgvtheqnecuggftrfgrthhtvghrnhephe egleefffejgfegueelhfdvieffleevueeggfdtfeefjedvuefhgfeiudeufeetnecuffho mhgrihhnpehoiihlrggsshdrohhrghdpghhithhhuhgsrdgtohhmnecuvehluhhsthgvrh fuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepghhrihhvvgesuhdvheeirdhn vght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 23 Feb 2022 13:48:16 -0500 (EST) From: Gaetan Rivet To: ovs-dev@openvswitch.org Date: Wed, 23 Feb 2022 19:48:12 +0100 Message-Id: <20220223184812.2413004-1-grive@u256.net> X-Mailer: git-send-email 2.31.1 MIME-Version: 1.0 Subject: [ovs-dev] [PATCH] ofproto: Use xlate map for uuid lookups 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 ofproto map 'all_ofproto_dpifs_by_uuid' does not support concurrent accesses. It is however read by upcall handler threads and written by the main thread at the same time. Additionally, handler threads will change the ams_seq while an ofproto is being destroyed, triggering crashes with the following backtrace: (gdb) bt hmap_next (hmap.h:398) seq_wake_waiters (seq.c:326) seq_change_protected (seq.c:134) seq_change (seq.c:144) ofproto_dpif_send_async_msg (ofproto_dpif.c:263) process_upcall (ofproto_dpif_upcall.c:1782) recv_upcalls (ofproto_dpif_upcall.c:1026) udpif_upcall_handler (ofproto/ofproto_dpif_upcall.c:945) ovsthread_wrapper (ovs_thread.c:734) To solve both issues, remove the 'all_ofproto_dpifs_by_uuid'. Instead, another map already storing ofprotos in xlate can be used. During an ofproto destruction, its reference is removed from the current xlate xcfg. Such change is committed only after all threads have quiesced at least once during xlate_txn_commit(). This wait ensures that the removal is seen by all threads, rendering impossible for a thread to still hold a reference while the destruction proceeds. Furthermore, the xlate maps are copied during updates instead of being written in place. It is thus correct to read xcfg->xbridges while inserting or removing from new_xcfg->xbridges. Finally, now that ofproto_dpifs lookups are done through xcfg->xbridges, it is important to use a high level of entropy. As it used the ofproto pointer hashed, fewer bits were random compared to the uuid key used in 'all_ofproto_dpifs_by_uuid'. To solve this, use the ofproto uuid as the key in xbridges as well, improving entropy. Fixes: fcb9579be3c7 ("ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user action cookie.") Suggested-by: Adrian Moreno Signed-off-by: Yunjian Wang Signed-off-by: Gaetan Rivet Tested-by: Alin-Gabriel Serdean Acked-by: Alin-Gabriel Serdean Signed-off-by: Yunjian Wang Signed-off-by: Gaetan Rivet Acked-by: Adrian Moreno --- Following the discussion on the fix https://patchwork.ozlabs.org/project/openvswitch/patch/1638530715-44436-1-git-send-email-wangyunjian@huawei.com/ I tested it with Peng's ofproto refcount patch in tree: https://patchwork.ozlabs.org/project/openvswitch/patch/20220219032607.15757-1-hepeng.0320@bytedance.com/ CI result: https://github.com/grivet/ovs/actions/runs/1889083195 ofproto/ofproto-dpif-xlate.c | 21 +++++++++++++++++++-- ofproto/ofproto-dpif-xlate.h | 1 + ofproto/ofproto-dpif.c | 19 +------------------ 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 578cbfe58..4a7388071 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -865,7 +865,7 @@ xlate_xbridge_init(struct xlate_cfg *xcfg, struct xbridge *xbridge) ovs_list_init(&xbridge->xbundles); hmap_init(&xbridge->xports); hmap_insert(&xcfg->xbridges, &xbridge->hmap_node, - hash_pointer(xbridge->ofproto, 0)); + uuid_hash(&xbridge->ofproto->uuid)); } static void @@ -1639,7 +1639,7 @@ xbridge_lookup(struct xlate_cfg *xcfg, const struct ofproto_dpif *ofproto) xbridges = &xcfg->xbridges; - HMAP_FOR_EACH_IN_BUCKET (xbridge, hmap_node, hash_pointer(ofproto, 0), + HMAP_FOR_EACH_IN_BUCKET (xbridge, hmap_node, uuid_hash(&ofproto->uuid), xbridges) { if (xbridge->ofproto == ofproto) { return xbridge; @@ -1661,6 +1661,23 @@ xbridge_lookup_by_uuid(struct xlate_cfg *xcfg, const struct uuid *uuid) return NULL; } +struct ofproto_dpif * +xlate_ofproto_lookup(const struct uuid *uuid) +{ + struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); + struct xbridge *xbridge; + + if (!xcfg) { + return NULL; + } + + xbridge = xbridge_lookup_by_uuid(xcfg, uuid); + if (xbridge != NULL) { + return xbridge->ofproto; + } + return NULL; +} + static struct xbundle * xbundle_lookup(struct xlate_cfg *xcfg, const struct ofbundle *ofbundle) { diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index 851088d79..2ba90e999 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -176,6 +176,7 @@ void xlate_ofproto_set(struct ofproto_dpif *, const char *name, struct dpif *, bool forward_bpdu, bool has_in_band, const struct dpif_backer_support *support); void xlate_remove_ofproto(struct ofproto_dpif *); +struct ofproto_dpif *xlate_ofproto_lookup(const struct uuid *uuid); void xlate_bundle_set(struct ofproto_dpif *, struct ofbundle *, const char *name, enum port_vlan_mode, diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 8143dd965..7b4a1b3d8 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -215,10 +215,6 @@ struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers); static struct hmap all_ofproto_dpifs_by_name = HMAP_INITIALIZER(&all_ofproto_dpifs_by_name); -/* All existing ofproto_dpif instances, indexed by ->uuid. */ -static struct hmap all_ofproto_dpifs_by_uuid = - HMAP_INITIALIZER(&all_ofproto_dpifs_by_uuid); - static bool ofproto_use_tnl_push_pop = true; static void ofproto_unixctl_init(void); static void ct_zone_config_init(struct dpif_backer *backer); @@ -1720,9 +1716,6 @@ construct(struct ofproto *ofproto_) hmap_insert(&all_ofproto_dpifs_by_name, &ofproto->all_ofproto_dpifs_by_name_node, hash_string(ofproto->up.name, 0)); - hmap_insert(&all_ofproto_dpifs_by_uuid, - &ofproto->all_ofproto_dpifs_by_uuid_node, - uuid_hash(&ofproto->uuid)); memset(&ofproto->stats, 0, sizeof ofproto->stats); ofproto_init_tables(ofproto_, N_TABLES); @@ -1820,8 +1813,6 @@ destruct(struct ofproto *ofproto_, bool del) hmap_remove(&all_ofproto_dpifs_by_name, &ofproto->all_ofproto_dpifs_by_name_node); - hmap_remove(&all_ofproto_dpifs_by_uuid, - &ofproto->all_ofproto_dpifs_by_uuid_node); OFPROTO_FOR_EACH_TABLE (table, &ofproto->up) { CLS_FOR_EACH (rule, up.cr, &table->cls) { @@ -5818,15 +5809,7 @@ ofproto_dpif_lookup_by_name(const char *name) struct ofproto_dpif * ofproto_dpif_lookup_by_uuid(const struct uuid *uuid) { - struct ofproto_dpif *ofproto; - - HMAP_FOR_EACH_WITH_HASH (ofproto, all_ofproto_dpifs_by_uuid_node, - uuid_hash(uuid), &all_ofproto_dpifs_by_uuid) { - if (uuid_equals(&ofproto->uuid, uuid)) { - return ofproto; - } - } - return NULL; + return xlate_ofproto_lookup(uuid); } static void