From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id E3073C282DE for ; Thu, 6 Mar 2025 19:02:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A207C280017; Thu, 6 Mar 2025 14:02:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 9CF3E280016; Thu, 6 Mar 2025 14:02:40 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 84A48280017; Thu, 6 Mar 2025 14:02:40 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 6A070280016 for ; Thu, 6 Mar 2025 14:02:40 -0500 (EST) Received: from smtpin25.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 8B462AC2F4 for ; Thu, 6 Mar 2025 19:02:41 +0000 (UTC) X-FDA: 83192047722.25.60F12DF Received: from out-184.mta1.migadu.com (out-184.mta1.migadu.com [95.215.58.184]) by imf09.hostedemail.com (Postfix) with ESMTP id E7EB014002E for ; Thu, 6 Mar 2025 19:02:38 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=egscfVf1; spf=pass (imf09.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.184 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1741287759; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=szZEP27dn4yWr8sJmNMilKVFtWeJegfQAPwNWoyBPvM=; b=ViJdqeA4KvnNcqISDrC+0hkrQPs9mRtfgjnQppPSigj+Zm+CPMNO+p/MEkP2oagXzpenB0 Fsl1ZKqQrerfEsJG/DeHOQvK52AFc7CXDAZQS21GDMEbfkF8PuE2B93SYBkZWLsrHefbU2 3CTefcUi5lZGrUrwge9Gv9hlTOZikEQ= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=egscfVf1; spf=pass (imf09.hostedemail.com: domain of yosry.ahmed@linux.dev designates 95.215.58.184 as permitted sender) smtp.mailfrom=yosry.ahmed@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1741287759; a=rsa-sha256; cv=none; b=YQxjadtzUsx6ccBES9vbzjdllEby6IThhBZASgqehOu1lo3xab8DuC8V7AE37Vi1W9HIv8 dd220GfqCNppaL693LGdQoLrHb/w6w9FyK/fP2vToW8l27MrZf9M3zadb2VqwK6UdKar5a m9x8CvwPNxFNyfsTpyAaqCKU5j5uFvI= Date: Thu, 6 Mar 2025 19:02:21 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1741287756; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=szZEP27dn4yWr8sJmNMilKVFtWeJegfQAPwNWoyBPvM=; b=egscfVf1IQ1ZMNVzPNyutGBZVHAri6lBJ7YxI6SYHHhTKmhbn/3uYif/d9t7HB0Gz9OcRp EqWG+/PaLIWVjYPUPUa25c3d2FWERbC8lcnYuqJEWdeUYWIAun2zI+i+anS0/KzTLub7DV XhpsVOHBJhxkuohYm1rskugC/FYlYgI= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Yosry Ahmed To: Andrew Morton Cc: Johannes Weiner , Nhat Pham , Chengming Zhou , "David S. Miller" , Herbert Xu , linux-mm@kvack.org, linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, syzbot+1a517ccfcbc6a7ab0f82@syzkaller.appspotmail.com, stable@vger.kernel.org Subject: Re: [PATCH v2] mm: zswap: fix crypto_free_acomp() deadlock in zswap_cpu_comp_dead() Message-ID: References: <20250226185625.2672936-1-yosry.ahmed@linux.dev> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250226185625.2672936-1-yosry.ahmed@linux.dev> X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Stat-Signature: 46kwzyksgh4bkxfiht5bujcyc8p3yc59 X-Rspamd-Queue-Id: E7EB014002E X-Rspamd-Server: rspam07 X-HE-Tag: 1741287758-89555 X-HE-Meta: U2FsdGVkX19nbDvz46f4kpRqS/x7BlELcfm3efWdga0bLiEHnC1kGHqoAttvIVGS6JQk5+jAyIrX1O+lW0c/+NBjg9RWYrqoVKKGZ9GjCnQhse/9MJNgLKJ5Tc3MO5a/2glAf+lB+5/9nTkI774r7h0wAQSlXN01IDphmQ2SXYe+K3HA5hM1LC+HXzX2cZG4m6zK8qzwSsxlVgKNHmIdkXxhm5TmWaK4ttAPzZMQ03LNgkPUKngFGqhvtuZ3HYIVhn+izl93EOT0fbC2ocXx5UDPtc27dqqwvCRCnE2EQwV47caZPqAVn9DsSaMIzq0emfxAbs3oQq22ElC97CXC3FHkkasFJyEub0kLoNxxtcDrftXWTA5033JFH/ndqixErjsp2JE6ZfB8R+vF+OqeWWH9CWcwr1+jT4RcoRXiB8GcJn+KW4htaMGG8kAZltygamo39WQTV5Y+ER7MVSf8j1TSQzelw7yLoPN2f7Ze3u/Z3trEsU6NbPWthNdZzqe/HLFSKPUlDjjoKpdpMmMXWYL6m638ASPWmMofneWuxrOYJv7alze9XG0kII6eH5FY3hEW1pBLBYa8hG2iP2Z3TeRmihYvlp3dPkD+h/N1xamCoKQxNGo4aBXor6M+H918amXH0nem1bHYmzC0Bn/Aq4OQsw7AU29hOmmMsiltKWe2Nb0Gu1C0rL8S622RFzY7bi+214qjClh+G84x5PHktQ87rSIbUEvZQ2leqlcYITMzlGxhK8Bz6qMiHi22hPwuDbZl17VypPlvmsW8YYnp/L/5HDAk5s5xSOZWjAz/IBfUJA4KIdHaaXMZgIllREbpp3t/1dXB7QPPhJBdTv4zEiiGj3mO5CC721byopXpIitFCMNIyZmG0+fVGy+wVUhU9OA6XNXij3uU0MAyGexvx1iLOaaJQOgrW7Pw88tE8np+SuxdR1d0BR8kI0MlijA3YHY0aL+rBu/WMiDnAiA a/RMp1OV q+lFbNLAWsEh7YY/JH5jmomr/+mZH1IypwLucGNiDjvLn9HhjE3fSQUPGIEWP+pvuyM4nkPEPyjDR9XjY1y0RU7Qjg/mPrTQuvD9UhcJYLtr9Ga8E58PQbSa3t3AQqwQOKs6dywenut4/gObffDFRLywa03sxpVnFMk5V/Yw91jhJjOJvpJkzjuW35APd0ayJxmQrhSM3cS8f8VnNxLkdYc0sOHVXWr0scsAz9qO+2emj/fdFdiQ8tyg8ZZucFu9u/YwfiseZKjG2prKriMLdkSWwUAmTppnTtTRyYnbfPESRwNxppM9gCKV1qKX8cGM4GUTGnA/hsSp9jAoXUTa6WsN9ACvMypudR5XDeyiMMsSjtkNP8crBcFFkt9xcxDx0aBqpT+qSgGuO1BxaSD+O1yGmTeFkBI3tscOOjujN3tlv8Vdq2335HqaoSqLyo6iV2efWPMTIVLM2Ou4uZZQgojNV4m9zIJ3DCCb/uR5P0HKty+4= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Feb 26, 2025 at 06:56:25PM +0000, Yosry Ahmed wrote: > Currently, zswap_cpu_comp_dead() calls crypto_free_acomp() while holding > the per-CPU acomp_ctx mutex. crypto_free_acomp() then holds scomp_lock > (through crypto_exit_scomp_ops_async()). > > On the other hand, crypto_alloc_acomp_node() holds the scomp_lock > (through crypto_scomp_init_tfm()), and then allocates memory. > If the allocation results in reclaim, we may attempt to hold the per-CPU > acomp_ctx mutex. > > The above dependencies can cause an ABBA deadlock. For example in the > following scenario: > > (1) Task A running on CPU #1: > crypto_alloc_acomp_node() > Holds scomp_lock > Enters reclaim > Reads per_cpu_ptr(pool->acomp_ctx, 1) > > (2) Task A is descheduled > > (3) CPU #1 goes offline > zswap_cpu_comp_dead(CPU #1) > Holds per_cpu_ptr(pool->acomp_ctx, 1)) > Calls crypto_free_acomp() > Waits for scomp_lock > > (4) Task A running on CPU #2: > Waits for per_cpu_ptr(pool->acomp_ctx, 1) // Read on CPU #1 > DEADLOCK > > Since there is no requirement to call crypto_free_acomp() with the > per-CPU acomp_ctx mutex held in zswap_cpu_comp_dead(), move it after the > mutex is unlocked. Also move the acomp_request_free() and kfree() calls > for consistency and to avoid any potential sublte locking dependencies > in the future. > > With this, only setting acomp_ctx fields to NULL occurs with the mutex > held. This is similar to how zswap_cpu_comp_prepare() only initializes > acomp_ctx fields with the mutex held, after performing all allocations > before holding the mutex. > > Opportunistically, move the NULL check on acomp_ctx so that it takes > place before the mutex dereference. > > Fixes: 12dcb0ef5406 ("mm: zswap: properly synchronize freeing resources during CPU hotunplug") > Reported-by: syzbot+1a517ccfcbc6a7ab0f82@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/all/67bcea51.050a0220.bbfd1.0096.GAE@google.com/ > Cc: > Co-developed-by: Herbert Xu > Signed-off-by: Herbert Xu > Signed-off-by: Yosry Ahmed > Acked-by: Herbert Xu > --- > > v1 -> v2: > - Explained the problem more clearly in the commit message. > - Moved all freeing calls outside the lock critical section. > v1: https://lore.kernel.org/all/Z72FJnbA39zWh4zS@gondor.apana.org.au/ > > --- Hi Andrew, I don't see this patch in any MM tree, I think it's because there were discussions about whether this should be fixed on the zswap side or the crypto side. I am not sure what the status of the crypto fix is, but if no one objects I'd like to get this zswap fix merged anyway if possible. > mm/zswap.c | 30 ++++++++++++++++++++++-------- > 1 file changed, 22 insertions(+), 8 deletions(-) > > diff --git a/mm/zswap.c b/mm/zswap.c > index ac9d299e7d0c1..adf745c66aa1d 100644 > --- a/mm/zswap.c > +++ b/mm/zswap.c > @@ -881,18 +881,32 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) > { > struct zswap_pool *pool = hlist_entry(node, struct zswap_pool, node); > struct crypto_acomp_ctx *acomp_ctx = per_cpu_ptr(pool->acomp_ctx, cpu); > + struct acomp_req *req; > + struct crypto_acomp *acomp; > + u8 *buffer; > + > + if (IS_ERR_OR_NULL(acomp_ctx)) > + return 0; > > mutex_lock(&acomp_ctx->mutex); > - if (!IS_ERR_OR_NULL(acomp_ctx)) { > - if (!IS_ERR_OR_NULL(acomp_ctx->req)) > - acomp_request_free(acomp_ctx->req); > - acomp_ctx->req = NULL; > - if (!IS_ERR_OR_NULL(acomp_ctx->acomp)) > - crypto_free_acomp(acomp_ctx->acomp); > - kfree(acomp_ctx->buffer); > - } > + req = acomp_ctx->req; > + acomp = acomp_ctx->acomp; > + buffer = acomp_ctx->buffer; > + acomp_ctx->req = NULL; > + acomp_ctx->acomp = NULL; > + acomp_ctx->buffer = NULL; > mutex_unlock(&acomp_ctx->mutex); > > + /* > + * Do the actual freeing after releasing the mutex to avoid subtle > + * locking dependencies causing deadlocks. > + */ > + if (!IS_ERR_OR_NULL(req)) > + acomp_request_free(req); > + if (!IS_ERR_OR_NULL(acomp)) > + crypto_free_acomp(acomp); > + kfree(buffer); > + > return 0; > } > > -- > 2.48.1.658.g4767266eb4-goog >