From patchwork Wed Oct 5 22:34:39 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Emilio Cota X-Patchwork-Id: 9363541 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 0A538600C8 for ; Wed, 5 Oct 2016 22:36:28 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id ED05C28D1A for ; Wed, 5 Oct 2016 22:36:27 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E13A328D21; Wed, 5 Oct 2016 22:36:27 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 2B8DD28D1A for ; Wed, 5 Oct 2016 22:36:27 +0000 (UTC) Received: from localhost ([::1]:51712 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brunZ-0007cA-4Y for patchwork-qemu-devel@patchwork.kernel.org; Wed, 05 Oct 2016 18:36:25 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51425) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brun2-0007bo-Sg for qemu-devel@nongnu.org; Wed, 05 Oct 2016 18:35:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1brumx-00067j-2M for qemu-devel@nongnu.org; Wed, 05 Oct 2016 18:35:51 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:39966) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1brumu-00062a-NF for qemu-devel@nongnu.org; Wed, 05 Oct 2016 18:35:46 -0400 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id A9C8920A85; Wed, 5 Oct 2016 18:35:35 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Wed, 05 Oct 2016 18:35:35 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=braap.org; h=cc :date:from:in-reply-to:message-id:references:subject:to :x-sasl-enc:x-sasl-enc; s=mesmtp; bh=Qrp9VS+pOxtk/R7PzjsFHOwC/NA =; b=uWIe11WsgiklvE3en2fpH/882QDZwC9e6MrkZaCzgHOL0M3W23x6gr+RR7g j9dzG6XHmX60DzBiSkoiei/CkZjVe6aIJrXwLIest+ZM/YqnadhBM16Qglg+g1pJ q2R+lx7s1ue2CXbbZpFFQ/dI3/Voy03/KKaIaubHNdTYOUfE= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:date:from:in-reply-to:message-id :references:subject:to:x-sasl-enc:x-sasl-enc; s=smtpout; bh=Qrp9 VS+pOxtk/R7PzjsFHOwC/NA=; b=Hcal1kWhm47JEW2PWD2Iwg8rYPxoN/aLItmF web5uwKSPCkUTSfrqVWGe/kq9mVWORwUKeIyUULiTdjwd+ewcB2UW0AIZQDzhpA2 k8gMXJ0zTw+GYMgAsgK4H6L7izFmI+ag6lfKPdq1TtAzAb9CAOWiw3aG7F3s6gpD bMDFMH0= X-Sasl-enc: qDBNchOob4o1gLeuRZdFwledwU3YAZZ1rTjCjYbSPqlx 1475706935 Received: from localhost (flamenco.cs.columbia.edu [128.59.20.216]) by mail.messagingengine.com (Postfix) with ESMTPA id 788C4F2D2A; Wed, 5 Oct 2016 18:35:35 -0400 (EDT) From: "Emilio G. Cota" To: Peter Maydell Date: Wed, 5 Oct 2016 18:34:39 -0400 Message-Id: <1475706880-10667-3-git-send-email-cota@braap.org> X-Mailer: git-send-email 2.7.4 In-Reply-To: <1475706880-10667-1-git-send-email-cota@braap.org> References: <1475706880-10667-1-git-send-email-cota@braap.org> X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 66.111.4.25 Subject: [Qemu-devel] [PATCH 2/3] qht: fix unlock-after-free segfault upon resizing X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: =?UTF-8?q?Alex=20Benn=C3=A9e?= , Paolo Bonzini , QEMU Developers , Richard Henderson Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP The old map's bucket locks are being unlocked *after* that same old map has been passed to RCU for destruction. This is a bug that can cause a segfault, since there's no guarantee that the deletion will be deferred (e.g. there may be no concurrent readers). The segfault is easily triggered in RHEL6/CentOS6 with qht-test, particularly on a single-core system or by pinning qht-test to a single core. Fix it by unlocking the map's bucket locks right after having published the new map, and (crucially) before marking the map for deletion via call_rcu(). While at it, expand qht_do_resize() to atomically do (1) a reset, (2) a resize, or (3) a reset+resize. This simplifies the calling code, since the new function (qht_do_resize_reset()) acquires and releases the buckets' locks. Note that no qht_do_reset inline is provided, since it would have no users--qht_reset() already performs a reset without taking ht->lock. Reported-by: Peter Maydell Reported-by: Daniel P. Berrange Signed-off-by: Emilio G. Cota --- util/qht.c | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/util/qht.c b/util/qht.c index af8da3c..6c61aca 100644 --- a/util/qht.c +++ b/util/qht.c @@ -133,7 +133,8 @@ struct qht_map { /* trigger a resize when n_added_buckets > n_buckets / div */ #define QHT_NR_ADDED_BUCKETS_THRESHOLD_DIV 8 -static void qht_do_resize(struct qht *ht, struct qht_map *new); +static void qht_do_resize_reset(struct qht *ht, struct qht_map *new, + bool reset); static void qht_grow_maybe(struct qht *ht); #ifdef QHT_DEBUG @@ -408,6 +409,16 @@ void qht_reset(struct qht *ht) qht_map_unlock_buckets(map); } +static inline void qht_do_resize(struct qht *ht, struct qht_map *new) +{ + qht_do_resize_reset(ht, new, false); +} + +static inline void qht_do_resize_and_reset(struct qht *ht, struct qht_map *new) +{ + qht_do_resize_reset(ht, new, true); +} + bool qht_reset_size(struct qht *ht, size_t n_elems) { struct qht_map *new = NULL; @@ -421,13 +432,7 @@ bool qht_reset_size(struct qht *ht, size_t n_elems) if (n_buckets != map->n_buckets) { new = qht_map_create(n_buckets); } - - qht_map_lock_buckets(map); - qht_map_reset__all_locked(map); - if (new) { - qht_do_resize(ht, new); - } - qht_map_unlock_buckets(map); + qht_do_resize_and_reset(ht, new); qemu_mutex_unlock(&ht->lock); return !!new; @@ -559,9 +564,7 @@ static __attribute__((noinline)) void qht_grow_maybe(struct qht *ht) if (qht_map_needs_resize(map)) { struct qht_map *new = qht_map_create(map->n_buckets * 2); - qht_map_lock_buckets(map); qht_do_resize(ht, new); - qht_map_unlock_buckets(map); } qemu_mutex_unlock(&ht->lock); } @@ -737,24 +740,31 @@ static void qht_map_copy(struct qht *ht, void *p, uint32_t hash, void *userp) } /* - * Call with ht->lock and all bucket locks held. - * - * Creating the @new map here would add unnecessary delay while all the locks - * are held--holding up the bucket locks is particularly bad, since no writes - * can occur while these are held. Thus, we let callers create the new map, - * hopefully without the bucket locks held. + * Atomically perform a resize and/or reset. + * Call with ht->lock held. */ -static void qht_do_resize(struct qht *ht, struct qht_map *new) +static void qht_do_resize_reset(struct qht *ht, struct qht_map *new, bool reset) { struct qht_map *old; old = ht->map; - g_assert_cmpuint(new->n_buckets, !=, old->n_buckets); + qht_map_lock_buckets(old); + if (reset) { + qht_map_reset__all_locked(old); + } + + if (new == NULL) { + qht_map_unlock_buckets(old); + return; + } + + g_assert_cmpuint(new->n_buckets, !=, old->n_buckets); qht_map_iter__all_locked(ht, old, qht_map_copy, new); qht_map_debug__all_locked(new); atomic_rcu_set(&ht->map, new); + qht_map_unlock_buckets(old); call_rcu(old, qht_map_destroy, rcu); } @@ -766,12 +776,9 @@ bool qht_resize(struct qht *ht, size_t n_elems) qemu_mutex_lock(&ht->lock); if (n_buckets != ht->map->n_buckets) { struct qht_map *new; - struct qht_map *old = ht->map; new = qht_map_create(n_buckets); - qht_map_lock_buckets(old); qht_do_resize(ht, new); - qht_map_unlock_buckets(old); ret = true; } qemu_mutex_unlock(&ht->lock);