From patchwork Thu Feb 23 21:31:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andy Zhou X-Patchwork-Id: 731763 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 3vTnVx2R6Lz9s2G for ; Fri, 24 Feb 2017 08:31:53 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 5DE4BC1A; Thu, 23 Feb 2017 21:31:51 +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 105E6C17 for ; Thu, 23 Feb 2017 21:31:50 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-pg0-f67.google.com (mail-pg0-f67.google.com [74.125.83.67]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id DD2F6168 for ; Thu, 23 Feb 2017 21:31:48 +0000 (UTC) Received: by mail-pg0-f67.google.com with SMTP id 5so395846pgj.0 for ; Thu, 23 Feb 2017 13:31:48 -0800 (PST) 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 :content-transfer-encoding; bh=We8SSGr/+jQSHzUwCPgyu6FbPqAnf9eBe2Fqa3lRWck=; b=LTy8eh56JATZTXFFKvDgdsCPIdEsbMSFTsYSIW1OLEJWBaKrshTh9vrSW4daAav0Qw bClbLWfgDsDNIgDq89t8w6GrYGBoHoNyHSqpqgLp4pfWvsmp/DmSWdK3nPIpPywN9dIl GX0SHw6yMRAC8qZ9WiUHKtaOIeVuFzJxF2fJsnstbjNBDmBtFd6OxppwQnAjqZJ3/nAs zN422RooPVnpviutvt/qZmbNaabKKmMXp5c7tbz21scNBgggr5OaHNfIN4Kj/oq6sBw3 KYUbqQcqLzimyV4o7vCfK3TGc4tWa/xKGOsuwkI/Gm2SO6FdSP2BZhclh+f798geh3rM FDdw== X-Gm-Message-State: AMke39m/QcAKRLuZtMDbY+nMbMEIeVEoq+owfyTA7YA23RN1r0jS1ryUMFHHci044OBgsg== X-Received: by 10.99.178.89 with SMTP id t25mr49918070pgo.183.1487885508226; Thu, 23 Feb 2017 13:31:48 -0800 (PST) Received: from ubuntu.localdomain ([208.91.1.34]) by smtp.gmail.com with ESMTPSA id l25sm11407706pfg.134.2017.02.23.13.31.47 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 23 Feb 2017 13:31:47 -0800 (PST) From: Andy Zhou To: dev@openvswitch.org Date: Thu, 23 Feb 2017 13:31:40 -0800 Message-Id: <1487885501-105263-1-git-send-email-azhou@ovn.org> X-Mailer: git-send-email 1.9.1 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Huanle Han Subject: [ovs-dev] [PATCH 1/2] ofproto/bond: Fix bond reconfiguration race condition. 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 During the upcall thread bond output translation, bond_may_recirc() is currently called outside the lock. In case the main thread executes bond_reconfigure() at the same time, the upcall thread may find bond state to be inconsistent when calling bond_update_post_recirc_rules(). This patch fixes the race condition by acquiring the write lock before calling bond_may_recirc(). The APIs are refactored slightly. The race condition can result in the following stack trace. Copied from 'Reported-at': Thread 23 handler69: Invalid write of size 8 update_recirc_rules (bond.c:385) bond_update_post_recirc_rules__ (bond.c:952) bond_update_post_recirc_rules (bond.c:960) output_normal (ofproto-dpif-xlate.c:2102) xlate_normal (ofproto-dpif-xlate.c:2858) xlate_output_action (ofproto-dpif-xlate.c:4407) do_xlate_actions (ofproto-dpif-xlate.c:5335) xlate_actions (ofproto-dpif-xlate.c:6198) upcall_xlate (ofproto-dpif-upcall.c:1129) process_upcall (ofproto-dpif-upcall.c:1271) recv_upcalls (ofproto-dpif-upcall.c:822) udpif_upcall_handler (ofproto-dpif-upcall.c:740) Address 0x18630490 is 1,904 bytes inside a block of size 12,288 free'd free (vg_replace_malloc.c:529) bond_entry_reset (bond.c:1635) bond_reconfigure (bond.c:457) bundle_set (ofproto-dpif.c:2896) ofproto_bundle_register (ofproto.c:1343) port_configure (bridge.c:1159) bridge_reconfigure (bridge.c:785) bridge_run (bridge.c:3099) main (ovs-vswitchd.c:111) Block was alloc'd at malloc (vg_replace_malloc.c:298) xmalloc (util.c:110) bond_entry_reset (bond.c:1629) bond_reconfigure (bond.c:457) bond_create (bond.c:245) bundle_set (ofproto-dpif.c:2900) ofproto_bundle_register (ofproto.c:1343) port_configure (bridge.c:1159) bridge_reconfigure (bridge.c:785) bridge_run (bridge.c:3099) main (ovs-vswitchd.c:111) Reported-by: Huanle Han Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-February/328969.html CC: Huanle Han Signed-off-by: Andy Zhou Acked-by: Jarno Rajahalme --- ofproto/bond.c | 27 +++++++++++++++------------ ofproto/bond.h | 3 ++- ofproto/ofproto-dpif-xlate.c | 24 ++++++++++++++++-------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/ofproto/bond.c b/ofproto/bond.c index 260023e4bb64..6e10c5143c0e 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -916,17 +916,16 @@ bool bond_may_recirc(const struct bond *bond, uint32_t *recirc_id, uint32_t *hash_bias) { - if (bond->balance == BM_TCP && bond->recirc_id) { - if (recirc_id) { - *recirc_id = bond->recirc_id; - } - if (hash_bias) { - *hash_bias = bond->basis; - } - return true; - } else { - return false; + bool may_recirc = bond->balance == BM_TCP && bond->recirc_id; + + if (recirc_id) { + *recirc_id = may_recirc ? bond->recirc_id : 0; } + if (hash_bias) { + *hash_bias = may_recirc ? bond->basis : 0; + } + + return may_recirc; } static void @@ -954,12 +953,16 @@ bond_update_post_recirc_rules__(struct bond* bond, const bool force) } void -bond_update_post_recirc_rules(struct bond* bond, const bool force) +bond_update_post_recirc_rules(struct bond *bond, uint32_t *recirc_id, + uint32_t *hash_basis) { ovs_rwlock_wrlock(&rwlock); - bond_update_post_recirc_rules__(bond, force); + if (bond_may_recirc(bond, recirc_id, hash_basis)) { + bond_update_post_recirc_rules__(bond, false); + } ovs_rwlock_unlock(&rwlock); } + /* Rebalancing. */ diff --git a/ofproto/bond.h b/ofproto/bond.h index 9a5ea9e21040..6e1221d2381b 100644 --- a/ofproto/bond.h +++ b/ofproto/bond.h @@ -120,7 +120,8 @@ void bond_rebalance(struct bond *); * Bond module pulls stats from those post recirculation rules. If rebalancing * is needed, those rules are updated with new output actions. */ -void bond_update_post_recirc_rules(struct bond *, const bool force); +void bond_update_post_recirc_rules(struct bond *, uint32_t *recirc_id, + uint32_t *hash_basis); bool bond_may_recirc(const struct bond *, uint32_t *recirc_id, uint32_t *hash_bias); #endif /* bond.h */ diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 503a347fc98f..89fc3a44a0d1 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1987,15 +1987,23 @@ output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle, struct flow_wildcards *wc = ctx->wc; struct ofport_dpif *ofport; - if (ctx->xbridge->support.odp.recirc) { - use_recirc = bond_may_recirc( - out_xbundle->bond, &xr.recirc_id, &xr.hash_basis); - - if (use_recirc) { - /* Only TCP mode uses recirculation. */ + if (ctx->xbridge->support.odp.recirc + && bond_may_recirc(out_xbundle->bond, NULL, NULL)) { + /* To avoid unnecessary locking, bond_may_recirc() is first + * called outside of the 'rwlock'. After acquiring the lock, + * bond_update_post_recirc_rules() will check again to make + * sure bond configuration has not been changed. + * + * In case recirculation is not actually in use, 'xr.recirc_id' + * will be set to '0', Since a valid 'recirc_id' can + * not be zero. */ + bond_update_post_recirc_rules(out_xbundle->bond, + &xr.recirc_id, + &xr.hash_basis); + if (xr.recirc_id) { + /* Use recirculation instead of output. */ + use_recirc = true; xr.hash_alg = OVS_HASH_ALG_L4; - bond_update_post_recirc_rules(out_xbundle->bond, false); - /* Recirculation does not require unmasking hash fields. */ wc = NULL; }