From patchwork Wed Mar 13 08:08:25 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darrell Ball X-Patchwork-Id: 1055951 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.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.b="UoRW8jg0"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 44K4Hb3Cvwz9s3q for ; Wed, 13 Mar 2019 19:09:15 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id C5097C6B; Wed, 13 Mar 2019 08:08:41 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 36234AF7 for ; Wed, 13 Mar 2019 08:08:40 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pf1-f193.google.com (mail-pf1-f193.google.com [209.85.210.193]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id DAC8582B for ; Wed, 13 Mar 2019 08:08:38 +0000 (UTC) Received: by mail-pf1-f193.google.com with SMTP id i20so825276pfo.6 for ; Wed, 13 Mar 2019 01:08:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:subject:date:message-id; bh=Ts8RnGB/KCnTWeD02+vBB97qGZmTxAcbXqbHrMV0tbo=; b=UoRW8jg05fSWYY8dxN3xgrCYvgFjExyiQyVmHoi9sH0Xc6nHRwXZwXRxcIZwcjKRov 2w/A92rBmvaYFaqguvlKo4tFMayXaeHl33Hjhp+Yn0BUFB4JRKRd0kjF1O9mP53PqxVp 8C2EgT5YBx+SN+dqoPXR7X/rb0AdfW8Dlok8J8nQEtbuSnt2Ok4B2QRlNdCjNuq6PwTY AX7QuwF9yWtyAdbgnAhjT1wtswZmqMnC20syD5gHypvs64+dFEtMjzfEs2PJOJC5AV0w C6PSPisgUFtOQL9exb8+c8+FAJriRz2uFI1d7FptbHOyuuSCvGZkHW/6zjdhf9lXpJmP 5b3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id; bh=Ts8RnGB/KCnTWeD02+vBB97qGZmTxAcbXqbHrMV0tbo=; b=a1z0HKcifdm+oXcWrISuRidRxd3FTGJBcdo/m/wwt3D4QDSkEVrZJ4rkfoWws3qb1M 1UoznnyHcxpzP9c8hY2EbIiVcwazhFp79UkRmq73GQawuYp913IRHZxIXr7MFtEPdbvp +oIKh75XN6ulAUuTVmfTwiN0HHXcPRAkC+Q427uUg8+TViRkC9dPf2N72PZfm/TVB3mr tJQxk+QlnQYQ2u37idiL3OxtdS5UEHlgLlJ2HO9jCUaMwjTlltIZKg+w9mjTyhO+uqFN W8L9HUt0yxGXmJ9/QAtVISYO24UA4m+cgQXtYT2gFXAyKRD+ftk/5tiwSl6hgMj+dmCi X4Wg== X-Gm-Message-State: APjAAAXTSqzeNbK/Bg8iiOnjNUD2Qx+6DTRH+nDFv9eYKAIlMDaOy3Fe twR96kWeCcsBwmUWVj54GNQ= X-Google-Smtp-Source: APXvYqwl5rHjj1M0ovwkVDbnSz+q/N2xCxifm998fbWo0ezTLbM67qrHzG0FHjfglpe04T/uMQgTqw== X-Received: by 2002:aa7:8a4c:: with SMTP id n12mr6273286pfa.127.1552464518338; Wed, 13 Mar 2019 01:08:38 -0700 (PDT) Received: from ubuntu.localdomain (c-76-102-76-212.hsd1.ca.comcast.net. [76.102.76.212]) by smtp.gmail.com with ESMTPSA id e63sm15461495pfa.116.2019.03.13.01.08.36 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 13 Mar 2019 01:08:37 -0700 (PDT) From: Darrell Ball To: dlu998@gmail.com, dev@openvswitch.org Date: Wed, 13 Mar 2019 01:08:25 -0700 Message-Id: <1552464506-35968-1-git-send-email-dlu998@gmail.com> X-Mailer: git-send-email 1.9.1 X-Spam-Status: No, score=-1.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE autolearn=no version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [patch v1 1/2] conntrack: Fix race for NAT cleanup. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Reference lists are not fully protected during cleanup of NAT connections where the bucket lock is transiently not held during list traversal. This can lead to referencing freed memory during cleaning from multiple contexts. Fix this by protecting with the existing 'cleanup' mutex in the missed cases where 'conn_clean()' is called. 'conntrack_flush()' is converted to expiry list traversal to support the proper bucket level protection with the 'cleanup' mutex. Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") Reported-by: solomon Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2019-March/357056.html Tested-by: solomon Signed-off-by: Darrell Ball --- lib/conntrack.c | 96 +++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 26 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 691782c..40a6021 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -753,6 +753,24 @@ conn_lookup(struct conntrack *ct, const struct conn_key *key, long long now) return ctx.conn; } +/* This function is called with the bucket lock held. */ +static struct conn * +conn_lookup_any(const struct conn_key *key, + const struct conntrack_bucket *ctb, uint32_t hash) +{ + struct conn *conn = NULL; + + HMAP_FOR_EACH_WITH_HASH (conn, node, hash, &ctb->connections) { + if (!conn_key_cmp(&conn->key, key)) { + break; + } + if (!conn_key_cmp(&conn->rev_key, key)) { + break; + } + } + return conn; +} + static void conn_seq_skew_set(struct conntrack *ct, const struct conn_key *key, long long now, int seq_skew, bool seq_skew_dir) @@ -823,6 +841,20 @@ conn_clean(struct conntrack *ct, struct conn *conn, } } +static void +conn_clean_safe(struct conntrack *ct, struct conn *conn, + struct conntrack_bucket *ctb, uint32_t hash) +{ + ovs_mutex_lock(&ctb->cleanup_mutex); + ct_lock_lock(&ctb->lock); + conn = conn_lookup_any(&conn->key, ctb, hash); + if (conn) { + conn_clean(ct, conn, ctb); + } + ct_lock_unlock(&ctb->lock); + ovs_mutex_unlock(&ctb->cleanup_mutex); +} + static bool ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl) { @@ -969,6 +1001,7 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, OVS_REQUIRES(ct->buckets[bucket].lock) { bool create_new_conn = false; + struct conn lconn; if (ctx->icmp_related) { pkt->md.ct_state |= CS_RELATED; @@ -995,7 +1028,10 @@ conn_update_state(struct conntrack *ct, struct dp_packet *pkt, pkt->md.ct_state = CS_INVALID; break; case CT_UPDATE_NEW: - conn_clean(ct, *conn, &ct->buckets[bucket]); + memcpy(&lconn, *conn, sizeof lconn); + ct_lock_unlock(&ct->buckets[bucket].lock); + conn_clean_safe(ct, &lconn, &ct->buckets[bucket], ctx->hash); + ct_lock_lock(&ct->buckets[bucket].lock); create_new_conn = true; break; default: @@ -1184,8 +1220,12 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, conn = ctx->conn; /* Delete found entry if in wrong direction. 'force' implies commit. */ - if (conn && force && ctx->reply) { - conn_clean(ct, conn, &ct->buckets[bucket]); + if (OVS_LIKELY(force && ctx->reply && conn)) { + struct conn lconn; + memcpy(&lconn, conn, sizeof lconn); + ct_lock_unlock(&ct->buckets[bucket].lock); + conn_clean_safe(ct, &lconn, &ct->buckets[bucket], ctx->hash); + ct_lock_lock(&ct->buckets[bucket].lock); conn = NULL; } @@ -1391,19 +1431,17 @@ sweep_bucket(struct conntrack *ct, struct conntrack_bucket *ctb, for (unsigned i = 0; i < N_CT_TM; i++) { LIST_FOR_EACH_SAFE (conn, next, exp_node, &ctb->exp_lists[i]) { - if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { - if (!conn_expired(conn, now) || count >= limit) { - min_expiration = MIN(min_expiration, conn->expiration); - if (count >= limit) { - /* Do not check other lists. */ - COVERAGE_INC(conntrack_long_cleanup); - return min_expiration; - } - break; + if (!conn_expired(conn, now) || count >= limit) { + min_expiration = MIN(min_expiration, conn->expiration); + if (count >= limit) { + /* Do not check other lists. */ + COVERAGE_INC(conntrack_long_cleanup); + return min_expiration; } - conn_clean(ct, conn, ctb); - count++; + break; } + conn_clean(ct, conn, ctb); + count++; } } return min_expiration; @@ -2547,16 +2585,19 @@ int conntrack_flush(struct conntrack *ct, const uint16_t *zone) { for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) { - struct conn *conn, *next; - - ct_lock_lock(&ct->buckets[i].lock); - HMAP_FOR_EACH_SAFE (conn, next, node, &ct->buckets[i].connections) { - if ((!zone || *zone == conn->key.zone) && - (conn->conn_type == CT_CONN_TYPE_DEFAULT)) { - conn_clean(ct, conn, &ct->buckets[i]); + struct conntrack_bucket *ctb = &ct->buckets[i]; + ovs_mutex_lock(&ctb->cleanup_mutex); + ct_lock_lock(&ctb->lock); + for (unsigned j = 0; j < N_CT_TM; j++) { + struct conn *conn, *next; + LIST_FOR_EACH_SAFE (conn, next, exp_node, &ctb->exp_lists[j]) { + if (!zone || *zone == conn->key.zone) { + conn_clean(ct, conn, ctb); + } } } - ct_lock_unlock(&ct->buckets[i].lock); + ct_lock_unlock(&ctb->lock); + ovs_mutex_unlock(&ctb->cleanup_mutex); } return 0; @@ -2573,16 +2614,19 @@ conntrack_flush_tuple(struct conntrack *ct, const struct ct_dpif_tuple *tuple, tuple_to_conn_key(tuple, zone, &ctx.key); ctx.hash = conn_key_hash(&ctx.key, ct->hash_basis); unsigned bucket = hash_to_bucket(ctx.hash); + struct conntrack_bucket *ctb = &ct->buckets[bucket]; - ct_lock_lock(&ct->buckets[bucket].lock); - conn_key_lookup(&ct->buckets[bucket], &ctx, time_msec()); + ovs_mutex_lock(&ctb->cleanup_mutex); + ct_lock_lock(&ctb->lock); + conn_key_lookup(ctb, &ctx, time_msec()); if (ctx.conn && ctx.conn->conn_type == CT_CONN_TYPE_DEFAULT) { - conn_clean(ct, ctx.conn, &ct->buckets[bucket]); + conn_clean(ct, ctx.conn, ctb); } else { VLOG_WARN("Must flush tuple using the original pre-NATed tuple"); error = ENOENT; } - ct_lock_unlock(&ct->buckets[bucket].lock); + ct_lock_unlock(&ctb->lock); + ovs_mutex_unlock(&ctb->cleanup_mutex); return error; }