From patchwork Wed Sep 9 11:58:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Jason A. Donenfeld" X-Patchwork-Id: 249458 Delivered-To: patch@linaro.org Received: by 2002:a92:5b9c:0:0:0:0:0 with SMTP id c28csp501562ilg; Wed, 9 Sep 2020 08:08:47 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzNCK4LEV2auIO5DpdgHO9qI+lcPmAYqMc+umh7BCTqo3EtwyKAeqGbpAsrjNM4Vwgfh5dB X-Received: by 2002:aa7:d6ce:: with SMTP id x14mr4587439edr.359.1599664126905; Wed, 09 Sep 2020 08:08:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1599664126; cv=none; d=google.com; s=arc-20160816; b=FZW4HaW2+gvKOhSZoJGK3O0sO9HlTGfZu18/cLTI9ZKvb90SQkHKUkCqiRR7DK25LP qiCjpoRsYkbWKPw6QCfIHBbEM+GXnWtdx0Q4PubZcuMx/UJLwzt/VlxHLgRbX+wcA56r 0relB+vHKo+1yhi049VZbn/OwxiVPOTMQ99iceXp7reZJyBVOz9wlEQSgegFpih4QMij n95iLfGpRfTnZFP2Pf2GjurU5E85QhSdhEoJghKTHaOy6E8yySmBRvV16o8Ikm4r1Djy b2a/B2z55XHaO23GCBZQz7h2r9im+9Fi0V2V37mdbDvVQE9QZBiHaSX/dxwDQK3wx6E5 eczQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=vhYEJ4RfuH1AgOSP5pYrgQpp8UEhpSdhTwJ9Sxva3ek=; b=zYbH7lXD6fZVHFw9TJKzfTBwLQ1yd4fxv4C7QFIsk5pPinUAAAd8OPYG5VJIAaarFV 0PmedY95J3Mxe5tKodYJltz0R1k6JWZeH0nTWqi7p2o6PEsgMPkALC7RM5paYVGQDjgr JvTa6WRRQiMDTkvbh2ywc2Rz9LUkKQ2+E+wK6550Lfrc6crar4AUPGVs96/KL/tJmkys uRYoXVbXXfJkt8d7pu3jXxmbblr/RwGHHPBBNcro8G5uakezu8y7r+Voc2ZIr5K58Ogh YAaOgE6eOw7KJsVQZvVRdIW4SV/X5SQsciNwM9F58jw9fv5IURYavI4nWu5E/Qfh79sk RjVw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=scWw2wrE; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id w11si1664474ejy.632.2020.09.09.08.08.46; Wed, 09 Sep 2020 08:08:46 -0700 (PDT) Received-SPF: pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@zx2c4.com header.s=mail header.b=scWw2wrE; spf=pass (google.com: domain of netdev-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=zx2c4.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726714AbgIIPH6 (ORCPT + 9 others); Wed, 9 Sep 2020 11:07:58 -0400 Received: from mail.zx2c4.com ([192.95.5.64]:44203 "EHLO mail.zx2c4.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730108AbgIIL7r (ORCPT ); Wed, 9 Sep 2020 07:59:47 -0400 Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTP id 95377b77; Wed, 9 Sep 2020 11:29:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=zx2c4.com; h=from:to:cc :subject:date:message-id:in-reply-to:references:mime-version :content-transfer-encoding; s=mail; bh=WW7WTo+4ZuqjauDDxeTu5QNSG m8=; b=scWw2wrECL47SQ9w+IMlWrHSFf0Yns2mnBP10l9R59+s6Iz3M3juB6gPg APjjwdvcWFRO5nHIHcf8qt5BunZe9AtzbEavJM/bv9z/ZW63W1iecdAhdoKgn+GE os3mglNAx3uqtJNZQS0WPtdcPwrrmg6HxgCYdpad3xCrj0ZMPv4tqjGqDsVqSHus 74eJaL7dQvKB8QL/RyBHcCM4+tOfIBSqarsb7v0blqjUBXwOlUCwCzucruRL4mZ0 TpNtf+H17CiXOHeGSpJ5zXLHKHQADf1lUbkKZg9KAO7d2iDReSox+73waEsrkM+a u1U7k8EACiH8DWNZXhI+SucTq6v5g== Received: by mail.zx2c4.com (ZX2C4 Mail Server) with ESMTPSA id 81b3e82f (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Wed, 9 Sep 2020 11:29:31 +0000 (UTC) From: "Jason A. Donenfeld" To: netdev@vger.kernel.org, davem@davemloft.net Cc: "Jason A. Donenfeld" , Eric Dumazet Subject: [PATCH net 2/2] wireguard: peerlookup: take lock before checking hash in replace operation Date: Wed, 9 Sep 2020 13:58:15 +0200 Message-Id: <20200909115815.522168-3-Jason@zx2c4.com> In-Reply-To: <20200909115815.522168-1-Jason@zx2c4.com> References: <20200909115815.522168-1-Jason@zx2c4.com> MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Eric's suggested fix for the previous commit's mentioned race condition was to simply take the table->lock in wg_index_hashtable_replace(). The table->lock of the hash table is supposed to protect the bucket heads, not the entires, but actually, since all the mutator functions are already taking it, it makes sense to take it too for the test to hlist_unhashed, as a defense in depth measure, so that it no longer races with deletions, regardless of what other locks are protecting individual entries. This is sensible from a performance perspective because, as Eric pointed out, the case of being unhashed is already the unlikely case, so this won't add common contention. And comparing instructions, this basically doesn't make much of a difference other than pushing and popping %r13, used by the new `bool ret`. More generally, I like the idea of locking consistency across table mutator functions, and this might let me rest slightly easier at night. Suggested-by: Eric Dumazet Link: https://lore.kernel.org/wireguard/20200908145911.4090480-1-edumazet@google.com/ Fixes: e7096c131e51 ("net: WireGuard secure network tunnel") Signed-off-by: Jason A. Donenfeld --- drivers/net/wireguard/peerlookup.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.28.0 diff --git a/drivers/net/wireguard/peerlookup.c b/drivers/net/wireguard/peerlookup.c index e4deb331476b..f2783aa7a88f 100644 --- a/drivers/net/wireguard/peerlookup.c +++ b/drivers/net/wireguard/peerlookup.c @@ -167,9 +167,13 @@ bool wg_index_hashtable_replace(struct index_hashtable *table, struct index_hashtable_entry *old, struct index_hashtable_entry *new) { - if (unlikely(hlist_unhashed(&old->index_hash))) - return false; + bool ret; + spin_lock_bh(&table->lock); + ret = !hlist_unhashed(&old->index_hash); + if (unlikely(!ret)) + goto out; + new->index = old->index; hlist_replace_rcu(&old->index_hash, &new->index_hash); @@ -180,8 +184,9 @@ bool wg_index_hashtable_replace(struct index_hashtable *table, * simply gets dropped, which isn't terrible. */ INIT_HLIST_NODE(&old->index_hash); +out: spin_unlock_bh(&table->lock); - return true; + return ret; } void wg_index_hashtable_remove(struct index_hashtable *table,