From patchwork Thu Mar 26 19:58:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yifeng Sun X-Patchwork-Id: 1262243 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.137; helo=fraxinus.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=Yzth+M89; dkim-atps=neutral Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48pG596Th3z9sSH for ; Fri, 27 Mar 2020 06:58:37 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id B3599873A3; Thu, 26 Mar 2020 19:58:35 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id uLkjqxsBXyfR; Thu, 26 Mar 2020 19:58:34 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by fraxinus.osuosl.org (Postfix) with ESMTP id 5837187398; Thu, 26 Mar 2020 19:58:34 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 12880C1D8F; Thu, 26 Mar 2020 19:58:34 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id 446F6C0177 for ; Thu, 26 Mar 2020 19:58:32 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 0D22A891B5 for ; Thu, 26 Mar 2020 19:58:32 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zHQpd82eQSPM for ; Thu, 26 Mar 2020 19:58:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by hemlock.osuosl.org (Postfix) with ESMTPS id 36053891A7 for ; Thu, 26 Mar 2020 19:58:30 +0000 (UTC) Received: by mail-pj1-f50.google.com with SMTP id v13so2934288pjb.0 for ; Thu, 26 Mar 2020 12:58:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=vde2zMhMJWClWNzQRILGph3Xq2fXpykbTczqY6Q2ta8=; b=Yzth+M8936/7aNxUZnGrxg6cIiuT/IVI0KvtaXSJNFCQeQ9wh90inXxDJNYNoChlWN XRuxj/lhLRLZX6agR57R/D+yfhdRUVsVwMPPnMX8WjunZdL29BUOVHFCWBkPtQukLffB lyheGwhuSPpuaIEk0Z1cuJoNrEVIcWakY42V3T1GjkDpTswNNU7FAPvopwBKUz/X8YJ5 MIqW5vmawHWkYGik8LN76jXVwDzfKZRwMsC6YOYYugqiiRRBblVvwCTDT5s7msibGlLV KSuekvzS2jmcMtaTyt0Hor+zZmxtrT9XsoRQuUZ84i4mjPu04ubFrWcHGVrKmPFMe5ws 7qiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=vde2zMhMJWClWNzQRILGph3Xq2fXpykbTczqY6Q2ta8=; b=sGUork+x9Pno1Yn/fZ/u43DS63Ep2fJw+xUtA2/7CjUYerSdw1OEbz0teLKwBWAO4x ZHsrRxw4R6H+lDCb9oemAFHp+vTV6uZnVJWtd1Ou+x2exvZFkCPjPnxuJevLWl4LB4wz g+GH5OaG6sXwqMKqGYPYMNZTfFKaH8419lLmGJOTRO5IkaIYfglAY+ZZCv/R0rUQIc8R grO3wIFLEsP8zkxAwXFDIcCnrXpbdyR6SpfULEdUQAaLUb961Gth5sMwFDZJpRQB9riu yERzdmq6TF8ow8GTLEPhm1jYWX4DTtuEvpDOr6hI/NuCAnVT3Hft6IYdpFc9HlHpX2s6 wpHg== X-Gm-Message-State: ANhLgQ1avBjUsN3BLZoPWpHM+uZZyTT1hnNoDtzlsiYTJSOfJtsZ3jbp +5qCo67tgiqChwujx4W3Yr/g9lDD0Fg= X-Google-Smtp-Source: ADFU+vssNgiYzaV3HyRcpo9IjyrJw/0KwVrOsWwX0N11LTlZBWOG6lb1ltXmcBBoQI8PQS8nz0kGFg== X-Received: by 2002:a17:90b:11ce:: with SMTP id gv14mr1291577pjb.63.1585252709072; Thu, 26 Mar 2020 12:58:29 -0700 (PDT) Received: from kern417.eng.vmware.com ([66.170.99.1]) by smtp.gmail.com with ESMTPSA id u41sm2320043pgn.8.2020.03.26.12.58.28 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 26 Mar 2020 12:58:28 -0700 (PDT) From: Yifeng Sun To: dev@openvswitch.org Date: Thu, 26 Mar 2020 12:58:21 -0700 Message-Id: <1585252702-8649-1-git-send-email-pkusunyifeng@gmail.com> X-Mailer: git-send-email 2.7.4 Subject: [ovs-dev] [PATCH 1/2] tun_metadata: Fix coredump caused by use-after-free bug 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Tun_metadata can be referened by flow and frozen_state at the same time. When ovs-vswitchd handles TLV table mod message, the involved tun_metadata gets freed. The call trace to free tun_metadata is shown as below: ofproto_run - handle_openflow - handle_single_part_openflow - handle_tlv_table_mod - tun_metadata_table_mod - tun_metadata_postpone_free Unfortunately, this tun_metadata can be still used by some frozen_state, and later on when frozen_state tries to access its tun_metadata table, ovs-vswitchd crashes. The call trace to access tun_metadata from frozen_state is shown as below: udpif_upcall_handler - recv_upcalls - process_upcall - frozen_metadata_to_flow This patch fixes it by introducing a reference count to tun_metadata. Whenever a pointer of tun_metadata is passed between flow and frozen_state, we increase its reference count. Reference count is decreased at deallocation. In present code, pointer of tun_metadata can be passed between flows. It is safe because of RCU mechanism. VMware-BZ: #2526222 Signed-off-by: Yifeng Sun --- lib/tun-metadata.c | 29 ++++++++++++++++++++++++++++- lib/tun-metadata.h | 2 ++ ofproto/ofproto-dpif-rid.c | 8 ++++++++ ofproto/ofproto-dpif-rid.h | 2 ++ 4 files changed, 40 insertions(+), 1 deletion(-) diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c index f8a0e19524e9..c4218a034a92 100644 --- a/lib/tun-metadata.c +++ b/lib/tun-metadata.c @@ -25,6 +25,7 @@ #include "nx-match.h" #include "odp-netlink.h" #include "openvswitch/ofp-match.h" +#include "ovs-atomic.h" #include "ovs-rcu.h" #include "packets.h" #include "tun-metadata.h" @@ -40,6 +41,11 @@ struct tun_meta_entry { /* Maps from TLV option class+type to positions in a struct tun_metadata's * 'opts' array. */ struct tun_table { + /* Struct tun_table can be referenced by struct frozen_state for a long + * time. This ref_cnt protects tun_table from being freed if it is still + * being used somewhere. */ + struct ovs_refcount ref_cnt; + /* TUN_METADATA is stored in element . */ struct tun_meta_entry entries[TUN_METADATA_NUM_OPTS]; @@ -79,6 +85,24 @@ tun_key_type(uint32_t key) return key & 0xff; } +void +tun_metadata_ref(const struct tun_table *tab) +{ + if (tab) { + ovs_refcount_ref(&CONST_CAST(struct tun_table *, tab)->ref_cnt); + } +} + +unsigned int +tun_metadata_unref(const struct tun_table *tab) +{ + if (tab) { + return ovs_refcount_unref_relaxed( + &CONST_CAST(struct tun_table *, tab)->ref_cnt); + } + return -1; +} + /* Returns a newly allocated tun_table. If 'old_map' is nonnull then the new * tun_table is a deep copy of the old one. */ struct tun_table * @@ -111,6 +135,7 @@ tun_metadata_alloc(const struct tun_table *old_map) hmap_init(&new_map->key_hmap); } + ovs_refcount_init(&new_map->ref_cnt); return new_map; } @@ -135,7 +160,9 @@ tun_metadata_free(struct tun_table *map) void tun_metadata_postpone_free(struct tun_table *tab) { - ovsrcu_postpone(tun_metadata_free, tab); + if (tun_metadata_unref(tab) == 1) { + ovsrcu_postpone(tun_metadata_free, tab); + } } enum ofperr diff --git a/lib/tun-metadata.h b/lib/tun-metadata.h index 7dad9504b8da..933021a0f679 100644 --- a/lib/tun-metadata.h +++ b/lib/tun-metadata.h @@ -33,6 +33,8 @@ struct ofputil_tlv_table_mod; struct ofputil_tlv_table_reply; struct tun_table; +void tun_metadata_ref(const struct tun_table *tab); +unsigned int tun_metadata_unref(const struct tun_table *tab); struct tun_table *tun_metadata_alloc(const struct tun_table *old_map); void tun_metadata_free(struct tun_table *); void tun_metadata_postpone_free(struct tun_table *); diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c index 29aafc2c0b40..d479e53d9b2d 100644 --- a/ofproto/ofproto-dpif-rid.c +++ b/ofproto/ofproto-dpif-rid.c @@ -201,6 +201,7 @@ static void frozen_state_clone(struct frozen_state *new, const struct frozen_state *old) { *new = *old; + tun_metadata_ref(old->metadata.tunnel.metadata.tab); new->stack = (new->stack_size ? xmemdup(new->stack, new->stack_size) : NULL); @@ -218,10 +219,17 @@ frozen_state_clone(struct frozen_state *new, const struct frozen_state *old) static void frozen_state_free(struct frozen_state *state) { + struct tun_table *tab; + free(state->stack); free(state->ofpacts); free(state->action_set); free(state->userdata); + + tab = CONST_CAST(struct tun_table *, state->metadata.tunnel.metadata.tab); + if (tun_metadata_unref(tab) == 1) { + tun_metadata_free(tab); + } } /* Allocate a unique recirculation id for the given set of flow metadata. diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index e5d02caf28a3..1fefbf53b94a 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -115,6 +115,7 @@ frozen_metadata_from_flow(struct frozen_metadata *md, { memset(md, 0, sizeof *md); md->tunnel = flow->tunnel; + tun_metadata_ref(flow->tunnel.metadata.tab); md->metadata = flow->metadata; memcpy(md->regs, flow->regs, sizeof md->regs); md->in_port = flow->in_port.ofp_port; @@ -125,6 +126,7 @@ frozen_metadata_to_flow(const struct frozen_metadata *md, struct flow *flow) { flow->tunnel = md->tunnel; + tun_metadata_ref(md->tunnel.metadata.tab); flow->metadata = md->metadata; memcpy(flow->regs, md->regs, sizeof flow->regs); flow->in_port.ofp_port = md->in_port; From patchwork Thu Mar 26 19:58:22 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yifeng Sun X-Patchwork-Id: 1262244 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20161025 header.b=LLdlIiap; dkim-atps=neutral Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48pG5C3kGgz9sSH for ; Fri, 27 Mar 2020 06:58:39 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 9CBBC891C8; Thu, 26 Mar 2020 19:58:35 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XCBaKo8Dp9MF; Thu, 26 Mar 2020 19:58:34 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 70545891A7; Thu, 26 Mar 2020 19:58:33 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 54C11C1831; Thu, 26 Mar 2020 19:58:33 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 0A99FC0177 for ; Thu, 26 Mar 2020 19:58:32 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 248DF805D8 for ; Thu, 26 Mar 2020 19:58:31 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rFJ6ZeMwSVqQ for ; Thu, 26 Mar 2020 19:58:30 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail-pf1-f194.google.com (mail-pf1-f194.google.com [209.85.210.194]) by whitealder.osuosl.org (Postfix) with ESMTPS id 96E33886E9 for ; Thu, 26 Mar 2020 19:58:30 +0000 (UTC) Received: by mail-pf1-f194.google.com with SMTP id 23so3348313pfj.1 for ; Thu, 26 Mar 2020 12:58:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=+cjXP01YAxijz1yxAeyTFYEgCF9J0p4oGcqkuMCQEjQ=; b=LLdlIiapOTYMhbBUHlihQ3/+zdkIdhYIRbmfmhEz8ZOEkdXWNICti3xHoIQ+i2T7BV b74IyDr1ZK0YS4iLHUeoSf/wGCg60zboxhcwwi964DEyA4n4vUB0437gbf7PRUv5YuLF sMSmKB4/k/KdFEJbw9frui/lOphMsqBCaYdA0Z99ibmecMXWU3M9J66/t2mR5Hu9rLcm Jk80AD6QbttVJDqpqm1CX8D2UfktnDrAjYMCWzArj+0e0ErpGlVeoXGEy+NMtILGFM1o PQy0ayErGVYRHpcLdR7UqW2J9xP8xft0qw6K7lv9eT4JLyVellUzvAarHs9Lczx2UylS HyiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=+cjXP01YAxijz1yxAeyTFYEgCF9J0p4oGcqkuMCQEjQ=; b=Mqp8GhVrYHXtXdztl/sgyg+VeWn3B4i3Cs5CjtGf9YUpu+HndBJmnVKn7X0VDFkluH neibXLT5i+7Unpj+uMpzPTX+IixmzPG58pCQEY6s5maSQQdUnFPpuUKsBqENccpeiu4U mnu9JKSl+4tlVLIqSkmh4VPVAg5ZUvO5wl8t+tbhKh+h7DPLZ1g8hAE/GUyvMDZaM0lX mgfv9iVQcAAwd0vNv5JOAOQpNWgytyKfo2OY4F7R4vjRSVVxZAJfxzfPAIKzLtvijUdB xU8+xZntTmR5G6lNgH5Q6ISDX952JUcWovkYxPCp71jhjNA9NRyLRldu0r4ZwSdmOzIC rLDg== X-Gm-Message-State: ANhLgQ25yuL9j5uK9QgRYAneBTfefFMQzw6au6pK571sWbRKeUFCURmB xJiyK1U+DK/2Ba7CkFiLDcuMoKqJISw= X-Google-Smtp-Source: ADFU+vts1e8c/WwBqWes/og9y3Usnnxxg3SkCMUtHfer8MxXy19Nk+9+UkE853JkKPNe4XUFw8NPlg== X-Received: by 2002:aa7:8ec1:: with SMTP id b1mr10597138pfr.125.1585252709833; Thu, 26 Mar 2020 12:58:29 -0700 (PDT) Received: from kern417.eng.vmware.com ([66.170.99.1]) by smtp.gmail.com with ESMTPSA id u41sm2320043pgn.8.2020.03.26.12.58.29 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 26 Mar 2020 12:58:29 -0700 (PDT) From: Yifeng Sun To: dev@openvswitch.org Date: Thu, 26 Mar 2020 12:58:22 -0700 Message-Id: <1585252702-8649-2-git-send-email-pkusunyifeng@gmail.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1585252702-8649-1-git-send-email-pkusunyifeng@gmail.com> References: <1585252702-8649-1-git-send-email-pkusunyifeng@gmail.com> Subject: [ovs-dev] [PATCH 2/2] system-traffic: Check frozen state handling with TLV map change 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: , MIME-Version: 1.0 Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" This patch enhances a system traffic test to prevent regression on the tunnel metadata table (tun_table) handling with frozen state. Without a proper fix this test can crash ovs-vswitchd due to a use-after-free bug on tun_table. These are the timed sequence of how this bug is triggered: - Adds an OpenFlow rule in OVS that matches Geneve tunnel metadata that contains a controller action. - When the first packet matches the aforementioned OpenFlow rule, during the miss upcall, OVS stores a pointer to the tun_table (that decodes the Geneve tunnel metadata) in a frozen state and pushes down a datapath flow into kernel datapath. - Issues a add-tlv-map command to reprogram the tun_table on OVS. OVS frees the old tun_table and create a new tun_table. - A subsequent packet hits the kernel datapath flow again. Since there is a controller action associated with that flow, it triggers slow path controller upcall. - In the slow path controller upcall, OVS derives the tun_table from the frozen state, which points to the old tun_table that is already being freed at this time point. - In order to access the tunnel metadata, OVS uses the invalid pointer that points to the old tun_table and triggers the core dump. Signed-off-by: Yi-Hung Wei Signed-off-by: Yifeng Sun Co-authored-by: Yi-Hung Wei --- tests/system-traffic.at | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 4a39c929c207..992de8546c41 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -611,6 +611,20 @@ NS_CHECK_EXEC([at_ns0], [ping -q -c 3 10.1.1.100 | FORMAT_PING], [0], [dnl 3 packets transmitted, 3 received, 0% packet loss, time 0ms ]) +dnl Test OVS handles TLV map modifictions properly when restores frozen state. +NS_CHECK_EXEC([at_ns0], [ping 10.1.1.100 > ping.out &]) + +sleep 2 + +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0x88,len=4}->tun_metadata1"]) +sleep 1 +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0x99,len=4}->tun_metadata2"]) +sleep 1 +AT_CHECK([ovs-ofctl add-tlv-map br0 "{class=0xffff,type=0xaa,len=4}->tun_metadata3"]) +sleep 1 + +dnl At this point, ovs-vswitchd will either crash or everything is OK. + OVS_APP_EXIT_AND_WAIT([ovs-ofctl]) OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP