From patchwork Fri Jul 17 01:50:41 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?Lui0uum5jw==?= X-Patchwork-Id: 1330681 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=bytedance.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=bytedance-com.20150623.gappssmtp.com header.i=@bytedance-com.20150623.gappssmtp.com header.a=rsa-sha256 header.s=20150623 header.b=rjjKQq+l; 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 4B7FHF6HSPz9sRR for ; Fri, 17 Jul 2020 12:21:29 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id EA98988313; Fri, 17 Jul 2020 02:21:27 +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 gjGVP93ayVhf; Fri, 17 Jul 2020 02:21:26 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id A772587E23; Fri, 17 Jul 2020 02:21:26 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 8CFE7C07FF; Fri, 17 Jul 2020 02:21:26 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 06ECFC0733 for ; Fri, 17 Jul 2020 02:21:23 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id DC10B203C7 for ; Fri, 17 Jul 2020 02:21:22 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EE4g50cXR5Cg for ; Fri, 17 Jul 2020 02:21:20 +0000 (UTC) X-Greylist: delayed 00:24:29 by SQLgrey-1.7.6 Received: from mail-oi1-f171.google.com (mail-oi1-f171.google.com [209.85.167.171]) by silver.osuosl.org (Postfix) with ESMTPS id 91792203C4 for ; Fri, 17 Jul 2020 02:21:20 +0000 (UTC) Received: by mail-oi1-f171.google.com with SMTP id h17so6801976oie.3 for ; Thu, 16 Jul 2020 19:21:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=IWKi10JYNrMp6F1yUtHAeeGBYaP2ySn+eYeGLIAfFpI=; b=rjjKQq+ld/uQX60uxekE2ECrwT0aS93GXSTodjPOVYcPlbrrDIIAs06wIFtqRfGB/s Roa6ZNo3T8KbJM4XtxHBpkFKa8ibC65GCUyvs5nW2o1iw+2MB11Fmlwh9YOyigtR+uL2 uXnwYIuTvVBgeuzqU8nHgn9OQZoI0vQ/meJ5GnLZKXO0EsT9VPzH5nk8T1wJ8QSDfNWz kr6c2l0et9O4tTp201Zb9/CUa999GWwhi29LD46lFZAIpbqI30geMjTu4Wie6RCCnJf0 V1VOhr6sOEVj1ifqDUs9o3NaLQZFTN8qUalKxLpzbYDR/U3VRE1LMwJhe7IvbjB8l1K0 PhAw== 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:mime-version :content-transfer-encoding; bh=IWKi10JYNrMp6F1yUtHAeeGBYaP2ySn+eYeGLIAfFpI=; b=YHcke18gJodF/jDtL1dzeCpbfn/Bpv49v8ph+SLqIedSFPRpbz0xSRzGF/GwRl8x8S xbnOAtyMx2/zTlONVI3Y8xFTnDhaaC8edtoiQbKuivTBd3ZCFSFv7Xfsb8hMJD2zmWvS DDbpNmC1LjdP/2yiV/YBRNxOM6XfRU1i7ZNrtpsdG9wgWvpDKCcFY7Y612ppYDd/X8tz qMplkBuZXi/HRfBwZjiGl+E622ZvSapAYIKcN6PLKc94FkyDSwWzr0Kfg4K2xSU7gFLU mX3aZimxSJzx03S2LyIiA3gRAWVLqSem5/SKeNmxnSK9DwzQYukqeeQPjwKEc9MJ+AwR IBog== X-Gm-Message-State: AOAM532Fp0nftG7AezUF5I2hgf5vYtJASyPRfr1rc2pRkiROHITdupuU I3KXAQzag+SPnjnJaih98njHkvaeK4cyjQ== X-Google-Smtp-Source: ABdhPJwehiSiRjvPzX9YijFwY8eUNoVUZDzk3jXBrKlWM8ZCQQA/om45z+XxaC4ksP6kZeb7a18MCw== X-Received: by 2002:a17:90a:8009:: with SMTP id b9mr8172431pjn.190.1594950647252; Thu, 16 Jul 2020 18:50:47 -0700 (PDT) Received: from localhost ([61.120.150.72]) by smtp.gmail.com with ESMTPSA id g5sm1088214pjl.31.2020.07.16.18.50.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 16 Jul 2020 18:50:45 -0700 (PDT) From: "hepeng.0320" To: dev@openvswitch.org Date: Fri, 17 Jul 2020 09:50:41 +0800 Message-Id: <20200717015041.82746-1-hepeng.0320@bytedance.com> X-Mailer: git-send-email 2.23.0 MIME-Version: 1.0 Cc: i.maximets@ovn.org Subject: [ovs-dev] [ovs-dev v2] ofproto: add refcount to ofproto to fix crash at ofproto_destroy 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" From: Peng He The call stack of rule_destroy_cb: remove_rules_postponed (one grace period) -> remove_rule_rcu -> remove_rule_rcu__ -> ofproto_rule_unref -> ref count != 1 -> ... more grace periods. -> rule_destroy_cb (> 2 grace periods) -> free Currently ofproto_destory waits only two grace periods, this means when rule_destroy_cb is called, the ofproto might have been already destroyed. This patch adds a refcount for ofproto to ensure ofproto exists when it is needed to call free functions. This patch fixes the crashes found: Program terminated with signal SIGSEGV, Segmentation fault. 0 0x0000562a55169e49 in rule_destroy_cb (rule=0x562a598be2c0) at ofproto/ofproto.c:2956 1 0x0000562a552623d6 in ovsrcu_call_postponed () at lib/ovs-rcu.c:348 2 0x0000562a55262504 in ovsrcu_postpone_thread (arg=) at lib/ovs-rcu.c:364 3 0x0000562a55264aef in ovsthread_wrapper (aux_=) at lib/ovs-thread.c:383 4 0x00007febe715a4a4 in start_thread (arg=0x7febe4c1c700) at pthread_create.c:456 5 0x00007febe6990d0f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97 (gdb) p rule->ofproto->ofproto_class $3 = (const struct ofproto_class *) 0x0 Also: 0 ofproto_dpif_credit_table_stats (ofproto=0x5583583332b0, table_id=0 '\000', n_matches=5, n_misses=0) at ofproto/ofproto-dpif.c:4310 1 0x0000558354c68514 in xlate_push_stats_entry (entry=entry@entry=0x7ff488031398, stats=stats@entry=0x7ff49af30b60) at ofproto/ofproto-dpif-xlate-cache.c:99 2 0x0000558354c686f3 in xlate_push_stats (xcache=, stats=stats@entry=0x7ff49af30b60) at ofproto/ofproto-dpif-xlate-cache.c:181 3 0x0000558354c5411a in revalidate_ukey (udpif=udpif@entry=0x5583583baba0, ukey=ukey@entry=0x7ff47809f770, stats=stats@entry=0x7ff49af32128, odp_actions=odp_actions@entry=0x7ff49af30c50, reval_seq=reval_seq@entry=275670456, recircs=recircs@entry=0x7ff49af30cd0) at ofproto/ofproto-dpif-upcall.c:2292 4 0x0000558354c57cbc in revalidate (revalidator=) at ofproto/ofproto-dpif-upcall.c:2681 5 0x0000558354c57f8e in udpif_revalidator (arg=0x5583583d2a90) at ofproto/ofproto-dpif-upcall.c:934 6 0x0000558354d24aef in ovsthread_wrapper (aux_=) at lib/ovs-thread.c:383 7 0x00007ff4a479d4a4 in start_thread (arg=0x7ff49af35700) at pthread_create.c:456 8 0x00007ff4a3fd3d0f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:97 (gdb) p ofproto->up.ofproto_class $3 = (const struct ofproto_class *) 0x0 Signed-off-by: Peng He --- ofproto/ofproto-dpif-xlate-cache.c | 1 + ofproto/ofproto-dpif.c | 22 ++++++++-------- ofproto/ofproto-provider.h | 2 ++ ofproto/ofproto.c | 41 +++++++++++++++++++++++++++--- ofproto/ofproto.h | 4 +++ 5 files changed, 56 insertions(+), 14 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 4f0638f23..6480b3c78 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4414,11 +4414,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; } @@ -4450,11 +4451,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 afecb24cb..34074fd62 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 refcount; }; void ofproto_init_tables(struct ofproto *, int n_tables); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 59f06aa94..0842c76c1 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -474,6 +474,24 @@ ofproto_bump_tables_version(struct ofproto *ofproto) ofproto->tables_version); } +void +ofproto_ref(struct ofproto *ofproto) +{ + ovs_refcount_ref(&ofproto->refcount); +} + +bool +ofproto_try_ref(struct ofproto *ofproto) +{ + return ovs_refcount_try_ref_rcu(&ofproto->refcount); +} + +void +ofproto_unref(struct ofproto *ofproto) +{ + ovs_refcount_unref(&ofproto->refcount); +} + int ofproto_create(const char *datapath_name, const char *datapath_type, struct ofproto **ofprotop) @@ -545,6 +563,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->refcount); error = ofproto->ofproto_class->construct(ofproto); if (error) { @@ -1696,14 +1715,26 @@ ofproto_destroy__(struct ofproto *ofproto) ofproto->ofproto_class->dealloc(ofproto); } -/* Destroying rules is doubly deferred, must have 'ofproto' around for them. - * - 1st we defer the removal of the rules from the classifier - * - 2nd we defer the actual destruction of the rules. */ +/* destroying a rule may have to wait multiple grace periods: + * remove_rules_postponed (one grace period) + * -> remove_rule_rcu + * -> remove_rule_rcu__ + * -> ofproto_rule_unref -> ref count != 1 + * -> ... more grace periods. + * -> rule_destroy_cb (> 2 grace periods) + * -> free + * + * So we have to check the refcount for sure all the rules + * have been destoryed. + */ static void ofproto_destroy_defer__(struct ofproto *ofproto) OVS_EXCLUDED(ofproto_mutex) { - ovsrcu_postpone(ofproto_destroy__, ofproto); + if (ovs_refcount_read(&ofproto->refcount) != 1) + ovsrcu_postpone(ofproto_destroy_defer__, ofproto); + else + ovsrcu_postpone(ofproto_destroy__, ofproto); } void @@ -2927,6 +2958,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); } @@ -5265,6 +5297,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); diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 2dd253167..3d3937b35 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -562,6 +562,10 @@ int ofproto_port_get_cfm_status(const struct ofproto *, enum ofputil_table_miss ofproto_table_get_miss_config(const struct ofproto *, uint8_t table_id); +void ofproto_ref(struct ofproto *p); +void ofproto_unref(struct ofproto *p); +bool ofproto_try_ref(struct ofproto *p); + #ifdef __cplusplus } #endif