From patchwork Mon Mar 8 03:34:11 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Hongzhi Guo X-Patchwork-Id: 1448830 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=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) 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 RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Dv3qp2TWQz9sCD for ; Mon, 8 Mar 2021 14:34:44 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 5BC914E6C3; Mon, 8 Mar 2021 03:34:40 +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 mkVbTgXN68JS; Mon, 8 Mar 2021 03:34:38 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp4.osuosl.org (Postfix) with ESMTP id B44A04C783; Mon, 8 Mar 2021 03:34:37 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 86C0DC000B; Mon, 8 Mar 2021 03:34:37 +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 15D35C0001 for ; Mon, 8 Mar 2021 03:34:35 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 03DAD83981 for ; Mon, 8 Mar 2021 03:34:35 +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 aYh-oacWKq1u for ; Mon, 8 Mar 2021 03:34:32 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by smtp1.osuosl.org (Postfix) with ESMTPS id E36E28343B for ; Mon, 8 Mar 2021 03:34:31 +0000 (UTC) Received: from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.60]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4Dv3nQ5b3CzrSDF; Mon, 8 Mar 2021 11:32:42 +0800 (CST) Received: from DESKTOP-ORJPOMD.china.huawei.com (10.174.243.147) by DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id 14.3.498.0; Mon, 8 Mar 2021 11:34:19 +0800 From: Hongzhi Guo To: Date: Mon, 8 Mar 2021 11:34:11 +0800 Message-ID: <20210308033411.32040-1-guohongzhi1@huawei.com> X-Mailer: git-send-email 2.21.0.windows.1 MIME-Version: 1.0 X-Originating-IP: [10.174.243.147] X-CFilter-Loop: Reflected Cc: guohongzhi1@huawei.com, jerry.lilijun@huawei.com, i.maximets@ovn.org, Jarno Rajahalme , zhoujingbin@huawei.com, chenchanghu@huawei.com Subject: [ovs-dev] [PATCH] [ovs-dev v2]ofproto:fix use-after-free of ofproto 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" ASAN report use-after-free of ofproto when destroy ofproto_rule. The rule uses both RCU and refcount, while the ofproto uses only RCU, and the rule retains the pointer of the proto. More importantly, ofproto cannot guarantee a longer grace period than the rule. So when the rule is deleted, it is possible that ofproto has been released, resulting in use-after-free of ofproto. This patch add ref_count for ofproto to avoid use-after-free. =================== ASAN report as following: ==10399==ERROR: AddressSanitizer: heap-use-after-free on address 0xffff61e1e420 at pc 0xaaaadcc29d1c bp 0xffff6c5fde40 sp 0xffff6c5fde60 READ of size 8 at 0xffff61e1e420 thread T12 (urcu2)     #0 0xaaaadcc29d1b (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:2916)     #1 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))     #2 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:363)     #3 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-thread.c:708)     #4 0xffff80fde8bb in start_thread (/lib64/libpthread.so.0+0x78bb)     #5 0xffff808fa5cb in thread_start (/lib64/libc.so.6+0xd55cb) 0xffff61e1e420 is located 32 bytes inside of 34496-byte region [0xffff61e1e400,0xffff61e26ac0) freed by thread T12 (urcu2) here:     #0 0xffff8214fe33 in free (/lib64/libasan.so.4+0xd2e33)     #1 0xaaaadcc576df (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto-dpif.c:734)     #2 0xaaaadcc21acb (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:1687)     #3 0xaaaadcf76f5f (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:348 (discriminator 3))     #4 0xaaaadcf770c7 (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-rcu.c:363)     #5 0xaaaadcf7fa9b (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/ovs-thread.c:708)     #6 0xffff80fde8bb in start_thread (/lib64/libpthread.so.0+0x78bb)     #7 0xffff808fa5cb in thread_start (/lib64/libc.so.6+0xd55cb) previously allocated by thread T0 here:     #0 0xffff821503c3 in __interceptor_calloc (/lib64/libasan.so.4+0xd33c3)     #1 0xaaaadd034717 in xcalloc (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/util.c:99 (discriminator 3))     #2 0xaaaadd034767 in xzalloc (/usr/src/debug/openvswitch-2.12.asan.aarch64/lib/util.c:110)     #3 0xaaaadcc576ab (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto-dpif.c:726)     #4 0xaaaadcc1be93 in ofproto_create (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:505)     #5 0xaaaadcbd793f (/usr/src/debug/openvswitch-2.12.asan.aarch64/vswitchd/bridge.c:1208)     #6 0xaaaadcbeefb7 in bridge_run (/usr/src/debug/openvswitch-2.12.asan.aarch64/vswitchd/bridge.c:3944 (discriminator 4))     #7 0xaaaadcbfdb83 in main (/usr/src/debug/openvswitch-2.12.asan.aarch64/vswitchd/ovs-vswitchd.c:240)     #8 0xffff80845adf in __libc_start_main (/lib64/libc.so.6+0x20adf)     #9 0xaaaadcbd19b3 (/usr/sbin/ovs-vswitchd-2.12.asan+0x26f9b3)     SUMMARY: AddressSanitizer: heap-use-after-free (/usr/src/debug/openvswitch-2.12.asan.aarch64/ofproto/ofproto.c:2916) =================== TEST: 1.ASAN test for three days CC: Jarno Rajahalme Fixes: 39c9459355b6 ("Use classifier versioning.") Signed-off-by: Hongzhi Guo Signed-off-by: hepeng --- ofproto/ofproto-dpif-xlate-cache.c | 1 + ofproto/ofproto-dpif.c | 20 ++++++++++------- ofproto/ofproto-provider.h | 5 +++++ ofproto/ofproto.c | 35 ++++++++++++++++++++++++++---- 4 files changed, 49 insertions(+), 12 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate-cache.c b/ofproto/ofproto-dpif-xlate-cache.c index dcc91cb38..0deee365d 100644 --- a/ofproto/ofproto-dpif-xlate-cache.c +++ b/ofproto/ofproto-dpif-xlate-cache.c @@ -209,6 +209,7 @@ xlate_cache_clear_entry(struct xc_entry *entry) { switch (entry->type) { case XC_TABLE: + ofproto_unref(&(entry->table.ofproto->up)); break; case XC_RULE: ofproto_rule_unref(&entry->rule->up); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index fd0b2fdea..8b22eda5a 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4416,10 +4416,12 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, if (xcache) { struct xc_entry *entry; - entry = xlate_cache_add_entry(xcache, XC_TABLE); - entry->table.ofproto = ofproto; - entry->table.id = *table_id; - entry->table.match = true; + if (ofproto_try_ref(&ofproto->up)) { + entry = xlate_cache_add_entry(xcache, XC_TABLE); + entry->table.ofproto = ofproto; + entry->table.id = *table_id; + entry->table.match = true; + } } return rule; } @@ -4452,10 +4454,12 @@ rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto, if (xcache) { struct xc_entry *entry; - entry = xlate_cache_add_entry(xcache, XC_TABLE); - entry->table.ofproto = ofproto; - entry->table.id = next_id; - entry->table.match = (rule != NULL); + if (ofproto_try_ref(&ofproto->up)) { + entry = xlate_cache_add_entry(xcache, XC_TABLE); + entry->table.ofproto = ofproto; + entry->table.id = next_id; + entry->table.match = (rule != NULL); + } } if (rule) { goto out; /* Match. */ diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 9ad2b71d2..8f2f41e0e 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -139,6 +139,8 @@ struct ofproto { /* Variable length mf_field mapping. Stores all configured variable length * meta-flow fields (struct mf_field) in a switch. */ struct vl_mff_map vl_mff_map; + + struct ovs_refcount ref_count; }; void ofproto_init_tables(struct ofproto *, int n_tables); @@ -442,6 +444,9 @@ struct rule { void ofproto_rule_ref(struct rule *); bool ofproto_rule_try_ref(struct rule *); void ofproto_rule_unref(struct rule *); +void ofproto_ref(struct ofproto *); +bool ofproto_try_ref(struct ofproto *); +void ofproto_unref(struct ofproto *); static inline const struct rule_actions * rule_get_actions(const struct rule *); static inline bool rule_is_table_miss(const struct rule *); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index b91517cd2..7037a9684 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -545,6 +545,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type, ovs_mutex_init(&ofproto->vl_mff_map.mutex); cmap_init(&ofproto->vl_mff_map.cmap); + ovs_refcount_init(&ofproto->ref_count); error = ofproto->ofproto_class->construct(ofproto); if (error) { @@ -1738,8 +1739,7 @@ ofproto_destroy(struct ofproto *p, bool del) p->connmgr = NULL; ovs_mutex_unlock(&ofproto_mutex); - /* Destroying rules is deferred, must have 'ofproto' around for them. */ - ovsrcu_postpone(ofproto_destroy_defer__, p); + ofproto_unref(p); } /* Destroys the datapath with the respective 'name' and 'type'. With the Linux @@ -2928,6 +2928,7 @@ ofproto_rule_destroy__(struct rule *rule) cls_rule_destroy(CONST_CAST(struct cls_rule *, &rule->cr)); rule_actions_destroy(rule_get_actions(rule)); ovs_mutex_destroy(&rule->mutex); + ofproto_unref(rule->ofproto); rule->ofproto->ofproto_class->rule_dealloc(rule); } @@ -2979,6 +2980,28 @@ ofproto_rule_unref(struct rule *rule) } } +void ofproto_ref(struct ofproto *ofproto) +{ + if (ofproto) { + ovs_refcount_ref(&ofproto->ref_count); + } +} + +bool ofproto_try_ref(struct ofproto *ofproto) +{ + if (ofproto) { + return ovs_refcount_try_ref_rcu(&ofproto->ref_count); + } + return false; +} + +void ofproto_unref(struct ofproto *ofproto) +{ + if (ofproto && ovs_refcount_unref_relaxed(&ofproto->ref_count) == 1) { + ovsrcu_postpone(ofproto_destroy_defer__, ofproto); + } +} + static void remove_rule_rcu__(struct rule *rule) OVS_REQUIRES(ofproto_mutex) @@ -3068,6 +3091,7 @@ group_destroy_cb(struct ofgroup *group) &group->props)); ofputil_bucket_list_destroy(CONST_CAST(struct ovs_list *, &group->buckets)); + ofproto_unref(group->ofproto); group->ofproto->ofproto_class->group_dealloc(group); } @@ -5266,6 +5290,7 @@ ofproto_rule_create(struct ofproto *ofproto, struct cls_rule *cr, /* Initialize base state. */ *CONST_CAST(struct ofproto **, &rule->ofproto) = ofproto; + ofproto_ref(ofproto); cls_rule_move(CONST_CAST(struct cls_rule *, &rule->cr), cr); ovs_refcount_init(&rule->ref_count); @@ -6214,7 +6239,7 @@ handle_flow_mod__(struct ofproto *ofproto, const struct ofputil_flow_mod *fm, error = ofproto_flow_mod_start(ofproto, &ofm); if (!error) { ofproto_bump_tables_version(ofproto); - error = ofproto_flow_mod_finish(ofproto, &ofm, req); + error = ofproto_flow_mod_finish(ofproto, &ofm, req); ofmonitor_flush(ofproto->connmgr); } ovs_mutex_unlock(&ofproto_mutex); @@ -7331,6 +7356,7 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, } *CONST_CAST(struct ofproto **, &(*ofgroup)->ofproto) = ofproto; + ofproto_ref(ofproto); *CONST_CAST(uint32_t *, &((*ofgroup)->group_id)) = gm->group_id; *CONST_CAST(enum ofp11_group_type *, &(*ofgroup)->type) = gm->type; *CONST_CAST(long long int *, &((*ofgroup)->created)) = now; @@ -7363,6 +7389,7 @@ init_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm, ofputil_bucket_list_destroy(CONST_CAST(struct ovs_list *, &(*ofgroup)->buckets)); ofproto->ofproto_class->group_dealloc(*ofgroup); + ofproto_unref(ofproto); } return error; } @@ -8199,7 +8226,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id, uint16_t flags) /* Send error referring to the original message. */ ofconn_send_error(ofconn, be->msg, error); error = OFPERR_OFPBFC_MSG_FAILED; - + /* 2. Revert. Undo all the changes made above. */ LIST_FOR_EACH_REVERSE_CONTINUE(be, node, &bundle->msg_list) { if (be->type == OFPTYPE_FLOW_MOD) {