From patchwork Thu Mar 14 19:00:04 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darrell Ball X-Patchwork-Id: 1056664 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="QCVS6p7P"; 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 44KyhS1LvFz9s6w for ; Fri, 15 Mar 2019 06:00:23 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 49D72C96; Thu, 14 Mar 2019 19:00:21 +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 68F23B3E for ; Thu, 14 Mar 2019 19:00:19 +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 9479F8AA for ; Thu, 14 Mar 2019 19:00:18 +0000 (UTC) Received: by mail-pf1-f193.google.com with SMTP id n125so4471874pfn.5 for ; Thu, 14 Mar 2019 12:00:18 -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=QbfGTdEKGb43ELFQB8wNH1Mea1ni4w3+CthP3onnmxQ=; b=QCVS6p7PqxQ2eChxn3dEXRzmX+N6e6vcogxuf6W2SDnE3s2gXaRChFEnVBzH92FLj9 NCqTW/tF/ygDVVoxpOS4HThTidF5w0DZlltOsRtov7uB0pT82npolgd7PJM466ueOAtj 3jeEooVwYestcOnrZewBv/l5WQ9lpabHrk4CHAfrR4h7CoaQmThtyxAVLUktsqbpA6ui eOyz3njcfvUiCwEInxw9fH6TybzM5WWiCydT8yO3SP2TuhxaJlUjmNF07A0oKrEiqPkf efaNZrNDauI1uDVfnhuUOK87TNFzuDlB1l4ijEuLkYpsYZVcSYxcntLqXSB6hJLHqRo3 bq+Q== 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=QbfGTdEKGb43ELFQB8wNH1Mea1ni4w3+CthP3onnmxQ=; b=qHs1bi/2xN1ylAbgqEx4NyhQqeRH+763u/UXPDNMKpUO1ccaVu6c9NT+RjCuIECWSh xqaKseiW5BHU3ZPKm7uPRG5AVDA9RiLNxopOPz8p4js7sYffhCmiasd7A4Ljm12ixOi/ bwmStEk0cUcc/gltxA2md8deArw8LB7u5gjpkFwG1O9gfPHV6kOGeFpMFsnOwnYdgkyR aCT/tE1dVt+V1WIQETIJHxV4e315RIYgucPBimxWPXD5ZnKUIzChqqUhDugpF1W4sMoH rVOKqRbLVNShxfSuzCPLzpbgjVUUH6kk0RlDwPwXam72PBqcxCaYKsPtZl6cjWgFIlVM UMeQ== X-Gm-Message-State: APjAAAVjbL56UXpaWTeFmd8ban9d3fRy/kXv567ITt79YjfglbRF2ccN hPiKK11ahgTGEJvDuE+cZr4= X-Google-Smtp-Source: APXvYqxbCWCsDfFzyTsJfiN020KaviOkPhcf9y+9McF2x5uSEifjLZMFbUjlX5PEI6SmV1uH/PDwNg== X-Received: by 2002:a63:6c43:: with SMTP id h64mr44655121pgc.22.1552590017260; Thu, 14 Mar 2019 12:00:17 -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 m20sm40831376pfj.142.2019.03.14.12.00.14 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 14 Mar 2019 12:00:16 -0700 (PDT) From: Darrell Ball To: dlu998@gmail.com, dev@openvswitch.org Date: Thu, 14 Mar 2019 12:00:04 -0700 Message-Id: <1552590006-69679-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 v5 1/3] 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. The NAT exhaustion case cleanup in 'conn_not_found()' is also modified to avoid the same issue. 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 --- This patch is targeted for earlier releases as new RCU patches inherently don't have this race. v5: Fix recent version compiler warning (Ilya) about local variable going out of scope. I don't think stack memory is reclaimed when block scope ends, but theoretically some compiler could do it. Moved "structure copy -> memcpy" changes into a new patch 3. Add function comment to conn_clean_safe(). v4: Fix exhaustion case cleanup race in conn_not_found() as well. At the same time, simplify the related code. v3: Use cleanup_mutex in conntrack_destroy(). v2: Fix typo. lib/conntrack.c | 138 ++++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 94 insertions(+), 44 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 691782c..48cd232 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -355,7 +355,7 @@ conntrack_destroy(struct conntrack *ct) struct conntrack_bucket *ctb = &ct->buckets[i]; struct conn *conn; - ovs_mutex_destroy(&ctb->cleanup_mutex); + ovs_mutex_lock(&ctb->cleanup_mutex); ct_lock_lock(&ctb->lock); HMAP_FOR_EACH_POP (conn, node, &ctb->connections) { if (conn->conn_type == CT_CONN_TYPE_DEFAULT) { @@ -365,7 +365,9 @@ conntrack_destroy(struct conntrack *ct) } hmap_destroy(&ctb->connections); ct_lock_unlock(&ctb->lock); + ovs_mutex_unlock(&ctb->cleanup_mutex); ct_lock_destroy(&ctb->lock); + ovs_mutex_destroy(&ctb->cleanup_mutex); } ct_rwlock_wrlock(&ct->resources_lock); struct nat_conn_key_node *nat_conn_key_node; @@ -753,6 +755,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 +843,21 @@ conn_clean(struct conntrack *ct, struct conn *conn, } } +/* Must be called with no locks held and upon return no locks are held. */ +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) { @@ -854,6 +889,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, enum ct_alg_ctl_type ct_alg_ctl) { struct conn *nc = NULL; + struct conn connl; if (!valid_new(pkt, &ctx->key)) { pkt->md.ct_state = CS_INVALID; @@ -876,8 +912,9 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, } unsigned bucket = hash_to_bucket(ctx->hash); - nc = new_conn(&ct->buckets[bucket], pkt, &ctx->key, now); - ctx->conn = nc; + nc = &connl; + memset(nc, 0, sizeof *nc); + memcpy(&nc->key, &ctx->key, sizeof nc->key); nc->rev_key = nc->key; conn_key_reverse(&nc->rev_key); @@ -921,6 +958,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, ct_rwlock_wrlock(&ct->resources_lock); bool nat_res = nat_select_range_tuple(ct, nc, conn_for_un_nat_copy); + ct_rwlock_unlock(&ct->resources_lock); if (!nat_res) { goto nat_res_exhaustion; @@ -929,14 +967,24 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, /* Update nc with nat adjustments made to * conn_for_un_nat_copy by nat_select_range_tuple(). */ *nc = *conn_for_un_nat_copy; - ct_rwlock_unlock(&ct->resources_lock); } conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT; conn_for_un_nat_copy->nat_info = NULL; conn_for_un_nat_copy->alg = NULL; nat_packet(pkt, nc, ctx->icmp_related); } - hmap_insert(&ct->buckets[bucket].connections, &nc->node, ctx->hash); + struct conn *nconn = new_conn(&ct->buckets[bucket], pkt, &ctx->key, + now); + memcpy(&nconn->key, &nc->key, sizeof nconn->key); + memcpy(&nconn->rev_key, &nc->rev_key, sizeof nconn->rev_key); + memcpy(&nconn->master_key, &nc->master_key, sizeof nconn->master_key); + nconn->alg_related = nc->alg_related; + nconn->alg = nc->alg; + nconn->mark = nc->mark; + nconn->label = nc->label; + nconn->nat_info = nc->nat_info; + ctx->conn = nc = nconn; + hmap_insert(&ct->buckets[bucket].connections, &nconn->node, ctx->hash); atomic_count_inc(&ct->n_conn); } @@ -949,13 +997,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, * against with firewall rules or a separate firewall. * Also using zone partitioning can limit DoS impact. */ nat_res_exhaustion: - ovs_list_remove(&nc->exp_node); - delete_conn(nc); - /* conn_for_un_nat_copy is a local variable in process_one; this - * memset() serves to document that conn_for_un_nat_copy is from - * this point on unused. */ - memset(conn_for_un_nat_copy, 0, sizeof *conn_for_un_nat_copy); - ct_rwlock_unlock(&ct->resources_lock); + free(nc->alg); + free(nc->nat_info); static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); VLOG_WARN_RL(&rl, "Unable to NAT due to tuple space exhaustion - " "if DoS attack, use firewalling and/or zone partitioning."); @@ -969,6 +1012,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 +1039,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 +1231,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_UNLIKELY(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 +1442,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; @@ -2344,12 +2393,7 @@ static struct conn * new_conn(struct conntrack_bucket *ctb, struct dp_packet *pkt, struct conn_key *key, long long now) { - struct conn *newconn = l4_protos[key->nw_proto]->new_conn(ctb, pkt, now); - if (newconn) { - newconn->key = *key; - } - - return newconn; + return l4_protos[key->nw_proto]->new_conn(ctb, pkt, now); } static void @@ -2547,16 +2591,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 +2620,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; } From patchwork Thu Mar 14 19:00:05 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darrell Ball X-Patchwork-Id: 1056665 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="HJvlFup3"; 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 44Kyj060Wfz9s3q for ; Fri, 15 Mar 2019 06:00:52 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 00CDECAC; Thu, 14 Mar 2019 19:00:22 +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 27373B3E for ; Thu, 14 Mar 2019 19:00:20 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pg1-f194.google.com (mail-pg1-f194.google.com [209.85.215.194]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 1A5E98C0 for ; Thu, 14 Mar 2019 19:00:19 +0000 (UTC) Received: by mail-pg1-f194.google.com with SMTP id h34so4593455pgh.11 for ; Thu, 14 Mar 2019 12:00:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:subject:date:message-id:in-reply-to:references; bh=eQ+vzIdTHEyS6INP5G23s/FdJNavI+GppmcJHdrgzpw=; b=HJvlFup3OEmN2CrPKmU8YvG4ZfVKdPy+sy/kQhaJTt3T90UCyx2p+yl2E1Wh0jHP2X TJJLX7M/nSCKioUZ30B9Pg1jxSwtw9UpZg+txb8Z5E/cRTJemhhsQYqc/9yB1deH9/fx uGNHj0AhlDQRk9g8KE9eKlCgMtu2JOEh1kHL2r55eK8ZOVqOFw8uj8Me6kw3CKsP2cR3 yXCO84te5LxnQVnAVoN2DWGWKYkHh3sz6iM1/Tk3gajWvsykmck1dsunG6eWRyDnabvS kOBa0m9rjvuCtA/4VGQoXxZ/AhWDjuMlgMW7eNt006EwERQBhIJ5htngkoR4bXqy+kH6 +SaA== 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:in-reply-to :references; bh=eQ+vzIdTHEyS6INP5G23s/FdJNavI+GppmcJHdrgzpw=; b=lnAqoCHC+E+CSBvb1gP2El+/3ZHghogUUFYn/w7euXj0nxUXJjrOEPS2EeC3J0qEgx oyX+x1AI2soAo0+AnlC3608l8gH1xos2Fz8h0PpgShiTZotiu7ubXoGGifsf3uJJR3J2 yIjXSNiteA9E1EkIPClFys6/uW9JTnGnH30Uj8ZFxL+j5eQZacOIGYW/i4rLOBFfANyS 6MCPHCZxaxTuyLd1w3K8qdkqvjV0IVFllSdE17mJtdSoUpsLFdszwIDMariP5+XwLtin 0tSKHebHa27RvAraELYWpNkbDxt4eK/xT/foujfAwRXwk7+6bBjWUzxnoovk1bAMFKeg 6jdg== X-Gm-Message-State: APjAAAWaO8+9bYo6WI/i8XiHIITNF0HA2biCj60Z1L7SwhsshH7FLUjQ nefPVRTFlaza19bkWIosCss= X-Google-Smtp-Source: APXvYqwytFvK2FeKYd6/fQS7LXwnMJY6LWXm8vY9YC4Lq+u39iggS6N3fiPqwaejWO/EDPTwGbhoJQ== X-Received: by 2002:a17:902:b60c:: with SMTP id b12mr52438912pls.261.1552590018648; Thu, 14 Mar 2019 12:00:18 -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 m20sm40831376pfj.142.2019.03.14.12.00.17 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 14 Mar 2019 12:00:18 -0700 (PDT) From: Darrell Ball To: dlu998@gmail.com, dev@openvswitch.org Date: Thu, 14 Mar 2019 12:00:05 -0700 Message-Id: <1552590006-69679-2-git-send-email-dlu998@gmail.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1552590006-69679-1-git-send-email-dlu998@gmail.com> References: <1552590006-69679-1-git-send-email-dlu998@gmail.com> 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 v5 2/3] conntrack: Lookup only 'UNNAT conns' in 'nat_clean()'. 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 When freeing 'UNNAT conns', lookup only 'UNNAT conns' to protect against possible address overlap with 'default conns' during a DOS attempt. This is very unlikely, but protection is simple. Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") Signed-off-by: Darrell Ball --- v5: Add fixes tag. v1->v4: No changes to this patch. lib/conntrack.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 48cd232..008fd79 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -773,6 +773,22 @@ conn_lookup_any(const struct conn_key *key, return conn; } +/* This function is called with the bucket lock held. */ +static struct conn * +conn_lookup_unnat(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) + && conn->conn_type == CT_CONN_TYPE_UN_NAT) { + 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) @@ -796,12 +812,13 @@ nat_clean(struct conntrack *ct, struct conn *conn, nat_conn_keys_remove(&ct->nat_conn_keys, &conn->rev_key, ct->hash_basis); ct_rwlock_unlock(&ct->resources_lock); ct_lock_unlock(&ctb->lock); - unsigned bucket_rev_conn = - hash_to_bucket(conn_key_hash(&conn->rev_key, ct->hash_basis)); + uint32_t hash = conn_key_hash(&conn->rev_key, ct->hash_basis); + unsigned bucket_rev_conn = hash_to_bucket(hash); ct_lock_lock(&ct->buckets[bucket_rev_conn].lock); ct_rwlock_wrlock(&ct->resources_lock); - long long now = time_msec(); - struct conn *rev_conn = conn_lookup(ct, &conn->rev_key, now); + struct conn *rev_conn = conn_lookup_unnat(&conn->rev_key, + &ct->buckets[bucket_rev_conn], + hash); struct nat_conn_key_node *nat_conn_key_node = nat_conn_keys_lookup(&ct->nat_conn_keys, &conn->rev_key, ct->hash_basis); From patchwork Thu Mar 14 19:00:06 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Darrell Ball X-Patchwork-Id: 1056666 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="JJ+qKQ3s"; 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 44KyjX55plz9s3q for ; Fri, 15 Mar 2019 06:01:20 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 94A84CA1; Thu, 14 Mar 2019 19:00:22 +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 37EE6C90 for ; Thu, 14 Mar 2019 19:00:20 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pg1-f193.google.com (mail-pg1-f193.google.com [209.85.215.193]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id D4F6D8AA for ; Thu, 14 Mar 2019 19:00:19 +0000 (UTC) Received: by mail-pg1-f193.google.com with SMTP id h34so4593483pgh.11 for ; Thu, 14 Mar 2019 12:00:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:subject:date:message-id:in-reply-to:references; bh=qobcZhyfFXPrznUHf1F/9GseHmLomPvR7ZuQJDReO5w=; b=JJ+qKQ3sWYOH+vbaLttxXAak84bGPkPZtU3L7+HMSBa9ruSjrVRtwA50rO9iKF8bJ1 VnwmQBdLB5CkHpOvXoRgMyhwa+TG1qYUEYXKXTbKoC5SzAQGCLSB5cXMhGL7VFrMzv+s b7QaRIaJueZTJgIv+MVfv2Z6QLs7qunVznQXwGG6OVRqfH0AdvatB+Peg0MPg5rF6sQW PGxJXhH0OkvRyaN1rjfxR/ss9MU34mBvR005yujnOCXxQNEaVqoto3oqj0x/GP21nboK nTkMQc1Q8UwdgbUEmsfZJnoo/vHVCizVMr3+kkz12oP5JY1rwiRRKAG/HDxE5qcvbBRw 9//w== 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:in-reply-to :references; bh=qobcZhyfFXPrznUHf1F/9GseHmLomPvR7ZuQJDReO5w=; b=K2rUubuoyhuqNZCj+wlnSA4ZutoRugveIJjKkLq1v4d59sATcpsC2WNUqEORjMpFgO +w5HohonJ9EknFw//8Ae7YXxuayJCJ07I0H3MoP75gagtZ6qRzoZf68G3wJt+I8VjPTn 07XKuKndtYdRLXAjXcRS9a6DGBSDqj5ugMEdlGMrEiUOrkOwH42ze3eHPXYEzxzXuqYu vyBziEVzFVvAx2vffUxdBz9jXC//E5SsIKhUdvsYh4jLrSGJZM5KnND5UzmnDQUHRukY CkVyOlgtU2/Fio1P1iOQFeXOSgUnBufy8LmjrNlRwMnKU1AWxsPN9MrTW9BzXKbFzXE3 eXDw== X-Gm-Message-State: APjAAAUsEXFWwHfvMHOAKDGWPauHvYAMklyaEdZ8hSWptbPsqmS6Ie46 jc/UvOr/Ffg+VfYc5xM4tRY7BYLt X-Google-Smtp-Source: APXvYqyrY50b6m9TnAjFQgcc5QF9TykeQXp7INYtEIoIYd4DYcf5kvQZo9EOf0JFrVyyZUTkHIUyQg== X-Received: by 2002:a63:445e:: with SMTP id t30mr25445306pgk.27.1552590019425; Thu, 14 Mar 2019 12:00:19 -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 m20sm40831376pfj.142.2019.03.14.12.00.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 14 Mar 2019 12:00:18 -0700 (PDT) From: Darrell Ball To: dlu998@gmail.com, dev@openvswitch.org Date: Thu, 14 Mar 2019 12:00:06 -0700 Message-Id: <1552590006-69679-3-git-send-email-dlu998@gmail.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1552590006-69679-1-git-send-email-dlu998@gmail.com> References: <1552590006-69679-1-git-send-email-dlu998@gmail.com> 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 v5 3/3] conntrack: Replace structure copy by memcpy(). 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 There are a few cases where structure copy can be replaced by memcpy(), for possible portability benefit. This is because the structures involved have padding and elements of the structure are used to generate hashes. Signed-off-by: Darrell Ball --- v5: New patch and moved some changes from Patch 1 here. lib/conntrack.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/conntrack.c b/lib/conntrack.c index 008fd79..b5c9c07 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -932,7 +932,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, nc = &connl; memset(nc, 0, sizeof *nc); memcpy(&nc->key, &ctx->key, sizeof nc->key); - nc->rev_key = nc->key; + memcpy(&nc->rev_key, &nc->key, sizeof nc->rev_key); conn_key_reverse(&nc->rev_key); if (ct_verify_helper(helper, ct_alg_ctl)) { @@ -983,7 +983,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt, /* Update nc with nat adjustments made to * conn_for_un_nat_copy by nat_select_range_tuple(). */ - *nc = *conn_for_un_nat_copy; + memcpy(nc, conn_for_un_nat_copy, sizeof *nc); } conn_for_un_nat_copy->conn_type = CT_CONN_TYPE_UN_NAT; conn_for_un_nat_copy->nat_info = NULL; @@ -1074,8 +1074,8 @@ create_un_nat_conn(struct conntrack *ct, struct conn *conn_for_un_nat_copy, long long now, bool alg_un_nat) { struct conn *nc = xmemdup(conn_for_un_nat_copy, sizeof *nc); - nc->key = conn_for_un_nat_copy->rev_key; - nc->rev_key = conn_for_un_nat_copy->key; + memcpy(&nc->key, &conn_for_un_nat_copy->rev_key, sizeof nc->key); + memcpy(&nc->rev_key, &conn_for_un_nat_copy->key, sizeof nc->rev_key); uint32_t un_nat_hash = conn_key_hash(&nc->key, ct->hash_basis); unsigned un_nat_conn_bucket = hash_to_bucket(un_nat_hash); ct_lock_lock(&ct->buckets[un_nat_conn_bucket].lock); @@ -1264,7 +1264,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, struct conn_lookup_ctx ctx2; ctx2.conn = NULL; - ctx2.key = conn->rev_key; + memcpy(&ctx2.key, &conn->rev_key, sizeof ctx2.key); ctx2.hash = conn_key_hash(&conn->rev_key, ct->hash_basis); ct_lock_unlock(&ct->buckets[bucket].lock); @@ -1350,7 +1350,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, struct conn conn_for_expectation; if (OVS_UNLIKELY((ct_alg_ctl != CT_ALG_CTL_NONE) && conn)) { - conn_for_expectation = *conn; + memcpy(&conn_for_expectation, conn, sizeof conn_for_expectation); } ct_lock_unlock(&ct->buckets[bucket].lock); @@ -2330,8 +2330,10 @@ nat_conn_keys_insert(struct hmap *nat_conn_keys, const struct conn *nat_conn, if (!nat_conn_key_node) { struct nat_conn_key_node *nat_conn_key = xzalloc(sizeof *nat_conn_key); - nat_conn_key->key = nat_conn->rev_key; - nat_conn_key->value = nat_conn->key; + memcpy(&nat_conn_key->key, &nat_conn->rev_key, + sizeof nat_conn_key->key); + memcpy(&nat_conn_key->value, &nat_conn->key, + sizeof nat_conn_key->value); hmap_insert(nat_conn_keys, &nat_conn_key->node, conn_key_hash(&nat_conn_key->key, basis)); return true; @@ -2819,7 +2821,8 @@ expectation_create(struct conntrack *ct, ovs_be16 dst_port, alg_exp_node->key.dst.port = dst_port; alg_exp_node->master_mark = master_conn->mark; alg_exp_node->master_label = master_conn->label; - alg_exp_node->master_key = master_conn->key; + memcpy(&alg_exp_node->master_key, &master_conn->key, + sizeof alg_exp_node->master_key); /* Take the write lock here because it is almost 100% * likely that the lookup will fail and * expectation_create() will be called below. */